Skip to content

fix(dist/triple): ensure dist::triple::known is up to date with platforms #3841

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

Merged
merged 5 commits into from
Jun 8, 2024

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented May 24, 2024

Closes #3783.

By introducing platforms as a new dependency, this PR tries to address both the problems that come with generated source files within src, as well as the issue that the list of known target triples is a moving target:

  • Since build.rs already depends on full target triples to work, it no longer has to include!() some file inside src;

    Grepping the whole repo showed that RUSTUP_OVERRIDE_BUILD_TRIPLE has been used only once in the actual business logic, and exactly as a full triple, so this partial triple thing in build.rs is probably not very useful after all?

    impl TargetTriple {

    rustup/src/dist/dist.rs

    Lines 266 to 268 in 2a5a69e

    pub(crate) fn from_build() -> Self {
    if let Some(triple) = option_env!("RUSTUP_OVERRIDE_BUILD_TRIPLE") {
    Self::new(triple)

    rustup component (add|remove) should not rely on hardcoded target triples #3783 (comment)

  • The source file containing lists of known target triple segments, albeit generated, is committed directly into the codebase as a regular Rust module to enhance visibility;
  • The code generation coherence test is based on a specific version of platforms, so the output will not change as long as platforms' version is locked.

Concerns

  • Are custom triples involved in this change? No, they are handled by another mechanism (CustomToolchainName).
  • Will the original approach of building a regex of all list members be not scalable?
    let name = format!("-{name}");
    static RE: Lazy<Regex> = Lazy::new(|| {
    Regex::new(&format!(
    r"^(?:-({}))?(?:-({}))?(?:-({}))?$",
    LIST_ARCHS.join("|"),
    LIST_OSES.join("|"),
    LIST_ENVS.join("|")
    ))
    .unwrap()
    });

@rami3l rami3l force-pushed the platforms-triples branch from 980a552 to c99a962 Compare May 28, 2024 06:58
@rami3l rami3l changed the title fix(dist/triple): use segments generated from platforms at comptime fix(dist/triple): ensure dist::triple::known is up to date with platforms May 28, 2024
@rami3l rami3l marked this pull request as ready for review May 28, 2024 07:02
@rami3l rami3l force-pushed the platforms-triples branch from c99a962 to 79c6ed7 Compare May 28, 2024 07:23
@rami3l rami3l requested a review from djc May 28, 2024 07:23
@rami3l rami3l force-pushed the platforms-triples branch from 79c6ed7 to 87f3097 Compare May 28, 2024 13:46
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I have a bunch of small suggestions but this is looking pretty good!

(Sorry for taking a while to review it.)

@rami3l rami3l force-pushed the platforms-triples branch from 87f3097 to 80e147a Compare June 8, 2024 02:28
@rami3l rami3l force-pushed the platforms-triples branch from 80e147a to 4c3fe19 Compare June 8, 2024 03:02
@rami3l rami3l enabled auto-merge June 8, 2024 03:05
@rami3l rami3l requested a review from djc June 8, 2024 04:10
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@rami3l rami3l added this pull request to the merge queue Jun 8, 2024
Merged via the queue into master with commit c19aae9 Jun 8, 2024
26 checks passed
@rami3l rami3l deleted the platforms-triples branch June 8, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustup component (add|remove) should not rely on hardcoded target triples
2 participants