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

Switch away from enums towards strings in TargetInfo? #8

Closed
sunshowers opened this issue Apr 20, 2020 · 10 comments · Fixed by #9
Closed

Switch away from enums towards strings in TargetInfo? #8

sunshowers opened this issue Apr 20, 2020 · 10 comments · Fixed by #9
Assignees
Labels
enhancement New feature or request

Comments

@sunshowers
Copy link
Contributor

sunshowers commented Apr 20, 2020

Is your feature request related to a problem? Please describe.
Target enums (e.g. https://docs.rs/cfg-expr/0.3.0/cfg_expr/targets/enum.Os.html) are currently modeled as a closed set. However, future versions of rustc may add new values to each of those. In addition, we have some use cases for custom platforms that aren't currently defined by rustc.

Describe the solution you'd like

  1. TargetInfo gains a lifetime parameter 'a.
  2. All the fields in https://docs.rs/cfg-expr/0.3.0/cfg_expr/targets/struct.TargetInfo.html (except pointer width maybe?) are &'a str instances, not enums.
  3. Add constants for known platforms to prevent typos, e.g.
pub struct Os;

impl Os {
    pub static HAIKU: &str = "haiku";
    ...
}

Describe alternatives you've considered
One option might be to add an Other variant:

pub enum Os {
   haiku,
   ...
   Other(&'static str),
}

but that has consistency issues since e.g. Os::haiku isn't the same as Os::Other("haiku").

I'd be happy to do the work if it sounds good to you!

Additional context
Thanks again for cfg-expr! We have several use cases for it, are already using it in target-spec, and would love to use it in tooling we're going to open source soon :)

@sunshowers sunshowers added the enhancement New feature or request label Apr 20, 2020
@Jake-Shadle
Copy link
Member

I would prefer to not use strings and constants, the available targets for rustc just do not change that often to justify it imo. That being said, having a fallback of other/unknown would be fine, since it could still be compared successfully as in your example by customizing the partialeq impls. Then there would be some work to infer the target pieces from a string, though that might be a little annoying as not all target specs have the same number of pieces, eg Darwin.

@sunshowers
Copy link
Contributor Author

Thanks for your response!

So one use case that's popped up is being able to read a custom target specification and parse it into a platform: https://doc.rust-lang.org/rustc/targets/custom.html. These specifications may contain arbitrary data.

Personally I'm not really a fan of the Other approach -- while customizing the PartialEq impl is a possibility, my understanding is that it would still break pattern matching. Meanwhile, just using strings would make this much simpler.

What do you think?

@Jake-Shadle
Copy link
Member

Custom targets were something I thought about briefly when I did the initial implementation of this, but I didn't have a concrete example so punted that until I did.

If you could give such an example that would be great!

The enum approach works well for the regular targets built in to rustc which is what most users will care about, I would hate to lose them just because of more niche custom targets that only a few would.

@roblabla
Copy link

roblabla commented May 28, 2020

So my use-case is a custom OS I'm working on. I wanted to use cargo-guppy to find out why a feature gets pulled in one of my libstd dependencies. cargo-guppy uses this crate to parse cfg() expressions found in Cargo.toml.

In my libstd fork, I use cfg(target_os = "sunrise") to add a custom that is only used when building for my custom target. cfg-expr chokes on it with the following error:

Error while computing package graph: for package 'std 0.0.0 (path+file:///home/roblabla/Dropbox/dev/src/rust/kfs-rust/src/libstd)': for dependency 'lazy_static', parsing target 'cfg(target_os = "sunrise")' failed: invalid cfg() expression: target_os = "sunrise"
             ^^^^^^^ expected one of `haiku`, `openbsd`, `freebsd`, `redox`, `vxworks`, `uefi`, `emscripten`, `netbsd`, `fuchsia`, `cloudabi`, `wasi`, `solaris`, `cuda`, `dragonfly`, `l4re`, `android`, `macos`, `hermit`, `linux`, `windows`, `unknown`, `ios` here
Aborting...

A lot of people working in embedded environment have to use custom targets.

@repi
Copy link

repi commented May 28, 2020

That's interesting, and does make sense to have for custom targets. We may even have some for ourselves for game consoles later (EmbarkStudios/rust-ecosystem#18).

To me the Other(str) variant does sound like it could be a good fit for that

@sunshowers
Copy link
Contributor Author

sunshowers commented May 28, 2020

I think that still has the problem that there are multiple ways to represent an enum and defining equality becomes very problematic.

This is unfortunately becoming a really big issue. My suggestion still remains to model all of these parameters as strings and provide static strings for all the known targets. I believe it gets the best of both worlds.

@roblabla
Copy link

roblabla commented May 28, 2020

Using associated constants allows modeling an API that looks a lot like using an enum.

pub struct Vendor<'a>(&'a str);
impl Vendor {
    pub const pc: Vendor<'static> = Vendor("pc");
    pub const unknown: Vendor<'static> = Vendor("unknown");
    pub const uwp: Vendor<'static> = Vendor("uwp");
    pub const nvidia: Vendor<'static> = Vendor("nvidia");
    pub const sun: Vendor<'static> = Vendor("sun");
    pub const fortanix: Vendor<'static> = Vendor("fortanix");
    pub const wrs: Vendor<'static> = Vendor("wrs");
    pub const rumprun: Vendor<'static> = Vendor("rumprun");
    pub const apple: Vendor<'static> = Vendor("apple");
}

This allows using it almost exactly like an enum: it can be used when pattern matching, creating an instance of the Vendor type, etc...

match vendor {
    Vendor::wrs => (),
    Vendor::apple => (),
    Vendor(other) => ()
}

@Jake-Shadle
Copy link
Member

That could work nicely and easy to do since we auto-generate the builtin targets already.

@repi
Copy link

repi commented May 29, 2020

Yeah think that would work. Although it does look odd to have a static variable with lower case, and would generate a naming warning. So using standard SCREAMING_SNAKE_CASE would be cleaner.

It does solve the equality issue that the Other(str) approach would have, which is good.

@Jake-Shadle Jake-Shadle self-assigned this May 29, 2020
@sunshowers
Copy link
Contributor Author

Thank you so much! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants