search

cardoe / cargo-ebuild

cargo extension that can generate ebuilds using the in-tree eclasses
Apache License 2.0
75 stars 13 forks source link

feat: include all features when pulling metadata #41

Open pdemonaco opened 4 years ago

pdemonaco commented 4 years ago

Optional package dependencies are not included when pulling metadata. In some cases these packages are still required to actually build even when those flags are turned off.

This change effectively adds --all-features to the call of cargo metadata.

I don't know if this is due to a change in behavior from cargo build, however, it seems prudent to include all possible dependencies.

Possibly fixes #31

leonardohn commented 4 years ago

I don't think it is desirable to add all features for any package by default, although there should be an option to choose which features to enable. In this issue there are some examples of packages that doesn't work when all features are enabled.

pdemonaco commented 4 years ago

This is not actually enabling features, just including all of the crate uri definitions so the files are downloaded by portage in the eventual ebuild.

The offline version of cargo build performed by the cargo.eclass will fail if it cannot retrieve files for optional dependencies even if they are not enabled. I discovered this while developing an ebuild for i3status-rust. In this case, the Cargo.toml includes an optional profile flag which depends on cpuprofiler and profiling. The proto ebuild generated by cargo-ebuild omitted the crate entries for these components and several of their dependencies. Once I included them via the change in this pull request the class built successfully.

The ebuild author still has to control the feature flags which are provided to cargo_src_compile to actually cause these components to build.

leonardohn commented 4 years ago

But what if you enable a feature that actually removes a dependency and adds another?

Please try generating the ebuild using the latest git version (0.3.2), as it now uses Cargo.lock to retrieve the dependency tree; Unfortunately it still doesn't support features other than default, but for most of the crates I've tested it is now resolving the dependencies accordingly.

pdemonaco commented 4 years ago

Ah. They now seem to be generating the same code so I'm not sure whether my change actually does anything.

At some level I think we're talking past each other, although, perhaps I'm not fully understanding rust.

As the features are enabled at compile time what possible harm is there to downloading additional crates which might not be used by the enabled features? As I understand it, the crate URIs are simply used by cargo.eclass to retrieve relevant sources. I think I've seen ebuilds in which use flags also change the sources which are downloaded, however, that seems needlessly complicated to manage, particularly for something like rust crates.

I think it's actually desirable to have all the crates as an ebuild author may configure features dynamically via use flags. Obviously they'll still need to ensure that use flags which would enable conflicting features cannot be enabled simultaneously but this is nothing new.

leonardohn commented 4 years ago

I'm just trying to understand possible edge cases so that no cargo-ebuild users would suffer from regressions.

As I've been finding out, there is no way to remove a dependency by enabling a feature, which means that the edge case that I've mentioned can't really happen.

However, the new version which uses Cargo.lock to resolve dependencies already contains any optional dependencies that could be enabled by features or even different architectures, which should already fix the problem that you are facing.

leonardohn commented 4 years ago

@cardoe I've tested this patch with rust-analyzer which wasn't working properly before v0.3.1 and surprisingly it leads to the same results as v0.3.2 which uses cargo-lock. So, could this be a better fix for the dependency resolution problems?

TomHotston commented 3 years ago

Hi there,

I've recently been introduced to this project from the forum. I'm new to Gentoo but have previously been using the cargo tree command to get the required dependencies.

cargo tree --all-features --target all --prefix none

I found this to actually catch all the features required and have tested this against the packages in the repositories and in some of the Bugzilla posts.

Using the --all-features flag is the only way to catch all of these dependencies required by the rebuild to actually build. In effect, this enables all the dependencies from all of the possible features and will duplicate any dependencies if different versions are required.

Somewhat stupidly I actually implemented this myself before checking for any issues or PRs here. After testing that my theory about using the --all-features does this with the cargo ebuild command it will generate the same output as manually adding the dependencies as they're suggested by the ebuild itself.

I would strongly suggest that this change could be highly valuable and solve some of the issues with missing features. In essence --all-features is a bad name for this flag as it is better described as all possible dependencies, which in my limited experience is what is required by ebuild.

gyakovlev commented 3 years ago

this project has been moved to https://gitweb.gentoo.org/proj/cargo-ebuild.git and mirrored to https://github.com/gentoo/cargo-ebuild for pull requests.

this repo still remains but should be marked as deprecated soon.

I've merged this PR and tagged 0.3.2 https://gitweb.gentoo.org/proj/cargo-ebuild.git/commit/?id=148fc783d6c6d00ea35e56e861cb5bc42d8a49c2 it seems to be catching deps better now. thanks for doing the work.