Skip to content

Build LLVM using CI image's Clang on Win + MacOS, instead of installing a copy #742

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

Closed
1 of 3 tasks
dpaoliello opened this issue Apr 22, 2024 · 2 comments
Closed
1 of 3 tasks
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@dpaoliello
Copy link

Proposal

As part of CI, we install LLVM 14.0.5 on Windows and MacOS builds in src/ci/scripts/install-clang.sh. This LLVM install is used to build LLVM as a library for use in the compiler and other C/C++ files for both the compiler and standard library. LLVM 14.0.5 is both an outdated version of LLVM, and not even the latest 14.x point-release.

Instead, we can use the version of LLVM that is pre-installed on the CI Images.

  • For Windows, this is 16.0.6
  • For MacOS, this is 15.0.7 (although we'll still have to set CC and CXX as the LLVM on the PATH is 14.0.0).

Potential Issues

This would take control of the Clang version used to build LLVM out of our hands, and we'd be at the mercy of whatever version GitHub decides to include. If we do run into issues, then we'd either need to request that GitHub update or rollback the version being used, or we'd have to re-introduce the logic to install a specific version.

It would also mean that Windows and MacOS will be using different versions of Clang. While this may seem problematic, Linux is already using a different version, so we are already having to deal with differing Clang versions per OS.

For Windows, there is a note that GitHub image 20210928.2 added LLVM, but it is broken - this is referring to lldb requiring that python310.dll be installed on the system, which it isn't for the GitHub builders. Since we don't use lldb on Windows anyway, we can change the workaround to deleting lldb.exe instead of the entire LLVM directory.

For MacOS, we still need to set the CC and CXX environment variables as the LLVM on the PATH is 14.0.0. Since the command to find the path to LLVM is major-version specific, we'll need to update this logic each time that we want to move MacOS to a new major version of LLVM.

Alternatives

Keep using Clang 14.0.5

This would block updating the Windows runners to Visual Studio 2022, as the STL included in VS2022 requires (and checks for) Clang 16+.

Additionally, since this copy of Clang is used to build both LLVM for use in the compiler and other C++ files for use in the standard library (e.g., compiler-builtins), we are denying codegen improvements (either for correctness or performance) to both devs and end-users on Windows and MacOS.

Update the version of Clang that gets installed

The LLVM installer on the Rust mirror appears to be a copy of the installer directly from LLVM's website/GitHub. We could update this installer with a newer version and use that instead.

While this is tempting (for giving us control of the version of LLVM to use), we would need to establish a cadence and process for updating the version used - adding overhead and work to the Rust project for something that GitHub is already providing. If we don't establish a regular update cadence, we are likely to end up in this same situation in a few years' time (even the version we are installing on Linux is now out-of-date).

Mentors or Reviewers

@dpaoliello

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@dpaoliello dpaoliello added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Apr 22, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 22, 2024

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

@rustbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rustbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Apr 22, 2024
@nikic
Copy link
Contributor

nikic commented Apr 24, 2024

@rustbot concern random-breakage-from-github-changes I think we have pretty extensive experience from other tooling that this is a bad idea, because changes to GitHub images will randomly break our CI -- in ways that we may not be able to quickly mitigate.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants