-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
docs(ref): Add section on Apple deployment target environment variables #13781
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I am a bit unsure about it for some reasons:
[env]
section here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that I should write it more tersely, or what do you mean by "best practice"?
I'm not really aware of other platforms, but I think that these (along with
SDKROOT
) is the only environment variables thatrustc
andclang
ever reads? And then I thought it'd make sense to document them in the same place as where other environment variables in the Rust ecosystem (i.e. Cargo's) is documented.Emphazising
[env]
is not really important, mostly wanted to say that it was possible, i.e. that the parsing of the variables happens after[env]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By best practice I mean this addition isn't going to be use as a reference book you will check very often. It's more like a how-to guide, which requires a different kind of reading flow. For example, the Cargo Guide shares a more similar reading flow with best practices like docs.
We couuld possibly tweak it into a reference-like doc, but at that point we might probably just refer to Apple's official documentation instead.
Then in my opinion this might be better being in rustc doc or elsewhere. Cargo is not the only tool calling rustc. Cargo also doesn't read those env vars, so prefer not adding them here to avoid any false impression that Cargo handles them for users.
I understand discoverability matters. Putting it here seems like a good place to expose to a wider audience. Cargo already have some cookbook like content for some CI best practices. That style of content is a better fit for this I feel like. Rust By Examples might also be a good place for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like, we can combine some practices from #13115 and write something somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've submitted rust-lang/rust#124772 to update the documentation in the platform support docs, let's see where that one goes first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rust-lang/rust#124772 just got merged. Should we close this then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd still prefer this to be here in some way, maybe just as a quick one or two-liner that links to the platform support docs?
I'd also be fine with writing a fuller guide-level documentation, if you think that'd be useful? Though it might also be a bit too platform-specific to really fit in the Cargo guide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, still feel a bit odd to have a paragraph explaining something that Cargo doesn't really read. Even some variables like
RUST_BACKTRACE
, andRUSTUP_TOOLCHAIN
,CLIPPY_CONF_DIR
, which could also potentially affect how Cargo works, are left off because they are either an implementation detail, or not really directly read/written/controlled by Cargo. I just don't know how to fit them in.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a great argument actually.
I'll close this, thanks for taking the time to review and discuss it! If Cargo one day decides to read these environment variables, we can document it then.