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

Only use DIST_TRY_BUILD for try jobs that were not selected explicitly #138533

Merged
merged 2 commits into from
Mar 18, 2025

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Mar 15, 2025

Some CI jobs (x64 Linux, ARM64 Linux and x64 MSVC) use the opt-dist tool to build an optimized toolchain using PGO and BOLT. When performing a default try build for x64 Linux, in most cases we want to run perf. on that artifact. To reduce the latency of this common use-case, opt-dist skips building several components not needed for perf., and it also skips running post-optimization tests, when it detects that the job is executed as a try job (not a merge/auto job).

This is useful, but it also means that if you want to run the tests, you had to go to jobs.yml and manually comment this environment variable, create a WIP commit, do a try build, and then remove the WIP commit, which is annoying (in the similar way that modifying what gets run in try builds was annoying before we had the try-job annotations).

I thought that we could introduce some additional PR description marker like try-job-run-tests, but it's hard to discover that such things exist.

Instead, I think that there's a much simpler heuristic for determining whether DIST_TRY_BUILD should be used (that I implemented in this PR):

  • If you do just @bors try, without any custom try jobs selected, DIST_TRY_BUILD will be activated, to finish the build as fast as possible.
  • If you specify any custom try jobs, you are most likely doing experiments and you want to see if tests pass and everything builds as it should. The DIST_TRY_BUILD variable will thus not be set in this case.

In this way, if you want to run dist tests, you can just add the try-job: dist-x86_64-linux line to the PR description, and you don't need to create any WIP commits.

r? @marcoieni

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 15, 2025
@jieyouxu
Copy link
Member

Could you also leave a backlink somewhere, from maybe citool README or rustc-dev-guide's try job docs about this distinction?

@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

@rustbot rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Mar 16, 2025
@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 16, 2025

Good idea, done.

@marcoieni
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 17, 2025

📌 Commit b2fda93 has been approved by marcoieni

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 Mar 17, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#138384 (Move `hir::Item::ident` into `hir::ItemKind`.)
 - rust-lang#138508 (Clarify "owned data" in E0515.md)
 - rust-lang#138531 (Store test diffs in job summaries and improve analysis formatting)
 - rust-lang#138533 (Only use `DIST_TRY_BUILD` for try jobs that were not selected explicitly)
 - rust-lang#138556 (Fix ICE: attempted to remap an already remapped filename)
 - rust-lang#138608 (rustc_target: Add target feature constraints for LoongArch)
 - rust-lang#138619 (Flatten `if`s in `rustc_codegen_ssa`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#138384 (Move `hir::Item::ident` into `hir::ItemKind`.)
 - rust-lang#138508 (Clarify "owned data" in E0515.md)
 - rust-lang#138531 (Store test diffs in job summaries and improve analysis formatting)
 - rust-lang#138533 (Only use `DIST_TRY_BUILD` for try jobs that were not selected explicitly)
 - rust-lang#138556 (Fix ICE: attempted to remap an already remapped filename)
 - rust-lang#138608 (rustc_target: Add target feature constraints for LoongArch)
 - rust-lang#138619 (Flatten `if`s in `rustc_codegen_ssa`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c19ce9d into rust-lang:master Mar 18, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 18, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2025
Rollup merge of rust-lang#138533 - Kobzol:try-job-auto-tests, r=marcoieni

Only use `DIST_TRY_BUILD` for try jobs that were not selected explicitly

Some CI jobs (x64 Linux, ARM64 Linux and x64 MSVC) use the `opt-dist` tool to build an optimized toolchain using PGO and BOLT. When performing a default try build for x64 Linux, in most cases we want to run perf. on that artifact. To reduce the latency of this common use-case, `opt-dist` skips building several components not needed for perf., and it also skips running post-optimization tests, when it detects that the job is executed as a try job (not a merge/auto job).

This is useful, but it also means that if you *want* to run the tests, you had to go to `jobs.yml` and manually comment this environment variable, create a WIP commit, do a try build, and then remove the WIP commit, which is annoying (in the similar way that modifying what gets run in try builds was annoying before we had the `try-job` annotations).

I thought that we could introduce some additional PR description marker like `try-job-run-tests`, but it's hard to discover that such things exist.

Instead, I think that there's a much simpler heuristic for determining whether `DIST_TRY_BUILD` should be used (that I implemented in this PR):
- If you do just ``@bors` try`, without any custom try jobs selected, `DIST_TRY_BUILD` will be activated, to finish the build as fast as possible.
- If you specify any custom try jobs, you are most likely doing experiments and you want to see if tests pass and everything builds as it should. The `DIST_TRY_BUILD` variable will thus *not* be set in this case.

In this way, if you want to run dist tests, you can just add the `try-job: dist-x86_64-linux` line to the PR description, and you don't need to create any WIP commits.

r? `@marcoieni`
@Kobzol Kobzol deleted the try-job-auto-tests branch March 18, 2025 09:12
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 20, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#138384 (Move `hir::Item::ident` into `hir::ItemKind`.)
 - rust-lang#138508 (Clarify "owned data" in E0515.md)
 - rust-lang#138531 (Store test diffs in job summaries and improve analysis formatting)
 - rust-lang#138533 (Only use `DIST_TRY_BUILD` for try jobs that were not selected explicitly)
 - rust-lang#138556 (Fix ICE: attempted to remap an already remapped filename)
 - rust-lang#138608 (rustc_target: Add target feature constraints for LoongArch)
 - rust-lang#138619 (Flatten `if`s in `rustc_codegen_ssa`)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustc-dev-guide Area: rustc-dev-guide A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants