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.

Sign up for GitHub

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

Jump to bottom

xtask-unpublished: output a markdown table #12085

Merged
merged 3 commits into from May 5, 2023
Merged

xtask-unpublished: output a markdown table #12085

merged 3 commits into from May 5, 2023

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented May 4, 2023 •
edited

What does this PR try to resolve?

Part of #12033

The purpose of these changes is to make sure that every subcrate gets a version bump if their source files change.

This turns out to be adding a table output for xtask-unpublished. Will address CI issue later on.

How should we test and review this PR?

This extends xtask-unpublished to

  • respects --package flag
  • outputs a markdown table of crate publish statuses

This also marks capture cargo-test-support, and cargo-test-marco crates as publish = false.

To review, you may want to

  • Review it by commits.
  • Run cargo unpublished and check the output correctness.

Additional information

Initially, I was going to run gh pr comment --body <comment> to write a comment on the pull request. I had a basic example here. However, it seems to have some safety concerns 1 — we may grant too many permissions to a malicious PR author to mess up our pull requests.

I stopped to provide this new CI job only. No any new GitHub Action permission required.

If you got a good idea to post a comment without sacrificing CI security and complexity, I am happy to know!

Footnotes

  1. https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ ↩

@rustbot
Copy link
Collaborator

rustbot commented May 4, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-testing-cargo-itself Area: cargo's tests S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2023
@weihanglo weihanglo marked this pull request as draft May 4, 2023 21:48
@weihanglo
Copy link
Member Author

weihanglo commented May 4, 2023 •
edited

  • Should make it work well with @bors try

It works! See https://github.com/rust-lang/cargo/actions/runs/4891160691/jobs/8731327847.

@weihanglo weihanglo marked this pull request as ready for review May 4, 2023 23:16
@weihanglo weihanglo force-pushed the ci-check-version-bump branch 3 times, most recently from ffa4702 to 01b3f1e Compare May 5, 2023 07:49
@weihanglo
Copy link
Member Author

@bors try

@bors
Copy link
Collaborator

bors commented May 5, 2023

⌛ Trying commit 01b3f1e with merge 4cff2cc...

bors added a commit that referenced this pull request May 5, 2023
@bors
Auto merge of #12085 - weihanglo:ci-check-version-bump, r=<try>
4cff2cc 
ci: check if a crate needs a version bump when source files change
@bors
Copy link
Collaborator

bors commented May 5, 2023

💥 Test timed out

epage
epage reviewed May 5, 2023
View reviewed changes
ci/validate-version-bump.sh Outdated
Comment on lines 12 to 17
set -euo pipefail

# When `BASE_SHA` is missing, we assume it is from bors merge commit,
# so `HEAD~` should find the previous commit on master branch.
base_sha=$(git rev-parse "${BASE_SHA:-HEAD~}")
commit_sha=$(git rev-parse "${COMMIT_SHA:-HEAD}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I was assuming this would be written as an xtask, rather than a shell script wrapping one. It'd make it easier to identify changes that way.

@weihanglo
xtask-unpublished: respect --package flag
8d753a7
@weihanglo
chore: publish = false for pkgs not meant to be published
0bffcbc 
These tree packages are considered to be published something.
To reduce the logic of xtask-unpubilshed, let's flag them false for now.
We can always set it `true` when needed.
@weihanglo weihanglo changed the title ci: check if a crate needs a version bump when source files change xtask-unpublished: output a markdown table May 5, 2023
@weihanglo
Copy link
Member Author

r? @epage

Tweaked it a bit. Thanks for the review!

@rustbot rustbot assigned epage and unassigned ehuss May 5, 2023
@epage
Copy link
Contributor

epage commented May 5, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented May 5, 2023

📌 Commit 8620f5a has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2023
@bors
Copy link
Collaborator

bors commented May 5, 2023

⌛ Testing commit 8620f5a with merge 569b648...

epage added a commit to epage/cargo that referenced this pull request May 5, 2023
@epage
feat(xtask): 'changes' xtask
32818f3 
This will report what commits affect each publishable package and serves
as an alternative to rust-lang#12085.

CI can run this as `cargo changes HEAD~ HEAD^2` and that will capture
all of the commits within the PR (this is the range I used in
`committed`).

There is still work needed to integrate this with an action; this is
just a rough base-line implementation.

I originally tried to use `cargo package --list` to determine what files
belong to what packages but that was taking too long for some reason (it
works well in `cargo release` for my crates).  We can either look into
that in the future or do our own heuristics to filter out content.
@epage epage mentioned this pull request May 5, 2023
Closed
@bors
Copy link
Collaborator

bors commented May 5, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 569b648 to master...

@bors bors merged commit 569b648 into rust-lang:master May 5, 2023
17 of 20 checks passed
@weihanglo weihanglo deleted the ci-check-version-bump branch May 5, 2023 16:44
@weihanglo weihanglo mentioned this pull request May 5, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2023
@bors
Auto merge of rust-lang#111258 - weihanglo:update-cargo, r=weihanglo
f9a6b71 
Update cargo

10 commits in ac84010322a31f4a581dafe26258aa4ac8dea9cd..569b648b5831ae8a515e90c80843a5287c3304ef
2023-05-02 13:41:16 +0000 to 2023-05-05 15:49:44 +0000
- xtask-unpublished: output a markdown table (rust-lang/cargo#12085)
- fix: hack around `libsysroot` instead of `libtest` (rust-lang/cargo#12088)
- Optimize usage under rustup. (rust-lang/cargo#11917)
- Update lock to normalize `home` dep (rust-lang/cargo#12084)
- fix:  doc-test failures (rust-lang/cargo#12055)
- feat(cargo-metadata): add `workspace_default_members` (rust-lang/cargo#11978)
- doc: clarify implications of `cargo-yank` (rust-lang/cargo#11862)
- chore: Use `[workspace.dependencies]` (rust-lang/cargo#12057)
- support for shallow clones and fetches with `gitoxide` (rust-lang/cargo#11840)
- Build by PackageIdSpec, not name, to avoid ambiguity (rust-lang/cargo#12015)

r? `@ghost`
@ehuss ehuss added this to the 1.71.0 milestone May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@epage epage

Labels
A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-testing-cargo-itself Area: cargo's tests S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.71.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@weihanglo @rustbot @bors @epage @ehuss