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

[stable-1.80] fix(vendor): Strip excluded build targets #14369

Closed
wants to merge 5 commits into from

Conversation

epage
Copy link
Contributor

@epage epage commented Aug 8, 2024

Stable backports

In order to make CI pass, the following PRs are also cherry-picked:

@epage epage marked this pull request as draft August 8, 2024 00:53
@rustbot
Copy link
Collaborator

rustbot commented Aug 8, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Aug 8, 2024

⚠️ Warning ⚠️

  • Pull requests are usually filed against the master branch for this repo, but this one is against rust-1.80.0. Please double check that you specified the right target!

@rustbot rustbot added A-manifest Area: Cargo.toml issues Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2024
@@ -220,7 +220,7 @@ fn sync(
let pathsource = PathSource::new(src, id.source_id(), gctx);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is targeting rust-1.80.0. Did that as a placeholder until we have rust-1.80.2

Copy link
Member

Choose a reason for hiding this comment

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

That branch is for 1.80 and would never change as far as I can tell.

@@ -220,7 +220,7 @@ fn sync(
let pathsource = PathSource::new(src, id.source_id(), gctx);
Copy link
Member

Choose a reason for hiding this comment

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

That branch is for 1.80 and would never change as far as I can tell.

@epage
Copy link
Contributor Author

epage commented Aug 8, 2024

weihanglo approved these changes

FYI, this intentionallty is broken. I wanted to get this up for the conversation but I still need to work through the build breakages from other parts of cargo being different and then figure out what unblocking backports are needed.

@weihanglo
Copy link
Member

@rfcbot fcp merge

I propose to backport #14368

There is a bug in 1.80.0/1.80.1 that cargo vendor generates broken unbuildable vendored Git sources when

  • The git dependency contains a build script
  • The build script is explicitly excluded via package.exclude.

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 8, 2024

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Aug 8, 2024
Since we already cover this for `cargo package` and we turn all
implicit targets into explicit targets (making implicit tests cover
explicit cases), this becomes redundant.
This is a **very** hacky solution, duplicating the minimum of what
`prepare_for_publish` does to fix this one issue and in the least
intrusive way to the vendor code.

The intention is to keep this low risk for backporting to beta and
stable.
We need to revisit this, refactoring the `cargo package` code so that we
can call into that for each vendored dependency.

Fixes rust-lang#14348
@epage
Copy link
Contributor Author

epage commented Aug 8, 2024

FYI I've given a heads up to the release team at https://rust-lang.zulipchat.com/#narrow/stream/241545-t-release/topic/1.2E80.2E2

@weihanglo
Copy link
Member

Should we bump the version of cargo crate to 0.81.1?

@epage
Copy link
Contributor Author

epage commented Aug 8, 2024

According to @pietroalbini, this doesn't meet the bar for a new point release, only piggy-backing off of an existing point release, as this is limited to cargo vendor users and can be worked around by running cargo vendor on a different version.

This aligns with our last cargo vendor breakage (#11192). That was broken in 1.64 and we didn't fix it until 1.68 (#11414). It doesn't look like we even did a beta backport for that.

@weihanglo
Copy link
Member

This aligns with our last cargo vendor breakage (#11192). That was broken in 1.64 and we didn't fix it until 1.68 (#11414). It doesn't look like we even did a beta backport for that.

I have no strong inclination for stable backport after you pointed this out.

@epage epage closed this Aug 8, 2024
@rfcbot rfcbot removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants