Bump boostrap `cc` to 1.2.17 and `cmake` to 0.1.54 #138784

madsmtm · 2025-03-21T12:36:42Z

The cc version in bootstrap was reverted down to 1.1.22 in #137460 (previously at 1.2.0). The offending issue has since then been resolved in rust-lang/cc-rs#1413, and a new version of cc has been released in rust-lang/cc-rs#1435, so let's try to update the version again.

See the cc-rs changelog and the cmake-rs changelog for details on what has changed here.

r? jieyouxu who tried this last in #137022.
@rustbot label T-bootstrap
try-job: apple

Kobzol · 2025-03-21T16:13:23Z

Please add try-job: dist-apple-various to the PR description and run a try build (@bors try). The dist jobs might not run on PR CI.

madsmtm · 2025-03-21T17:14:38Z

Ah, that's why, I really couldn't understand why it didn't work (and failing in sccache looked very much like a spurious network error or something).

Have added to try-job to the description, though pretty sure I don't have permissions to do @bors try. Let's try:
@bors try

Kobzol · 2025-03-21T17:36:06Z

@bors try

…try> Bump boostrap `cc` to 1.2.17 and `cmake` to 0.1.54 The `cc` version in `bootstrap` was reverted down to 1.1.22 in rust-lang#137460 (previously at 1.2.0). The offending issue has since then been resolved in rust-lang/cc-rs#1413, and a new version of `cc` has been released in rust-lang/cc-rs#1435, so let's try to update the version again. See [the changelog](https://github.com/rust-lang/cc-rs/blob/d9dd20e376368c7535f6ef89b809098f5f203c1a/CHANGELOG.md) for exact details on what has changed here. r? jieyouxu who tried this last in rust-lang#137022. `@rustbot` label T-bootstrap try-job: dist-apple-various

bors · 2025-03-21T17:37:17Z

⌛ Trying commit 673b519 with merge dbfe100...

bors · 2025-03-21T20:45:47Z

💔 Test failed - checks-actions

jieyouxu · 2025-03-22T03:50:24Z

@bors delegate+ (for try-jobs only, I haven't looked at the cc diff)

bors · 2025-03-22T03:50:27Z

✌️ @madsmtm, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

madsmtm · 2025-03-22T04:47:53Z

Okay so the failure comes from all the way back in rust-lang/cmake-rs#158, see also rust-lang/cmake-rs#228. I've added a workaround in bootstrap, though ideally this should be fixed in cmake-rs proper (but it requires target parsing to do correctly... So I'm hesitant to attempt it yet).

@bors try

bors · 2025-03-22T04:49:04Z

⌛ Testing commit 061eda1 with merge 77ad7c5...

Bump boostrap `cc` to 1.2.17 and `cmake` to 0.1.54 The `cc` version in `bootstrap` was reverted down to 1.1.22 in rust-lang#137460 (previously at 1.2.0). The offending issue has since then been resolved in rust-lang/cc-rs#1413, and a new version of `cc` has been released in rust-lang/cc-rs#1435, so let's try to update the version again. See [the changelog](https://github.com/rust-lang/cc-rs/blob/d9dd20e376368c7535f6ef89b809098f5f203c1a/CHANGELOG.md) for exact details on what has changed here. r? jieyouxu who tried this last in rust-lang#137022. `@rustbot` label T-bootstrap try-job: dist-apple-various

madsmtm · 2025-03-25T14:29:57Z

Hmm, seems like it failed while testing bootstrap itself after I changed download-ci-llvm-stamp? I don't know why that's the case?

Failure:

thread 'core::config::tests::download_ci_llvm' panicked at src/bootstrap/src/core/config/tests.rs:30:9:
assertion failed: config.llvm_from_ci

jieyouxu · 2025-03-25T14:38:28Z

This one I'm not sure about... @Kobzol this can't be from #138704 can it?

Kobzol · 2025-03-25T15:43:14Z

Hmm. Looking at the test, I think that it doesn't really make sense anymore after #138704.

klensy · 2025-03-25T17:07:48Z

Submitted a PR to cmake-rs for the reliance on CARGO_CFG_*, see rust-lang/cmake-rs#242.

Should wait few days for pr, maybe it will be merged?

madsmtm · 2025-03-25T19:18:49Z

Submitted a PR to cmake-rs for the reliance on CARGO_CFG_*, see rust-lang/cmake-rs#242.

Should wait few days for pr, maybe it will be merged?

I'd rather file a follow-up later, it can take time for a new cmake-rs release.

Hmm. Looking at the test, I think that it doesn't really make sense anymore after #138704.

Do you think I should remove it, then?

Kobzol · 2025-03-25T21:32:53Z

Do you think I should remove it, then?

I will do it in a separate PR.

Kobzol · 2025-03-27T10:12:48Z

Sent #139015.

jieyouxu · 2025-03-27T10:34:56Z

I suppose this is temporarily blocked on fixing the test first.
@rustbot blocked (#139015)

…ozkan Remove unneeded LLVM CI test assertions The `download_ci_llvm` bootstrap test was checking implementation details of the LLVM CI download check, which isn't very useful. It was essentially testing "if function_that_checks_if_llvm_ci_is_available returns true, we enable CI LLVM", but the usage of the function was an implementation detail. After rust-lang#138704, the inner implementation has changed, so the test now breaks if LLVM is updated. I don't think that it's very useful to test implementation details like this, without taking the outside git state into account. Ideally, we should mock the git state for the test, otherwise the test will randomly break when executed in environments which the test does not control (e.g. on CI when a LLVM change happens). I only kept the part of the test that checks that LLVM CI isn't used when we specify `download-ci-llvm = false`, as that should hold under all conditions, CI/local, and all git states. I also kept the `if-unchanged` assertion, but only on CI, and as a temporary measure. After rust-lang#138591, we should have a proper way of mocking the git state to make the test robust, and make it test what we actually want. Fixes [this](rust-lang#138784 (comment)). r? `@ghost`

Rollup merge of rust-lang#139015 - Kobzol:llvm-ci-test-fixes, r=onur-ozkan Remove unneeded LLVM CI test assertions The `download_ci_llvm` bootstrap test was checking implementation details of the LLVM CI download check, which isn't very useful. It was essentially testing "if function_that_checks_if_llvm_ci_is_available returns true, we enable CI LLVM", but the usage of the function was an implementation detail. After rust-lang#138704, the inner implementation has changed, so the test now breaks if LLVM is updated. I don't think that it's very useful to test implementation details like this, without taking the outside git state into account. Ideally, we should mock the git state for the test, otherwise the test will randomly break when executed in environments which the test does not control (e.g. on CI when a LLVM change happens). I only kept the part of the test that checks that LLVM CI isn't used when we specify `download-ci-llvm = false`, as that should hold under all conditions, CI/local, and all git states. I also kept the `if-unchanged` assertion, but only on CI, and as a temporary measure. After rust-lang#138591, we should have a proper way of mocking the git state to make the test robust, and make it test what we actually want. Fixes [this](rust-lang#138784 (comment)). r? `@ghost`

madsmtm · 2025-03-28T00:04:26Z

Uh, I think this got closed by the "fixes" sentence in that PR, and I can't re-open it myself. Can someone else do that, or should I make a new PR?

EDIT:
@rustbot ready

To avoid a panic in cmake-rs that was introduced in: rust-lang/cmake-rs#158

compiler-rt's CMake setup seems to have special logic for Apple platforms that works poorly when this is not set.

Similarly to what was previously done for the `llvm` step.

This PR makes a fairly large version update to CMake and cc, so it is likely that LLVM is built differently.

jieyouxu

Thanks. It is indeed very nice if we no longer have to pin bootstrap cc/cmake to old versions that are quite a ways away from the compiler cc versions.

I'm semi-expecting this to maybe cause issues in obscure cases, so I'm going to mark this as rollup-never for bisection purposes, but we should definitely try this.

jieyouxu · 2025-03-28T12:48:19Z

@bors r+ rollup=never

bors · 2025-03-28T12:48:21Z

📌 Commit 7a6a324 has been approved by jieyouxu

It is now in the queue for this repository.

rustbot assigned jieyouxu Mar 21, 2025