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

Update cc crate for bootstrap to v1.0.97 #122504

Merged
merged 1 commit into from
May 7, 2024

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
Contributor

@jfgoog Please see my comments in #122498

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Contributor

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
Contributor

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

@rust-log-analyzer

This comment has been minimized.

@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
Contributor

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
Contributor

???

@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
Contributor

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
Contributor

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
Contributor

Ahhh.

@workingjubilee
Copy link
Contributor

workingjubilee commented Mar 14, 2024

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

@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

@jfgoog
Copy link
Contributor Author

jfgoog commented May 2, 2024

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

@jfgoog jfgoog force-pushed the update-cc-crate-version-2 branch from c618e3d to 0b5e082 Compare May 2, 2024 16:44
@onur-ozkan
Copy link
Member

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
Contributor

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 jfgoog force-pushed the update-cc-crate-version-2 branch from 0b5e082 to d767c96 Compare May 6, 2024 16:22
@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.

@jfgoog jfgoog changed the title Update cc crate for bootstrap to v1.0.90 Update cc crate for bootstrap to v1.0.97 May 6, 2024
@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.
@jfgoog jfgoog force-pushed the update-cc-crate-version-2 branch from d767c96 to 615b485 Compare May 6, 2024 21:32
@onur-ozkan
Copy link
Member

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
Contributor

bors commented May 7, 2024

📌 Commit 615b485 has been approved by onur-ozkan

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 7, 2024
@bors
Copy link
Contributor

bors commented May 7, 2024

⌛ Testing commit 615b485 with merge e2865db...

@bors
Copy link
Contributor

bors commented May 7, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 7, 2024
@bors bors merged commit e2865db into rust-lang:master May 7, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 7, 2024
@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.

None yet