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

Add cargo-vendor-filterer in devel version and drop it from CI #754

Merged
merged 7 commits into from
Jan 28, 2025

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Jan 23, 2025

Ubuntu devel already contains rust-cargo-vendor-filterer, we can't yet
build-depend on it in debian/control for everybody (as it's not in noble
yet), but we can still workaround this.

A change to dh-rust (just after the last run of the
build-deb run on the PR), broke the usage of cargo-vendor-filterer
when installed from cargo itself.

This is ugly, but avoids us to branch for now.


However... This still doesn't work for plucky because of this bug:

UDENG-5865

@3v1n0 3v1n0 requested a review from a team as a code owner January 23, 2025 18:08
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.11%. Comparing base (36511cd) to head (b55bfa2).
Report is 268 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
- Coverage   83.43%   83.11%   -0.33%     
==========================================
  Files          83       96      +13     
  Lines        8689     9646     +957     
  Branches       74       74              
==========================================
+ Hits         7250     8017     +767     
- Misses       1111     1244     +133     
- Partials      328      385      +57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@3v1n0
Copy link
Collaborator Author

3v1n0 commented Jan 23, 2025

Ok, it will be fixed by https://code.launchpad.net/~juliank/ubuntu/+source/dh-cargo/+git/dh-cargo/+merge/480016

We may not need the ci / debian/rules change, so maybe I can go back to my initial change (3v1n0@df187b1)

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Right, not the most elegant solution, but that should do it for now.

3v1n0 added 5 commits January 27, 2025 21:47
Ubuntu devel already contains rust-cargo-vendor-filterer, we can't yet
build-depend on it in debian/control, but for now we should install it
as build-dependency.
It's not used by dh-cargo anymore
Ubuntu devel already contains rust-cargo-vendor-filterer, we can't yet
build-depend on it in debian/control for everybody (as it's not in noble
yet), but we can still workaround this.

A change to dh-rust (just after the last run of the
build-deb run on the PR), broke the usage of cargo-vendor-filterer
when installed from cargo itself.

This is ugly, but avoids us to branch for now.
If cargo-vendor-filter is used to generate the debian/control vendored
sources it won't include the windows and other filtered crates.

However the version that is in noble is not ready for this since it does
not support filtering and then we'd just fail at configure phase,
expecting the filtered crates to be there.

As per this, in the cargo filterer missing case, just do manually what
dh-cargo does without the vendoring checks
@3v1n0 3v1n0 force-pushed the deb-ci-install-filterer-from-repos branch from e484665 to 1d82b25 Compare January 27, 2025 20:52
This is what dh-cargo expects in plucky, so let's track devel, while
noble won't perform such check
If cargo home is set we should not remove it during cleanup
@3v1n0
Copy link
Collaborator Author

3v1n0 commented Jan 28, 2025

@didrocks needed to add other changes as the dh-cargo in plucky now properly checks on filtered vendored sources to generate XS-Vendored-Sources-Rust, so we needed either to fully fork or workaround it in the meanwhile.

For now I think ignoring the result of /usr/share/cargo/bin/dh-cargo-vendored-sources in noble is fine since we're running it anyways during CI jobs (see #760) for plucky, while for rust it would generate a wrong value anyways (since we do filter those sources).

@didrocks
Copy link
Member

@didrocks needed to add other changes as the dh-cargo in plucky now properly checks on filtered vendored sources to generate XS-Vendored-Sources-Rust, so we needed either to fully fork or workaround it in the meanwhile.

For now I think ignoring the result of /usr/share/cargo/bin/dh-cargo-vendored-sources in noble is fine since we're running it anyways during CI jobs (see #760) for plucky, while for rust it would generate a wrong value anyways (since we do filter those sources).

Ack, thanks on the rationale! Sounds good to me.

@3v1n0 3v1n0 merged commit cb4eb06 into ubuntu:main Jan 28, 2025
16 checks passed
3v1n0 added a commit that referenced this pull request Jan 28, 2025
#760)

As per the plucky changes on dh-cargo we need to use a filtered
`XS-Vendored-Sources-Rust` value in `debian/control`.

The logic we had in the CI job that is in charge of updating such field
become then incorrect.
So, use the same logic that we have during debian package cleanup and
support targeting different branches.

For now we don't fully branch for noble but this allows to be prepared
to that event (that is likely to have to happen soon).

Fixed also an issue that lead to ignoring dh-cargo issues when running
the script.

It shares few commits with #754

UDENG-5865
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.

3 participants