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

Target Bundles #1711

Closed
wants to merge 4 commits into from
Closed

Target Bundles #1711

wants to merge 4 commits into from

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Aug 9, 2016

"target_env": "gnu",
"target_vendor": "unknown",
"target_has_atomic": ["8", "16", "32", "64"], // any of #[cfg(target_has_atomic={"8", "16", "32", "64")] work.
"target_has_atomic_ptr": null,
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the previous system where you only need to specify a single value for atomic support: just the maximum atomic bit width supported by the target. It's much simpler and gives the same information.

@codyps
Copy link

codyps commented Aug 9, 2016

If we're going to change the target format anyway (by moving around json keys as proposed here), I'm not sure what the downside of switching to toml would be (and at that point we'll get comments natively, and have the same config format as cargo)

values, though.

# Alternatives
[alternatives]: #alternatives
Copy link

Choose a reason for hiding this comment

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

Seems like an alternative (in some sense) would be to have a rustc --print target-spec option that spit out more info.

Copy link
Member Author

@nagisa nagisa Aug 9, 2016

Choose a reason for hiding this comment

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

Its not an alternative at all, because that option does not help creating and distributing custom targets.

Copy link

@codyps codyps Aug 9, 2016

Choose a reason for hiding this comment

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

It's a solution to the issue of being able to determine information about a target externally (which I saw as one of the motivations for moving all targets to json), but you're right that it doesn't have anything to do with creating & distributing custom targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've got the code for rustc --print target-spec at https://github.com/cardoe/rust/tree/target-spec I'll gladly resubmit it.

@nagisa
Copy link
Member Author

nagisa commented Aug 9, 2016

@Amanieu commented on the line 128 (about target_has_atomic):

I would prefer the previous system where you only need to specify a single value for atomic support: just the maximum atomic bit width supported by the target. It's much simpler and gives the same information.

GitHub really needs a way to mark comments as not-outdated.

@Ericson2314
Copy link
Contributor

Speaking of cross compiling, I think rustc should support separate build and host platforms (to use the autoconf terminology---which we may want to abandon as there's no need to ever bake in a target so 2 platforms suffices). This would certainly make build.rs easier to work with.

Also, while I'd like to do this ASAP, I don't think it should be stabilized ASAP as there is some interaction with scenarios.

@Ericson2314
Copy link
Contributor

Also, Now more than ever, it should be clear that that the target triple does not determine the target. So that we still by default name build directories by the target triple is prone for errors. I think there should at least be an option (if not the default) to instead normalize the config JSON, hash it, and use the hash instead.

@codyps
Copy link

codyps commented Aug 9, 2016

Now more than ever, it should be clear that that the target triple does not determine the target.

+1 to that (now we just need to get rustbuild to stop trying to parse them...)

I think there should at least be an option (if not the default) to instead normalize the config JSON, hash it, and use the hash instead.

As long as this can be overridden in cargo via a flag or env var (so higher level build systems can know where files end up without hashing things themselves) that would work for me.

@Ericson2314
Copy link
Contributor

As long as this can be overridden in cargo via a flag or env var (so higher level build systems can know where files end up without hashing things themselves) that would work for me.

Quickly glancing over --help and the man page, I didn't see anything, but I swear this already existed.

@codyps
Copy link

codyps commented Aug 9, 2016

I swear this already existed.

There is a env var to change the target directory (CARGO_TARGET_DIR, it's addition broke some of my makefiles that used a variable of the same name to track the deeper directory), but not (afaik) to change the target/<target-name> directory.

@eternaleye
Copy link

I'd also like to chime in supporting the idea of using TOML rather than JSON, if we make incompatible changes. IMO, it'd make it considerably easier to work with targets - in particular, the built-in targets, well-commented, could themselves serve as half-decent documentation.

```js
{
// REQUIRED
"llvm-target": "x86_64-unknown-linux-gnu", // LLVM target triple (does not need to match with rustc triple)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use dashes and instead use underscores so that you can just use #[derive(RustcEncodable, RustcDecodable)]. The dashes are the problem as I pointed out in rust-lang/rust#32988

@Ericson2314
Copy link
Contributor

Actually, there might be a use for all 3 autoconfig-style platforms after all. "host" for the project becomes "target" for build scripts and procedural macros.

@eternaleye
Copy link

eternaleye commented Aug 9, 2016

@Ericson2314: Moreover, build scripts and procedural macros probably should be aware of target (consider build scripts that compile C, or procedural macros that need to care about mem::size_of<usize>())

@japaric
Copy link
Member

japaric commented Aug 9, 2016

I'd also like to chime in supporting the idea of using TOML rather than JSON, if we make incompatible changes.

It might even be possible to provide a migration path for this: Support both $TARGET.json and $TARGET.toml where the TOML spec has precedence over the JSON spec. Warn when a JSON spec is used and point to the new TOML spec. After a few cycles, ignore the JSON spec. Though I'm not sure if it's worth the trouble.

@codyps
Copy link

codyps commented Aug 9, 2016

Though I'm not sure if it's worth the trouble.

Target specs have gotten members added (some mandatory) and removed as a regular occurrence, without warnings. I don't think there is really stability with the specs yet. I don't see it as a big deal to just remove the json spec support (presuming we switch toml). That said, I say this as someone using target specs with release versions, not with nightlies, so adjusting to changes here isn't so bad for me.

@alexcrichton alexcrichton added the T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. label Aug 9, 2016
than the built-in targets rustc knows about, therefore distributing targets built-in into rustc is
providing no benefits.

Native libraries and linkers aside, it is obvious there’s little sense in distributing targets
Copy link
Member

Choose a reason for hiding this comment

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

I don't agree with this. Once "on the fly compilation of std" becomes a thing, it actually makes sense to provide all the target specifications "upfront" with rustc. Then people, specially the ones working with/on embedded systems, can choose which std features to enable/disable (e.g. remove RUST_BACKTRACE support) (*) and/or compile std with CPU specific optimizations for maximum performance (**). In this scenario, rust-std becomes a convenience to avoid the compile time required to build std the first time.

(*) This actually makes more sense for final deployments/releases.
(**) This makes sense for both development (slightly faster execution of test suites but the savings add up quickly) and for deployment to embedded systems. For releases that must work everywhere, one should instead not enable any CPU-specific optimization -- this would be the default, zero-configuration option, which is also how rust-std is compiled.

Instead of what's proposed here, I propose adding a new targets component that will be installed by rustup once regardless of which or how many toolchains (rustc components) one installs. After all, target specifications are indepedent of the host system. This also means one less step (rustup target add foo) in the initial cross compilation setup (once "on the fly compilation of std" is here).

Copy link

Choose a reason for hiding this comment

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

Getting rid of precompiled std altogether isn't something I had considered in my other comments here. If we're sure that is going to happen (and as a result ${libdir}/rustlib/<target>/* is going to go away as most places that isn't writable) then it definitely makes sense to distribute specs for all supported targets at once rather than requiring them to each be downloaded separately because the size of a target will just be the size of it's spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

After all, target specifications are indepedent of the host system.

They aren't independent of the compiler however, as the fields will probably be subject to change for a while.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Then we could use the same "channel" policy that the toolchain uses. If you do rustup update nightly, you get an updated targets-nightly; If you do rustup default stable, you get targets-stable; and so for.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not disagree with any points raised here, but I feel like solving this concern should become a part of eventual “custom std on demand” RFC, which AFAICT not even feasible at the moment due to stability concerns.

@strega-nil
Copy link

ping @nagisa

status?

"target_pointer_width": "64",
"target_env": "gnu",
"target_vendor": "unknown",
"target_has_atomic": ["8", "16", "32", "64"], // any of #[cfg(target_has_atomic={"8","16","32","64"}] work.
Copy link
Member Author

Choose a reason for hiding this comment

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

@Amanieu previously complained about this being an explicit listing. See rust-lang/rust#38579

@nagisa
Copy link
Member Author

nagisa commented Dec 23, 2016

@ubsan I’m considerably comfortable with current PR as it is written. I still think it makes a lot of sense to bundle libraries with their target specs.

There has been some concerns raised about future std-on-demand RFC, but it seems to me like concern is better handled in the future RFC. I do not see anything in this RFC that would prohibit on-demand compilation of std or even core in the future.

All in all, this RFC is mostly waiting for somebody to shepherd this and move the RFC into FPC for either acception or rejection.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Dec 29, 2016

As the author of std-aware cargo, I see no conflict. I hope this can be shepherded soon!

@alexcrichton
Copy link
Member

Overall this RFC seems great to me, it looks like a fantastic cleanup of the current json target specs and should also simplify some compiler internals by making them a little more agnostic to platforms.

It's also worth pointing out that since this RFC was opened the internal targets are all "JSON-ified" in the sense that the compiler only consumes them through the JSON format. They're not literally stored in JSON yet (to preserve comments), but we should be able to easily convert them over.

One thing I've noticed is that over time we continually add is_like_foo keys to the target JSON, typically because one platform just sticks out from the rest. @nagisa do you feel that we'd avoid this and instead add a "proper" configuration targeted at the exact use case of where you'd otherwise write if is_like_foo? That means that we'll probably want some story for the default for each key as it may be common to hit specs without keys specified as the compiler evolves.

The final point of this RFC, distributing target specs with libraries, I may still need convincing for. The RFC claims:

Currently every built-in target is distributed along with the rustc compiler. This is suboptimal, because in majority of cases users are interested in targets which rustc can compile for, rather than the built-in targets rustc knows about, therefore distributing targets built-in into rustc is providing no benefits.

I'd personally argue that there's no downside to the current scheme, and the drawback of a different scheme is that it will take time and effort to implement. I also agree with @japaric's sentiments that this may be useful in the future.

Is this part of the RFC, distributing target specs with libraries, crucial to the RFC? It seems to me like we've got a lot to gain from cleaning up the format (I'm a fan of TOML too) and making them less is_like_* and more real_option: ....

@nagisa
Copy link
Member Author

nagisa commented Feb 15, 2017

do you feel that we'd avoid this and instead add a "proper" configuration targeted at the exact use case of where you'd otherwise write if is_like_foo?

Yes, that’s one of the goals by the new schema proposed by this RFC. (note the lack of the is_like keys in it).

That means that we'll probably want some story for the default for each key as it may be common to hit specs without keys specified as the compiler evolves.

Since we already have is_like_unix: true and is_like_everything_else: false by default, one option is to keep those keys’ defaults as applicable to unix.

Is this part of the RFC, distributing target specs with libraries, crucial to the RFC? It seems to me like we've got a lot to gain from cleaning up the format (I'm a fan of TOML too) and making them less is_like_* and more real_option: ....

There’s one counter-point I’d like to make here. I feel that just cleaning up and converting the target specs all by themselves doesn’t necessarily deserve a full RFC – it is mostly an implementation detail of the rustc compiler. It is also easy to support the old custom target specs for a while alongside the new ones to provide a migration path. In that sense, my original intent with this RFC was more to propose the idea of distributing target specs with the libcore/libstd and less the target spec refactor.

I was thinking about working the target spec refactor on the weekends for a while now, and I feel like it would be able to land it just fine with a PR FCP (those didn’t exist back when I wrote the RFC either) for tools and compiler teams.

@alexcrichton
Copy link
Member

Ok that sounds reasonable to me, and yeah for defaults I think we'd just need to be proactive about choosing a good default to break as few targets as possible, but in theory the targets that would be broken are esoteric regardless and as @jmesmon mentions custom targets aren't really that stable today anyway.

I'd also be ok yeah with just a PR to modify these pieces of the compiler (and rework the target specs a bit). If you're ok with that and separating out the distribution of the target specs for now perhaps we can close this RFC?

@nagisa nagisa closed this Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-dev-tools Relevant to the development tools team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants