Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make pico-args a binary-only dependency #686

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

CGMossa
Copy link
Contributor

@CGMossa CGMossa commented Nov 30, 2023

This library currently includes pico-args as a dependency, but this is very much a binary-specific dependency
Thus I've made it optional, and added as a feature to the binary.

This will make this library less congested with non-library things.

In addition, resolver = "2" was set, as this isn't the case for workspaces,
it is the case for crates falling under 2021 editions -- but not workspaces.

@RazrFalcon
Copy link
Owner

Hm... I remember having issues with making pico-args an optional dependency. But I guess it works just fine now.
Thanks!

@RazrFalcon RazrFalcon merged commit 9f3aa52 into RazrFalcon:master Nov 30, 2023
3 checks passed
@CGMossa CGMossa deleted the optional_pico_args branch November 30, 2023 13:00
@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 30, 2023

I don't know.. maybe the resolver thing helped, as it is related to feature resolution, or something...
thanks for merging this!

@RazrFalcon
Copy link
Owner

I remembered why:

> cargo run
error: target `resvg` in package `resvg` requires the features: `text`, `system-fonts`, `memmap-fonts`, `pico-args`
Consider enabling them by passing, e.g., `--features="text system-fonts memmap-fonts pico-args"`

Unless you have an idea how to avoid it I would have to revert this PR.

PS: no, passing --features each time is not a solution.

@LaurenzV
Copy link
Contributor

LaurenzV commented Dec 3, 2023

Only solution I can think of is to split resvg as a library and as a CLI into two different crates, which would be kind of unfortunate I guess.. Not sure if there is another way.

@CGMossa
Copy link
Contributor Author

CGMossa commented Dec 4, 2023

Sorry, I've been sick. @RazrFalcon. I did not expect this behavior from cargo, this is very unforunate, but it makes sense.
I'd like to suggest this change to the Cargo.toml,

[features]
default = ["standalone", "pico-args"]

# Necessary features for using resvg as a library
standalone = ["text", "system-fonts", "memmap-fonts", "raster-images"]

What do you think?

@RazrFalcon
Copy link
Owner

What do you think?

Unnecessary complexity. pico-args is designed to be tiny. It doesn't affect anything. It wouldn't be compiled into the library to begin with.

The only solution is to wait for cargo to support binary-dependencies. If it ever happen.

@CGMossa
Copy link
Contributor Author

CGMossa commented Dec 4, 2023

That's fine. I may write up an issue on cargo, and I've also discovered another cargo limitation that's quite annoying.

Can I recommend that you put in the resolver ="2" in the workspace Cargo.toml.
It's not default behavior for workspaces, only for packages, and maybe it will shield you from bugs with cargo in the future. 😅

@RazrFalcon
Copy link
Owner

I don't like adding thing I don't understand the purpose of. Everything works just fine without resolver=2. Why do I need it?

@CGMossa
Copy link
Contributor Author

CGMossa commented Dec 4, 2023

Upon investigating, your crates uses edition = "2018". The resolver is set to 1 in those, and thus it works fine for the workspace.

I've just seen several issues with cryptic error messages, due to using the old resolver. See
gfx-rs/wgpu#2356
rust-lang/cargo#9956 (here at the bottom there are many mentions specifically to this)

I'm going to investigate this a bit more...

@RazrFalcon
Copy link
Owner

Does updating to 2021 would solve the issue? If so, I plan to do this during the next resvg release. We require MSRV 1.65 anyway.

@CGMossa
Copy link
Contributor Author

CGMossa commented Dec 4, 2023

I think if your crates are "2021" and your workspace doesn't have that explicitly set, then you might get weird issues like the once wgpu, and wgpu-hal received.

I regret bringing this up, but if you upgrade editions, you'd probably need resolver = "2" as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants