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

Resolver is too strict in its multiple-crates-links-ing check #5969

Open
kamalmarhubi opened this issue Sep 3, 2018 · 15 comments
Open

Resolver is too strict in its multiple-crates-links-ing check #5969

kamalmarhubi opened this issue Sep 3, 2018 · 15 comments
Labels
A-links Area: `links` native library links setting A-optional-dependencies Area: dependencies with optional=true

Comments

@kamalmarhubi
Copy link
Contributor

I'm attempting to use a feature to allow controlling which version of a -sys crate is used. It looks like I'm running afoul of the restriction on multiple crates having a links specified for the same library, even though in any particular invocation only one will be used.

Does the resolver ignore features when deciding if a library is being linked multiple times? It seems that it is, and I think that it should only check the final resolved dependency tree for the target being built, and in particular after the features are applied.

@alexcrichton
Copy link
Member

cc @Eh2406, would you be able to help out with this one?

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 4, 2018

@kamalmarhubi that is an important issue, but I am not quite seeing how you were proposing to use features to solve it. What prevents someone from specifying that they need both features? Can you point me at what you have tried so far?

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 22, 2018

Note that #6179 (comment) wuld provide a workaround by using --minimal-cargo-lock to test only one of the features at a time. (you would need to run the tests twice once with each feature)

@kornelski
Copy link
Contributor

kornelski commented Oct 29, 2018

I know that features are additive, and I would like to use it as advantage: emit an error if and only if the final union of all enabled features ends up pulling two crates with conflicting links attributes. If feature settings actually happen to pull only one crate, then don't err.

Regarding lockfile, maybe build the lockfile while completely ignoring existence of links (allowing the lockfile to contain links-conflicting crates), and check links conflicts later as a pass only on the subset of crates that have been selected by the features. In other words, it should be build-time check, not update-time check.

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 29, 2018

Features being additive means that if it work with feature A and it works with feature B then it must work with A and B. I know that is not how features are used in practise, but that is the axiom on which they are designed.

Checking links conflicts later unfortunately, fails with an even worse ux problem. If the resolver is unaware of the links attribute, then it easily can decide that 2 of your dependencies 10 or 100 layers deep should use different versions of some sys crate you have never heard of. What ux is there to fix it? You can do the NP-Hard problem of resolving dependencies in your head, figure out which decision (of the thousands) the resolver made incorrectly. If you are lucky pin to the correct version in your Cargo.toml with a blabla = "=1.2.24" # only here to get the correct version of foo-sys. If you are not lucky, then the only thing you can do is write the cargo.lock by hand. What is worse is that this complete breakdown of ux, has everyone involved using everything correctly. The only party at fault is the cargo for making a lockfile that it was then going to refuse to bild.

@kornelski
Copy link
Contributor

The example you've linked to contains two semver-major-incompatible sqlite dependencies (0.x acts as major), so I think it's working as intended, and there is simply no solution for that dependency graph. I don't see how using links in resolution could change anything about it.

The UX of dealing with conflicts is indeed poor. However, I think not looking at unused crates would make conflicts rarer, so it should actually improve the situation.

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 29, 2018

">=0.8.0, <0.10.0" means that it will take one of several different semver-major-incompatible versions of sqlite. The resolution where both take v0.9.x is the only one that will build. If the resolver doesn't know about links then it will take the opportunity to give desle the newest v0.10.x. (the test that demonstrates the difference in resolution)

We do ignore the unused optional crates in your dependencies. see my earlier reply. I believe this provides a way to work around this limitation in the functionality of features. I suspect that the number of times there are links related conflicts in optional dependencies in the crate being developed is small.

@kornelski
Copy link
Contributor

kornelski commented Oct 29, 2018

In previous reply you've mentioned something about tests, but I don't see how that's related. The problem is reproducible with an empty project and just two dependencies (they are not dev deps, they're supposed to be usable in the final product).

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 29, 2018

I am sorry for not explaining myself clearly. I am also sorry for coming across as rude and dismissive. You deserve a clear, explained, and reproducible response. Give me a little while to make sure I have my facts straight; I'd hate to spend the time to have a high quality right up that is completely wrong.

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 30, 2018

Attached is a zip file, containing the folder structure I used to check that I was correct.
issues6231.zip

The setup
For reference here is the tree of that folder:

+---hard
|   |   Cargo.toml
|   \---src
|           lib.rs
+---tester1
|   |   Cargo.toml
|   \---src
|           main.rs
\---tester2
    |   Cargo.toml
    \---src
            main.rs

The library hard is very similar to your excellent example here

[package]
name = "hard"
version = "0.1.0"

[features]
default = ["libz-sys"]

[dependencies]
libz-sys = { version = "1.0.25", optional = true }
cloudflare-zlib-sys = { version = "0.1", optional = true }

Just for testing, I added some code that depended on which features are on:

#[cfg(feature = "libz-sys")]
pub fn thing() -> &'static str {
    "libz"
}

#[cfg(feature = "cloudflare-zlib-sys")]
pub fn thing() -> &'static str {
    "cloudflare-zlib"
}

The problem There is no way to build "hard". A cargo build in the hard folder will give an error.

The solution workaround Add an new project that depends on hard and thereby test if hard can be used.

// tester1/Cargo.toml
[package]
name = "tester1"
version = "0.1.0"

[dependencies]
hard = {path="../hard", version="*", default-features = false, features = ["cloudflare-zlib-sys"]}

It can be built just fine and when it calls hard::thing() it gets "cloudflare-zlib".
To test the other configuration,

// tester2/Cargo.toml
[package]
name = "tester1"
version = "0.1.0"

[dependencies]
hard = {path="../hard", version="*"}

It can be built just fine and when it calls hard::thing() it gets "libz".

So there is a way to make a shim library that can switch between two crates that have the same links attribute. It is painful to develop in, but it works.

@kornelski
Copy link
Contributor

Thank you very much for your explanation and the example files.

The result is very surprising to me, since I can build tester1 with hard, tester2 with hard, but not hard itself.

@Eh2406
Copy link
Contributor

Eh2406 commented Oct 30, 2018

It is not intuitive. I would go so far as to say if we had to remake features from scratch this would not be the behaviour we would have designed. There are any number of "maybe someday we could" idys that may make this better without regressing the usability. Few of them are close to being designed let alone implemented. At least there is a workaround.

@maxammann
Copy link

Attached is a zip file, containing the folder structure I used to check that I was correct.
issues6231.zip

I just experiemented with this example. If you are using workspaces and both library configurations are "reachable" within that workspace (choosing once libz-sys and once the cloudflare one), then this does not work.
I think the reason for this is that two Cargo.lock files are created in if you are not usage workspaces. In the workspace a single Cargo.lock is used.

@simonbuchan
Copy link

It still conflicts even in the [target.'cfg(foo)'.dependencies] vs [target.'cfg(not(foo))'.dependencies], which is pretty frustrating.

In my case, I have a conflict in switching between web-view and wry with a feature on windows (where the former doesn't have a dependency, but the latter can be debugged), but there is a links conflict for the library both use on linux :|

Tastaturtaste added a commit to Tastaturtaste/argmin that referenced this issue Oct 6, 2023
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: argmin-rs#368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
Tastaturtaste added a commit to Tastaturtaste/argmin that referenced this issue Oct 6, 2023
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: argmin-rs#368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
Tastaturtaste added a commit to Tastaturtaste/argmin that referenced this issue Oct 6, 2023
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: argmin-rs#368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
Tastaturtaste added a commit to Tastaturtaste/argmin that referenced this issue Oct 6, 2023
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: argmin-rs#368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
Tastaturtaste added a commit to Tastaturtaste/argmin that referenced this issue Oct 6, 2023
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: argmin-rs#368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
Tastaturtaste added a commit to Tastaturtaste/argmin that referenced this issue Oct 6, 2023
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: argmin-rs#368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
Tastaturtaste added a commit to Tastaturtaste/argmin that referenced this issue Oct 6, 2023
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: argmin-rs#368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
@epage
Copy link
Contributor

epage commented Oct 23, 2023

I have a proposal for global, mutually exclusive features (https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618) and hadn't considered this case! I wonder if there is a way we could tell cargo with that feature that this is safe.

Tastaturtaste added a commit to Tastaturtaste/argmin that referenced this issue Jan 4, 2024
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: argmin-rs#368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
Tastaturtaste added a commit to Tastaturtaste/argmin that referenced this issue Jan 12, 2024
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: argmin-rs#368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
stefan-k pushed a commit to argmin-rs/argmin that referenced this issue Jan 14, 2024
Due to the additivity requirement on cargo features even for features
that are not enabled simultaneously, together with the requirement that
only one crate in the dependency tree specifies any `links` attribute,
it was not possible to test multiple backend versions for ndarray-linalg
by features: #368 (comment)
Previously this approach worked for different versions of
ndarray-linalg insofar as the used backend `netlib` just coincidentally
remained the same for all tested ndarray-linalg versions.
This is not the case for the intel-mkl backend, which is desirable to
use as the default because it works on all three major platforms. To
enable the change of the backend as well as make the testing more
robust against future changes in ndarray-linalg backend versions, the
tests were factored out into their own crates. This works because the
strict additivity requirement for inactive features is not enforced for
dependencies: rust-lang/cargo#5969 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-links Area: `links` native library links setting A-optional-dependencies Area: dependencies with optional=true
Projects
None yet
Development

No branches or pull requests

8 participants