Skip to content
This repository has been archived by the owner on Jan 8, 2022. It is now read-only.

feat: include all features when pulling metadata #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pdemonaco
Copy link

@pdemonaco pdemonaco commented Aug 29, 2020

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

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.
@leonardohn
Copy link
Contributor

leonardohn commented Aug 29, 2020

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
Copy link
Author

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
Copy link
Contributor

leonardohn commented Aug 31, 2020

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link

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
Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

0.3.0 having problems with optional deps
4 participants