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

Improve per-platform feature selection. #478

Merged
merged 41 commits into from Jun 20, 2022
Merged

Conversation

sayrer
Copy link
Contributor

@sayrer sayrer commented Mar 28, 2022

This patch should fix #451.

The patch runs cargo-tree to determine which features are enabled on each platform, and then groups them by using vectors of platforms as map keys. It's a lot of code, and there are no tests yet. But I wanted to post this initial effort and get some feedback from the code owners. @illicitonion has already seen this code, but I believe it will be new to @dfreese and @PiotrSikora (who filed the linked issue).

The new file is copyright AgileBits, as this was work for hire for them. They are fine with contributing, and my repo has been public for months.

@sayrer
Copy link
Contributor Author

sayrer commented Mar 28, 2022

I've looked around the examples and the impl/ directory, and I'm not sure where to add this crate: https://github.com/lambda-fairy/rust-errno/blob/main/Cargo.toml

This patch should consolidate the unix and wasi requirements for the libc crate required there.

Copy link
Collaborator

@dfreese dfreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I'm not thrilled by the idea of having to invoke and parse cargo-tree for every platform, but cargo isn't giving good alternatives.

I'm generally supportive of the approach. Are there any other tactics for this in the wild?

impl/src/features.rs Outdated Show resolved Hide resolved
impl/src/features.rs Outdated Show resolved Hide resolved
impl/src/features.rs Outdated Show resolved Hide resolved
impl/src/features.rs Outdated Show resolved Hide resolved
impl/src/features.rs Outdated Show resolved Hide resolved
impl/src/features.rs Show resolved Hide resolved
@illicitonion
Copy link
Collaborator

I'm also generally supportive of the approach given the state of cargo, but sad that there don't seem to be viable alternatives - thanks so much for putting this together and upstreaming it 👍

@sayrer
Copy link
Contributor Author

sayrer commented Mar 30, 2022

It looks like this patch will not fix errno, but it will fix a bunch of related issues with features. I must have another, shorter, patch in my code for the problem in deps.

As for approaches to this problem, I only found two choices: Import cargo as a crate directly, and use its internal resolver implementation (which is pub), or use cargo-tree.

I chose to use cargo-tree, because taking cargo internals as a dependency seemed even less appealing than what I have here. I haven't seen anyone else solve this problem in the wild, and I definitely looked before writing this code.

@leebenson
Copy link

Very much looking forward to this landing!

@dfreese
Copy link
Collaborator

dfreese commented Apr 6, 2022

Sorry, missed that you had also responded to review comments as well. I'm good with this as long as the new code can be covered by unit tests where reasonable.

The copyright/license notice probably needs to be updated, but I'm not entirely sure what needs to happen given the CLA. @hlopko do you have any familiarity with that?

impl/src/features.rs Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@sayrer
Copy link
Contributor Author

sayrer commented Apr 10, 2022

I have some better tests coming, and I still need to go through the ? early exits and give them some messages.

@dfreese
Copy link
Collaborator

dfreese commented Apr 10, 2022

I have some better tests coming, and I still need to go through the ? early exits and give them some messages.

Sounds good. Thanks for the update!

Copy link
Collaborator

@dfreese dfreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handful of comments. Generally looks good to me. Thanks for putting all of this together.

Can you rebase to HEAD as well just to make sure no issues have cropped up?

impl/src/features.rs Outdated Show resolved Hide resolved
impl/src/features.rs Outdated Show resolved Hide resolved
impl/src/rendering/bazel.rs Outdated Show resolved Hide resolved
@sayrer
Copy link
Contributor Author

sayrer commented Jun 18, 2022

Will address these, but I think the Dependabot stuff and #482 should go in first. Not in a rush over the weekend.

@sayrer
Copy link
Contributor Author

sayrer commented Jun 19, 2022

OK, I think this is all set.

Copy link
Collaborator

@dfreese dfreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, one question.

impl/src/rendering/templates/partials/features.template Outdated Show resolved Hide resolved
@dfreese
Copy link
Collaborator

dfreese commented Jun 20, 2022

Great. Thanks. Can you merge with main and rerun bootstrap? Then I think this is good to go. Thanks for all the work on this.

@sayrer
Copy link
Contributor Author

sayrer commented Jun 20, 2022

Merged main, running bootstrap had no effect vs the commit with the Tera fix.

@dfreese dfreese merged commit 743bd26 into google:main Jun 20, 2022
@dfreese
Copy link
Collaborator

dfreese commented Jun 20, 2022

Released in v0.16.0

@PiotrSikora
Copy link
Contributor

I'm not sure if this is intentional or not, but rules for crates with default = ["std"] no longer include:

crate_features = [
    "default",
    "std",
],

(e.g. third_party/cargo/remote/BUILD.blake2b_simd-0.5.11.bazel in this PR), which brakes crates that verify that they were built with "std" feature enabled (e.g. BurntSushi/aho-corasick).

@bsilver8192
Copy link
Contributor

Regenerating the examples with this PR makes many of them fail to build because no crates have any features enabled. I'm looking at fixing it, FYI for anybody else encountering random build failures.

bsilver8192 added a commit to bsilver8192/cargo-raze that referenced this pull request Sep 14, 2022
Currently many things fail to build due to google#478 stripping out all the
features.
bsilver8192 added a commit to bsilver8192/cargo-raze that referenced this pull request Sep 14, 2022
It works properly with no explicit targets set (aka "use all the
targets"), and it also works properly with some of the examples that set
`default-members = []`.

I think there's still something wrong, because it dropped features from
some Windows-specific crates relative to before google#478, but I'm not sure
what's supposed to happen. At least all the tests in this repository
pass on Linux again...
dfreese pushed a commit that referenced this pull request Nov 25, 2022
It works properly with no explicit targets set (aka "use all the
targets"), and it also works properly with some of the examples that set
`default-members = []`.

I think there's still something wrong, because it dropped features from
some Windows-specific crates relative to before #478, but I'm not sure
what's supposed to happen. At least all the tests in this repository
pass on Linux again...
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.

Duplicated deps when multiple targets share dependencies
6 participants