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

Change Platform struct to Target #127

Closed
volks73 opened this issue Nov 10, 2020 · 4 comments
Closed

Change Platform struct to Target #127

volks73 opened this issue Nov 10, 2020 · 4 comments
Assignees

Comments

@volks73
Copy link
Owner

volks73 commented Nov 10, 2020

I am wondering if the Platform struct should be renamed and its implementation changed. It is currently just a "container" for a string that is the Rust compiler (rustc) target triple with some helper methods for extracting the "target/architecture/platform" in various string formats. This was type was originally created for the Platform attribute of the Product tag in the WiX Source (WXS) XML schema but it is deprecated (#121). The Platform WiX variable is no longer needed and terminology is based on "arch" and "target" rather than "platform".

An idea would be to rename the struct to "Target" or "RustcTriple" and have field members for the various components of the target triple. Then, implement a couple of TryFrom or FromStr traits that parse a string into the individual components. Methods would be added to get each component. These methods would make creating WiX variables for each component very easy, too. What are the various components of the target triple?

@roblabla
Copy link
Contributor

roblabla commented Nov 10, 2020

An idea would be to rename the struct to "Target" or "RustcTriple" and have field members for the various components of the target triple.

So the thing is, we currently use it to access two separate pieces of information:

  1. The WiX architecture (x86, x64, arm, arm64)
  2. The target triple name to pass to cargo build --target.

A triple composition is usually <arch-ish>-<vendor>-<os>-<env>:

  • Arch-ish is the architecture to use, but can also select a particular implementation of the architecture, for instance i686/i586 for x86, thumbv6m for ARM.
  • Vendor is rarely used nowadays. It is pc for windows targets, and unknown for almost everybody else.
  • OS allows you to target family of OS, for instance windows, linux, freebsd, darwin (for macos).
  • Env is a bit of a mash of calling convention, libc, and other platform-specific differences being used. For instance, on windows we have msvc vs gnu (for mingw), on linux we have gnu vs musl.

Note however, that all components are technically optional, and each component is rarely useful on its own (with the exception of arch, which is already available in wix through BUILDARCH, and os, which isn't really relevant to WiX, being windows-only).

More importantly, we currently only have access to the target triple name. I believe it'd be a mistake to try and parse it any further than we already do, because it's really not meant to be parsed. The proper way to get access to the individual component would be through the nightly-only rustc --print-target-json.

EDIT: See new comment in #126, I'm wrong, there are other ways. So getting the individual component is actually entirely possible.

@volks73
Copy link
Owner Author

volks73 commented Nov 10, 2020

I have read the comment in #126. So, should the Platform type still be changed into a type that focuses on parsing and providing the information from the rustc --print cfg output? Maybe it should be called TargetCfg or TargetConfig or TargetConfiguration.

I agree that most of the component are not going to be useful or needed, especially on Windows.

@volks73
Copy link
Owner Author

volks73 commented Nov 12, 2020

@roblabla: Based on the discussion in #115, it looks like the Platform struct should just be removed completely but a type specific to parsing the output from the rustc --print cfg command should be added. I was thinking of creating a rustc.rs module and adding a Configuration struct type in the module that would focus on parsing the output. Then, I thought this was very similar to the cargo-metadata crate, so maybe this functionality would be better served as a separate rustc-cfg crate outside of the cargo-wix crate. I did a quick search on crates.io and found there is already a rustc-cfg crate that does exactly this. It looks a little stale (last update in 2018, the docs.rs page for the latest version is not working for me), but the functionality is there. Thus, to avoid duplicate work, let's refactor the Platform struct and triple parsing to use the rustc-cfg crate.

The wix_arch method would be changed to a function or its own Arch struct type that implements a TryFrom trait for the Cfg struct from the rustc-cfg crate. At least the function implementation would become less messy and easier.

If the -t,--target option is used during cargo wix installer creation but with a --no-build, would the rustc --print cfg still be reliable for obtaining the information? In other words, do we still need to implement parsing a target triple string when a target triple is explicitly stated? If yes, then we probably need to create a type that "wraps"/contains the Cfg struct from the rustc-cfg crate that includes target triple parsing. Wait...nevermind. There is the Cfg::of method that runs the rustc --print cfg <target> command and returns the parsed output. It appears rustc --print cfg handles parsing a target triple for us and the rust-cfg crate provides this functionality, too.

One small correction to the rustc-cfg crate documentation, the command is rustc --target <target> --print cfg not rustc --print cfg <target>.

volks73 added a commit that referenced this issue Nov 21, 2020
This replaces/removes the `Platform` struct. The `WixArch` struct uses
the [rustc-cfg](https://crates.io/crates/rustc-cfg) crate to parse the
output from the `rustc --target <target> --print cfg` command and
determine the WiX Toolset architecture for the `-arch` option of the WiX
Toolset compiler (candle.exe). See Issue #127 for more information.
@volks73
Copy link
Owner Author

volks73 commented Nov 21, 2020

The Platform struct has been removed and the WixArch enum type has been added as a replacement as of 0da95c9. The rustc-cfg crate is used with the WixArch type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants