Skip to content

Update cc crate for bootstrap to v1.0.97#122504

Merged
bors merged 1 commit intorust-lang:masterfrom
jfgoog:update-cc-crate-version-2
May 7, 2024
Merged

Update cc crate for bootstrap to v1.0.97#122504
bors merged 1 commit intorust-lang:masterfrom
jfgoog:update-cc-crate-version-2

Conversation

@jfgoog
Copy link
Contributor

@jfgoog jfgoog commented Mar 14, 2024

Reason:

In order to build the Windows version of the Rust toolchain for the Android platform, the following patch to the cc is crate is required to avoid incorrectly determining that we are building with the Android NDK: rust-lang/cc-rs@57853c4

This patch is present in version 1.0.80 and newer versions of the cc crate. The rustc source distribution currently has 3 different versions of cc in the vendor directory, only one of which has the necessary fix.

We (the Android Rust toolchain) are currently maintaining local patches to upgrade the cc crate dependency versions, which we would like to upstream.

Furthermore, beyond the specific reason, the cc crate in bootstrap is currently pinned at an old version due to problems in the past when trying to update it. It is worthwhile to figure out and resolve these problems so we can keep the dependency up-to-date.

Other fixes:

As of cc v1.0.78, object files are prefixed with a 16-character hash.
Update src/bootstrap/src/core/build_steps/llvm.rs to account for this to
avoid failures when building libunwind and libcrt. Note that while the hash
prefix was introduced in v1.0.78, in order to determine the names of the
object files without scanning the directory, we rely on the compile_intermediates
method, which was introduced in cc v1.0.86

As of cc v1.0.86, compilation on MacOS uses the -mmacosx-version-min flag.
A long-standing bug in the CMake rules for compiler-rt causes compilation
to fail when this flag is specified. So we add a workaround to suppress this
flag.

Updating to cc v1.0.91 and newer requires fixes to bootstrap unit tests.
The unit tests use targets named "A", "B", etc., which fail a validation
check introduced in 1.0.91 of the cc crate.

As of cc v1.0.74, the SDKROOT environment variable is used on Darwin if present,
regardless of whether it is for the correct platform or not. This is fixed in cc v1.0.97.

@rustbot
Copy link
Collaborator

rustbot commented Mar 14, 2024

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
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 rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 14, 2024
@jfgoog
Copy link
Contributor Author

jfgoog commented Mar 14, 2024

As noted in the change description, I'm hoping to get all cc crate dependencies for rustc updated to at least 1.0.80, which has a fix we need for building the Rust toolchain for the Android platform.

I saw the comment in Cargo.toml, and I understand there is a good reason for pinning the version. I am happy to work on any fixes to the bootstrap code that are necessary to land this change.

I set the version to 1.0.90, which is the most recent, but I'd be happy with anything >=1.0.80, if that would be preferable.

@workingjubilee
Copy link
Member

@jfgoog Please see my comments in #122498

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

Bootstrap has its own lockfile, I see. I didn't notice that before. It will need the .lock updated too.

@jfgoog
Copy link
Contributor Author

jfgoog commented Mar 14, 2024

Here are all 3 PRs I've sent:

#122498
rust-lang/backtrace-rs#592
#122504

@workingjubilee
Copy link
Member

@jfgoog It should just be a matter of using cargo update -p cc in the bootstrap dir to bump the .lock forward.

@jfgoog jfgoog force-pushed the update-cc-crate-version-2 branch from 7094ed0 to 6979b48 Compare March 14, 2024 17:38
@rust-log-analyzer

This comment has been minimized.

@jfgoog jfgoog force-pushed the update-cc-crate-version-2 branch from 6979b48 to 51e31ff Compare March 14, 2024 17:42
@jfgoog
Copy link
Contributor Author

jfgoog commented Mar 14, 2024

@jfgoog It should just be a matter of using cargo update -p cc in the bootstrap dir to bump the .lock forward.

Done.

@workingjubilee
Copy link
Member

Cool.

@albertlarsan68 I will leave this to you but I have to wonder if there is any chance of bootstrap using the workspace cc version? Perhaps we should pursue that conversation separately.

@rust-log-analyzer

This comment has been minimized.

@jfgoog
Copy link
Contributor Author

jfgoog commented Mar 14, 2024

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

I'm not sure what to do about this failure.

@workingjubilee
Copy link
Member

???

@NobodyXu This is a peculiar error message, "Compiler version doesn't include clang or GCC"?

@jfgoog
Copy link
Contributor Author

jfgoog commented Mar 14, 2024

???

@NobodyXu This is a peculiar error message, "Compiler version doesn't include clang or GCC"?

I have a suspicion that things like this are the reason that the version is pinned.

@workingjubilee
Copy link
Member

Apparently!

@jfgoog
Copy link
Contributor Author

jfgoog commented Mar 14, 2024

More conservative version, updating to 1.0.80 instead of all the way to 1.0.90: #122507

Let's see if that fares better in presubmit checks.

@lukas-code
Copy link
Member

The Compiler version doesn't include clang or GCC is #122231 / rust-lang/cc-rs#958 and should be fine to ignore.

The actual error is x.py completions were changed; run `x.py run generate-completions` to update them which probably happens, because you also updated clap_complete with the Cargo.lock bump.

@klensy
Copy link
Contributor

klensy commented Mar 14, 2024

This pr accidentally bumped not only cc crate; plus see #119654 where cc bump was attempted and failed.

@workingjubilee
Copy link
Member

Ahhh.

@workingjubilee
Copy link
Member

workingjubilee commented Mar 14, 2024

@jfgoog Can you back out the other .lock changes and retry the cargo update cc?

@jfgoog jfgoog force-pushed the update-cc-crate-version-2 branch from 51e31ff to 0ded290 Compare March 14, 2024 19:51
@jfgoog
Copy link
Contributor Author

jfgoog commented Mar 14, 2024

@jfgoog Can you back out the other .lock changes and retry the cargo update cc?

Done

@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2024
@onur-ozkan onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2024
@jfgoog
Copy link
Contributor Author

jfgoog commented May 2, 2024

The latest error is not being able to find errno.h in "Building stage0 library artifacts (x86_64-apple-darwin)"

@onur-ozkan
Copy link
Contributor

I am unsure if that's the actual problem.

@jfgoog
Copy link
Contributor Author

jfgoog commented May 2, 2024

I suspect that I caused this by clearing SDKROOT. There are workarounds in multiple places to ignore SDKROOT if it's clearly set for the wrong platform:

  • compiler/rustc_target/src/spec/base/apple/mod.rs
  • compiler/rustc_codegen_ssa/src/back/link.rs

Maybe the cc crate needs to do the same thing?

@jfgoog
Copy link
Contributor Author

jfgoog commented May 2, 2024

I updated #124565 with an explanation of what I think is going on.

@onur-ozkan
Copy link
Contributor

Can we somehow set the correct platform instead of removing SDKROOT?

@jfgoog
Copy link
Contributor Author

jfgoog commented May 2, 2024

I've been trying to avoid it until now, but I feel like fixing the SDKROOT problem has to be done upstream in the cc crate. I don't see how we can detect every instance where we are cross-compiling, clear SDKROOT, and then presumably restore it afterwards.

@jfgoog
Copy link
Contributor Author

jfgoog commented May 2, 2024

Can we somehow set the correct platform instead of removing SDKROOT?

I don't think so, for two reasons:

  1. The SDKROOT has to be set to a valid MacOSX SDK in order for our build of clang to be able to find headers and libraries.
  2. When building for multiple platforms (e.g. iOS and watchOS) , which is common, there is no single correct value of SDKROOT that we can use.

@bors
Copy link
Collaborator

bors commented May 5, 2024

☔ The latest upstream changes (presumably #124726) made this pull request unmergeable. Please resolve the merge conflicts.

@jfgoog
Copy link
Contributor Author

jfgoog commented May 6, 2024

rust-lang/cc-rs#1047 is merged

@jfgoog
Copy link
Contributor Author

jfgoog commented May 6, 2024

Updated to the latest cc v1.0.97 to pick up the fix to SDKROOT.

@rust-log-analyzer

This comment has been minimized.

Reason:

In order to build the Windows version of the Rust toolchain for the Android platform, the following patch to the cc is crate is required to avoid incorrectly determining that we are building with the Android NDK: rust-lang/cc-rs@57853c4

This patch is present in version 1.0.80 and newer versions of the cc crate. The rustc source distribution currently has 3 different versions of cc in the vendor directory, only one of which has the necessary fix.

We (the Android Rust toolchain) are currently maintaining local patches to upgrade the cc crate dependency versions, which we would like to upstream.

Furthermore, beyond the specific reason, the cc crate in bootstrap is currently pinned at an old version due to problems in the past when trying to update it. It is worthwhile to figure out and resolve these problems so we can keep the dependency up-to-date.

Other fixes:

As of cc v1.0.78, object files are prefixed with a 16-character hash.
Update src/bootstrap/src/core/build_steps/llvm.rs to account for this to
avoid failures when building libunwind and libcrt. Note that while the hash
prefix was introduced in v1.0.78, in order to determine the names of the
object files without scanning the directory, we rely on the compile_intermediates
method, which was introduced in cc v1.0.86

As of cc v1.0.86, compilation on MacOS uses the -mmacosx-version-min flag.
A long-standing bug in the CMake rules for compiler-rt causes compilation
to fail when this flag is specified. So we add a workaround to suppress this
flag.

Updating to cc v1.0.91 and newer requires fixes to bootstrap unit tests.
The unit tests use targets named "A", "B", etc., which fail a validation
check introduced in 1.0.91 of the cc crate.
@onur-ozkan
Copy link
Contributor

rust-lang/cc-rs#1047 is merged

Updated to the latest cc v1.0.97 to pick up the fix to SDKROOT.

Thanks a lot for working on this!

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented May 7, 2024

📌 Commit 615b485 has been approved by onur-ozkan

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 7, 2024

⌛ Testing commit 615b485 with merge e2865db...

@bors
Copy link
Collaborator

bors commented May 7, 2024

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing e2865db to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e2865db): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.8% [2.1%, 3.8%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 676.36s -> 675.387s (-0.14%)
Artifact size: 315.83 MiB -> 315.97 MiB (0.04%)

@jfgoog
Copy link
Contributor Author

jfgoog commented May 7, 2024

Holy crap...it merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Development

Successfully merging this pull request may close these issues.