5

Add deployment-target --print flag for Apple targets by BlackHoleFox · Pull Requ...

 2 years ago
source link: https://github.com/rust-lang/rust/pull/105354
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Conversation

Contributor

This is very useful for crates that need to know what the Apple OS deployment target is for their build scripts or inside of a build environment. Right now, the defaults just get copy/pasted around the ecosystem since they've been stable for so long. But with #104385 in progress, that won't be true anymore and everything will need to move. Ideally whenever it happens again, this could be less painful as everything can ask the compiler what its default is instead.

To show examples of the copy/paste proliferation, here's some crates and/or apps that do:

messense reacted with thumbs up emoji

Collaborator

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

rustbot

added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Dec 6, 2022

Collaborator

These commits modify compiler targets.
(See the Target Tier Policy.)

Contributor

Author

@rustbot label O-macos O-ios

rustbot

added O-ios Operating system: iOS O-macos Operating system: macOS

labels

Dec 6, 2022

Contributor

Author

So far I'm unsure of the flag name. minimum-version might be better but deployment-version might be easier to find?

r? compiler

Contributor

Contributor

This adds a new insta-stable compiler flag. This seems like a good solution to the problem for me.

@rfcbot fcp merge

rfcbot

commented

Dec 16, 2022

•

edited by michaelwoerister

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot

added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it.

labels

Dec 16, 2022

"ios" => ios_deployment_target(),

"watchos" => watchos_deployment_target(),

"tvos" => tvos_deployment_target(),

_ => unreachable!("unknown Apple target OS"),

This arm is reachable with custom target specs.
I suggest returning an Option here, and relying on it in the driver instead of using sess.target.is_like_osx.

Contributor

Author

Ah, thanks, and good to know. Moved up to rustc_driver. For the future, why is doing it there better? Do custom target specs go through a different codepath that wouldn't panic?

This comment has been minimized.

rustbot

added the A-testsuite Area: The testsuite used to check the correctness of rustc label

Dec 21, 2022

Contributor

Author

There's now also a test for valid use of the new flag. It isn't very picky about the output beyond requiring that it only contains {Digit}.0. Imo its not worth hardcoding rustc's minimum macOS version in another place because there's already compile tests that verify it. That and it makes updating it later more tedious.

Contributor

umbrella The latest upstream changes (presumably #106087) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk

removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label

Jan 10, 2023

Member

One concern I have with making --print=deployment-target a separate flag is that my intuition that when this is being invoked, tools will also be invoking rustc to obtain other information, such as cfg, at the same time. If cargo wanted to make deployment target into an environment variable passed down to build scripts (which would make a ton of sense to me), it would probably be ideal for it if it could obtain all the information it needs here in one of those rustc invocations that it already executes.

It may also be worth considering if crates may want to cfg based on apple’s deployment target and if there perhaps might be a mechanism for them that's better than having to invoke rustc in their build scripts. Maybe,, perhaps, it would make sense to expose it as a target_* cfg? This problem is also relevant to some other targets such as FreeBSD as well, so maybe such a mechanism could be more generally applicable across targets?

For what it is worth, multiple --print flags already work today, but it is hard and error prone to tell where the output from one print ends and begins for the other. A solution there might very well simply be in our changing the --print infrastructure to account for --error-format=json or something. Another – changing the output of --print=deployment-target to be more self-describing in the first place.

Member

This has been discussed many times, https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Pre-RFC.3A.20CFG.20OS.20Version is one of the more recent efforts. Anecdotally, they seem to tend to hit snags as they often come in with only planned support for a single OS, and wish to leave the others as future work, which may not be viable (in the case of Apple targets, given that we already have support for this in the compiler as it's not exactly an optional aspect of those targets, my feeling is that it seems like it would need to be addressed).

Regardless, I'm not sure these two overlap that much -- the cfgs that have been suggested are in the form of an os_version_min predicate, and it isn't clear how that will come through to build scripts (FWIW, I'd love to be able to use this in cc).

Contributor

Author

Like what @thomcc said, I think this should exist in parallel with a hypothetical os_version_min. One's for your own Rust code and the other for anything (clang, gcc, swiftc, etc) running inside of a build script.

and it isn't clear how that will come through to build scripts

In the same grouping as the other cfg!(platform_stuff) conditionals, I don't think it would.

If cargo wanted to make deployment target into an environment variable passed down to build scripts (which would make a ton of sense to me), it would probably be ideal for it if it could obtain all the information it needs here in one of those rustc invocations that it already executes.

At least on Apple platforms, I don't know what else would be useful for cargo to get from rustc and then pass down. Or, do you mean that there should be some new print that gives it the deployment target and other unrelated information?

...Another – changing the output of --print=deployment-target to be more self-describing in the first place.

Do you mean something like deployment_target=10.x or similar? I don't think that JSON is a good option for a build script, since it usually just slows compile times down. Do you imagine something simple like above or would it be much more complex to the extent you need a crate like rustc_version to do anything?

Member

Do you mean something like deployment_target=10.x or similar? [...] Do you imagine something simple like above or would it be much more complex to the extent you need a crate like rustc_version to do anything?

Something as simple as deployment_target=10.x is kind of exactly what I’m thinking of, again drawing parallels with the format of output of --print=cfg. The parsing code of --print=both output then can special-case deployment_target= prefix and be done with it.

rfcbot

added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised.

and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off.

labels

Apr 20, 2023

bellThis is now entering its final comment period, as per the review above. bell

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

Reviewers

petrochenkov

petrochenkov left review comments

lcnr

lcnr left review comments

oli-obk

oli-obk requested changes
Assignees

oli-obk

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. O-ios Operating system: iOS O-macos Operating system: macOS T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

None yet

11 participants

report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK