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

Introduce strongly-typed strings, starting with TargetTriple #1474

Merged
merged 5 commits into from
Oct 23, 2024

Conversation

fasterthanlime
Copy link
Contributor

As discussed, for the price of having to think about TargetTriple (like String) vs &TargetTripleRef (like &str), we get:

  • No accidentally passing some other kind of string to a thing expecting a TargetTriple
  • Serialization/deserialization is still transparent, no schema changes or anything
  • We can add methods to it (like is_windows() in this PR - note that I dream of a ParsedTargetTriple in a separate PR)
  • Those methods are the only place where we check properties of the string (before this commit, we have .contains("windows") and .contains("pc-windows") for example)
  • We can "find all references" to the type itself ("where do we care about targets?")
  • We can "find all references" to TargetTriple::new ("where do we build targets from strings?")
  • We can "find all references" to TargetTripleRef::as_str ("where do we coerce it back into a string to pass it to a tool like cargo/wix/etc.)

That kind of change is invaluable for me when working on cross-compilation support, and I suspect it will be invaluable for any current and future maintainers of cargo-dist as well (I've used it with great success in other large codebases).

You can still treat TargetTriple as a string, but it'll be uglier (on purpose).

There is however, some ugliness that isn't on purpose. In this changeset I discovered some annoyances around .iter() (which returns an Iterator<Item = &TargetTriple> instead of an Iterator<Item = &TargetTripleRef>. I've added .as_explicit_ref to work around those cases.

Similarly, calling Vec<TargetTriple>::contains() with a &TargetTripleRef doesn't work (and you cannot convert a &TargetTripleRef into a &TargetTriple, the same way you cannot convert a &str back into a &String - you don't know where it's allocated from!).

Finally, I ran into rust-lang/rfcs#1445 while making this change: there was a big match for converting target triples to their display names, and although that works with &str constants, it doesn't work with &TargetTripleRef constants, due to Rust limitations right now. That explains the lazy_static (which we already depended on transitively, so at least that). I would've used LazyLock but our MSRV is currently 1.79 and LazyLock is since 1.80 :(


(This is the standalone PR version of a commit in #1470 — to land this (relatively) big change separately. It makes thinking about cross-compilation much easier, of course.)

@fasterthanlime fasterthanlime requested review from mistydemeo and ashleygwilliams and removed request for mistydemeo October 22, 2024 16:43
Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

The changes you've made using this make a strong argument in favour of making this change!

Structurally, I think we need to think about what projects exactly need to use this. The reason all the platform information was in axoproject before is that Oranda also uses the constants from axoproject::platforms, so moving them into cargo-dist itself makes them inaccessible to it. So... if the macro has to exist in cargo-dist-schema, I guess we need to think about what's allowed to rely on what.

@@ -50,6 +50,7 @@ pub mod linkage;
pub mod manifest;
pub mod net;
pub mod platform;
pub mod platforms;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably rethink this name to avoid having both a platform and platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same but eventually decided to go for that instead of having a platform.rs that's twice as long (and has a "mostly constants" section and a "mostly logic" section).

We should feel free to restructure this as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, true, since it's totally internal to us it's easy enough to rename it as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just bringing up an off-issue discussion: we've decided to make this a targets mod inside platform.

Copy link
Contributor Author

@fasterthanlime fasterthanlime Oct 22, 2024

Choose a reason for hiding this comment

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

I think ashley said to then move platform.rs to platform/mod.rs (since we now have platform/targets.rs) but I think you said the way it was now was fine? So I'm not sure what to do and I'd vote we merge first and think up a policy later, probably.

For reference, both of these work:

crate/
  platform.rs
  platform/targets.rs
crate/
   platform/mod.rs
   platform/targets.rs

My preference is for the former because otherwise my code editor is full of mod.rs tabs and that's not super helpful. Some folks prefer the latter because, well, files are grouped into folders, so the file tree looks tidier I guess.

Copy link
Member

Choose a reason for hiding this comment

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

so the codebase uses the mod.rs pattern. i don't feel strongly beyond that we should try to be consistent. i am fine with this being the first pass and we can slowly just update the others because i agree 100 tabs of "mod.rs" is not great

cargo-dist/src/tasks.rs Outdated Show resolved Hide resolved
As discussed, for the price of having to think about `TargetTriple` (like `String`)
vs `&TargetTripleRef` (like `&str`), we get:

  * No accidentally passing some other kind of string to a thing expecting a `TargetTriple`
  * Serialization/deserialization is still transparent, no schema changes or anything
  * We can add methods to it (like `is_windows()` in this PR - note that I dream of a `ParsedTargetTriple` in a separate PR)
  * Those methods are the only place where we check properties of the string
    (before this commit, we have `.contains("windows")` and `.contains("pc-windows")` for example)
  * We can "find all references" to the type itself ("where do we care about targets?")
  * We can "find all references" to `TargetTriple::new` ("where do we build targets from strings?")
  * We can "find all references" to `TargetTripleRef::as_str` ("where do we coerce it back
    into a string to pass it to a tool like cargo/wix/etc.)

That kind of change is invaluable for me when working on cross-compilation
support, and I suspect it will be invaluable for any current and future
maintainers of cargo-dist as well (I've used it with great success in other
large codebases).

You can still treat `TargetTriple` as a string, but it'll be uglier (on purpose).

There is however, some ugliness that isn't on purpose. In this changeset I
discovered some annoyances around `.iter()` (which returns an `Iterator<Item = &TargetTriple>`
instead of an `Iterator<Item = &TargetTripleRef>`. I've added `.as_explicit_ref` to work
around those cases.

Similarly, calling `Vec<TargetTriple>::contains()` with a `&TargetTripleRef` doesn't
work (and you cannot convert a `&TargetTripleRef` into a `&TargetTriple`, the same way
you cannot convert a `&str` back into a `&String` - you don't know where it's allocated
from!).

Finally, I ran into <rust-lang/rfcs#1445> while making this
change: there was a big `match` for converting target triples to their display names,
and although that works with `&str` constants, it doesn't work with `&TargetTripleRef`
constants, due to Rust limitations right now. That explains the lazy_static (which
we already depended on transitively, so at least that). I would've used `LazyLock`
but our MSRV is currently 1.79 and LazyLock is since 1.80 :(
Copy link
Contributor

@mistydemeo mistydemeo left a comment

Choose a reason for hiding this comment

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

Looks good, and thanks for the detailed docs!

cargo-dist-schema/src/macros.rs Outdated Show resolved Hide resolved
@fasterthanlime fasterthanlime merged commit bba11f3 into main Oct 23, 2024
17 checks passed
@fasterthanlime fasterthanlime deleted the strongly-typed-target-triple branch October 23, 2024 11:46
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.

3 participants