-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add text for the CFG OS Version RFC #3379
base: master
Are you sure you want to change the base?
Conversation
text/3349-cfg-os-version.md
Outdated
# Summary | ||
[summary]: #summary | ||
|
||
A new `cfg` key-value option `target_os_version`, and new predicates `os_version_eq`, `os_version_min`, and `os_version_range` that allow users to declare the primary (target-defined) API level required/supported by a block. A second version of the predicates would take an additional key argument allowing targets to specify the versions of different OS components, e.g. kernel and libc versions. |
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.
Can you add some more detail explaining the target_os_version
key-value option somewhere? I don't see it explicitly described.
In particular, this RFC seems to introduce a novel syntax using dots. Could it maybe say why it is introducing that, instead of for example just underscores (like target_os_version_windows
)? That could introduce some complications.
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.
Will do. I have no strong opinions on the syntax for target_os_version
so I'll present both options with some pros/cons listed for each.
text/3349-cfg-os-version.md
Outdated
|
||
```toml | ||
[target.x86_64-pc-windows-msvc] | ||
min_os_version_windows = "6.0.6000" # Vista |
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.
Would the set of keys here need to be hard-coded in Cargo? What is the complete set of keys needed? For example, would there need to be ones for libc
and kernel
?
Would it be feasible to make this more generic so that the values can be passed to rustc
without Cargo interpreting them? For example, something like min_os_version.windows = "6.0.6000"
could then be passed to rustc as-is (something like --min-os-version windows=6.0.6000
or whatever).
Also, what is the behavior if using [target.cfg()]
expressions? Is that allowed? Are duplicates an error?
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 believe a missing key should imply a lack of requirements. This would mean the new behavior would be backwards compatible with existing Cargo manifests.
As for rustc
I think the goal is to put all of the logic in the compiler and have Cargo simply pass through any values found in the manifest. (I'm still learning some of the compiler internals and how rustc
and cargo
interact so please excuse any incorrect terminology).
Regarding [target.cfg()]
, maybe @rylev can better answer that?
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.
Also, what is the behavior if using [target.cfg()] expressions?
I would have assumed this would work just like any other cfg predicate. If the predicate evaluates to true then the dependencies listed are compiled (and passed the same flags as the primary crate). But perhaps I'm missing potential issues besides the ones you listed.
Are duplicates an error?
If you have multiple [target.cfg()]
blocks with the same dependency that all evaluate to true, I suppose we would error. I imagine that cargo could attempt to deduplicate if the feature sets were equal, but it might be clearer to always error for now. Though this is something that we might want to think about more thoroughly.
text/3349-cfg-os-version.md
Outdated
min_os_version_windows = "6.0.6000" # Vista | ||
``` | ||
|
||
When compiling, the user can provide the API levels to compile for: `rustc --cfg 'target_os_version.windows="6.0.6000"'`. |
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.
Is there a particular reason to pass these values in via --cfg
instead of a dedicated flag? In my perspective, --cfg
is used for passing in user-defined cfg values, not ones that will influence how the compiler itself will work.
It might be good to also define the behavior if it is specified more than once. The compiler handles that differently with different flags.
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 have no strong preference between using --cfg
and a dedicated flag.
If there isn't a default policy for handling redundant flags then I would propose that we use the value provided by the right-most instance of the flag. That would be consistent with what seems to be the default for a wide variety of projects.
text/3349-cfg-os-version.md
Outdated
|
||
When compiling, the user can provide the API levels to compile for: `rustc --cfg 'target_os_version.windows="6.0.6000"'`. | ||
|
||
If an end user sets their `os_version.windows` to an incompatible version then the user receives an error. For instance, in the example above where the user is setting their `min_os_version_windows` to Windows Vista, they will receive an error when linking with the standard library which imposes Windows 7 as its minimum `os_version.windows` by default for the `x86_64-pc-windows-msvc` target. |
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 assume this is what is meant here?
If an end user sets their `os_version.windows` to an incompatible version then the user receives an error. For instance, in the example above where the user is setting their `min_os_version_windows` to Windows Vista, they will receive an error when linking with the standard library which imposes Windows 7 as its minimum `os_version.windows` by default for the `x86_64-pc-windows-msvc` target. | |
If an end user sets their `target_os_version.windows` to an incompatible version then the user receives an error. For instance, in the example above where the user is setting their `min_os_version_windows` to Windows Vista, they will receive an error when linking with the standard library which imposes Windows 7 as its minimum `target_os_version.windows` by default for the `x86_64-pc-windows-msvc` target. |
Also, I'm a little unclear here, won't this make it impractical to set any min version above the standard library? I don't see a situation where setting this minimum could be used. (I'm likely having a very fundamental misunderstanding here, but I'm a bit confused.) If this is only useful with build-std, it might be good to make that clear up-front.
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.
You are correct that you couldn't use a minimum version above that used by the standard library, but different versions of the standard library can be built with different minimum versions. For example, the Android Rust toolchain uses a minimum Android API of 31 as opposed to API 19 used by the official Rust toolchain.
I believe the issue you are concerned about is mostly limited to "whole crate" minimum target versions, which I suspect (with no data to back it up) will make up the minority of use cases. I would expect this cfg
option to be used to conditionally compile individual bits of code instead.
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.
(Typo fixes applied to my local edits to this commit.)
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.
won't this make it impractical to set any min version above the standard library?
To expand on what @chriswailes said, this can also be used in a no_std
context as well. You are correct that this becomes much more useful in a buildstd
context like in the aforementioned internal Android toolchain, but it's not exclusively useful there.
As Chris also points out, libraries can use this to more directly indicate that they are reliant on an API that is present only in some minimum API version number. This is not currently possible to do unless you have some bespoke features indicating this which if used incorrectly lead to just bad linker errors.
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.
Usually no_std
is equivalent to "no operating system", so it seems like target_os_version
wouldn't really apply there? What would that mean?
If this is only for users that build the standard library themselves, I would suggest making that clear up front. That is generally not the primary workflow for most Rust developers (I suspect), so this seems like a relatively niche feature.
It also makes me wonder if this should be more hidden or otherwise not exposed in Cargo. For example, it sounds like it would not be usable for the typical user of crates.io.
Or maybe I am still quite confused as to how this would work.
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.
Usually no_std is equivalent to "no operating system", so it seems like target_os_version wouldn't really apply there? What would that mean?
Some libraries might choose to be able to work in environments where std is not supported but that still have some operating system facilities. After all no_std
does not mean no OS, just that std makes assumptions about the platform that the program cannot make.
For examples, windows-rs
has (at least last time I checked), no_std
support even though its purpose is to interact with the Windows operating system. Users of that library sometimes cannot rely on the standard library because they are in a Windows context that does not support all the features that std relies on.
While the assumption of there not being an OS at all is certainly more common, it's not the only use of no_std
.
If this is only for users that build the standard library themselves, I would suggest making that clear up front
I think this would make sense to do with the caveat above of the possibility of this making sense in a no_std
context. The most likely scenario for using this feature does seem to ultimately be in a buildstd
environment.
text/3349-cfg-os-version.md
Outdated
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
|
||
The various target API version `cfg` predicates allow users to conditionally compile code based on the API version supported by the target platform. Each platform is responsible for defining a default key, a set of keys it supports, and functions that are able to compare the version strings they use. A set of comparison functions can be provided by `rustc` for common formats such as 2- and 3-part semantic versioning. When a platform detects a key it doesn’t support it will return `false` and emit a warning. |
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.
"default key" here isn't clear to me. Does that mean it should define a default value for each key it defines?
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 not clear at all. What I had meant was:
If we support an overload of target_os_version
that only takes a version string as an argument the platform will need to specify what that version string will be compared to. E.g. target_os_version("6.1.7600")
would be interpreted as target_os_version("windows", "6.1.7600")
.
The more I think about this the less I like it and we should probably require explicit key arguments?
text/3349-cfg-os-version.md
Outdated
* Command line | ||
* Cargo.toml target sections |
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.
What command-line is this referring to?
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.
An argument passed to either cargo
or rustc
. I've updated this to "Command line arguments to rustc or cargo" but can change the language further if you have a better suggestion.
text/3349-cfg-os-version.md
Outdated
} | ||
``` | ||
|
||
Crate authors can set the API requirements of their Cargo configuration file under the [`target key`](https://doc.rust-lang.org/cargo/reference/config.html#target) like so (suggested variable name/syntax only): |
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.
How would this interact with things like {MACOSX,IOS,TVOS,WATCHOS}_DEPLOYMENT_TARGET
environment variables?
We currently respect those on the Apple targets, for example.
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.
Ideally, I believe this feature should supersede the use of environment variables for this type of use. If we design this correctly, because the env vars wouldn't disappear existing projects should be able to make the transition seamlessly by replacing any tests of env vars (in a build.rs
file I assume) with annotations.
If I misunderstood your comment and you are instead talking about the platforms supported target then I would say that each platform should be able to test env vars to identify version information.
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.
No, that answers the question. That said, I think we have to support the environment vars to correctly link against C programs compiled with those environment vars. I might be misremembering the details though.
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.
Ideally, I believe this feature should supersede the use of environment variables for this type of use. If we design this correctly, because the env vars wouldn't disappear existing projects should be able to make the transition seamlessly by replacing any tests of env vars (in a build.rs file I assume) with annotations.
Assuming that everyone is on the same page, I would be against superseding the standardized environment variables other platforms/build systems already have in place. That would make things much more annoying if you have something else invoking cargo
that also uses the variables. My example case is XCode.
At work, we define our minimum versions with XCode project variables. XCode is also responsible for invoking cargo
for building Rust components and then our Rust components reuse the the already-defined minimums at build time. If we had to go the other way, we would need to maintain two independent lists since, to the best of my knowledge, XCode does not let you dynamically evaluate what minimum versions are.
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'm not proposing that we scrub the environment of these values. These would still exist and code could use them. I was more suggesting that the new recommended way of testing for OS version information in Rust code would be using these cfg
predicates.
Each platform would also be free to use the environment variables when implementing their support for this feature.
text/3349-cfg-os-version.md
Outdated
|
||
## Storing Information in Metadata | ||
|
||
The values for the various `cfg` requirements must be stored in a crate’s metadata if the value is set. If no value is set the platform’s default API versions are used. If no value is set it will default to the maximum of all the crate’s transitive dependencies’ requirements. |
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.
If I understand this clause correctly then this feature have little resemblance to regular cfg
.
In particular it cannot be used during macro expansion / "preprocessing" and requires something like monomorphisation to implement.
Regular cfg
cannot do any of that.
We have another feature with similar semantics - link time cfg
(#[link(..., cfg(predicate))]
) for which the predicate is evaluated and used only when the final binary (e.g. executable or a shared library) is linked.
A target version cfg evaluated only when producing a "final binary" would certainly be a much more useful feature, in particular it would not require rebuilding standard library (and would be more similar to version headers in C in general), but that's a separate feature that's only similar to regular expansion time cfg
because it can evaluate predicates.
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'm afraid I don't see why this would require monomorphisation. It seems that if the requirements are recorded in the crate metadata rustc can verify that they are met when producing new binaries. If there is a mismatch in requirements an error is produced, otherwise we link normally.
I do agree that being able to evaluate the condition at link time would be ideal. Does the compiler currently support anything like that?
text/3349-cfg-os-version.md
Outdated
|
||
If an end user sets their `os_version.windows` to an incompatible version then the user receives an error. For instance, in the example above where the user is setting their `min_os_version_windows` to Windows Vista, they will receive an error when linking with the standard library which imposes Windows 7 as its minimum `os_version.windows` by default for the `x86_64-pc-windows-msvc` target. | ||
|
||
If a library does not explicitly set its `min_os_target_windows` value, it will automatically be set to the largest `min_windows_build_version` of all of its transitive dependencies. |
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 further explain my confusion. It sounds like this could end up with a crate graph with crates being built with different minimum versions. It seems like the final binary's minimum version would then be the max of those minimum versions. Is that the correct interpretation?
If so, I'm curious if it was considered to use a single minimum version for the entire build graph? That is, Cargo could use the max of the minimums and use that as the floor to use for all crates. (With the caveat that would only work with build-std since it wouldn't be able to "raise" the pre-built std's version.)
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.
Yes, that is the correct interpretation. I had not considered that possibility before but agree that it would be ideal in situations where we can compile every crate.
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 really second this feature, I have worked on checking the {MACOSX,IPHONEOS,TVOS,WATCHOS}_DEPLOYMENT_TARGET
in a library before (using either env!
, a proc-macro or build.rs
), but this is a much nicer solution.
text/3349-cfg-os-version.md
Outdated
|
||
Crates including the standard library must account for various API version requirements for the crate to be able to run. Rust currently has no mechanism for crates to compile different code (or to gracefully fail to compile) depending on the minimum targeted API version. This leads to the following issues: | ||
|
||
* Relying on dynamic detection of API support has a runtime cost. The standard library often performs [dynamic API detection](https://github.com/rust-lang/rust/blob/f283d3f02cf3ed261a519afe05cde9e23d1d9278/library/std/src/sys/windows/compat.rs) falling back to older (and less ideal) APIs or forgoing entire features when a certain API is not available. For example, the [current `Mutex` impl](https://github.com/rust-lang/rust/blob/234099d1d12bef9d6e81a296222fbc272dc51d89/library/std/src/sys/windows/mutex.rs#L1-L20) has a Windows XP fallback. Users who only ever intend to run their code on newer versions of Windows will still pay a runtime cost for this dynamic API detection. Providing a mechanism for specifying which minimum API version the user cares about, allows for statically specifying which APIs a binary can use. |
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.
The standard library no longer supports Windows XP, and this fallback is no longer since rust-lang/rust#81250
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 we think that this example should be removed from the RFC now that it's no longer in the codebase? Does anyone else have another example we could use?
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.
Hmm, perhaps we can just note that it would have allowed the standard library to by default, be compiled without support for Windows XP, but still allowing a user that uses -Zbuild-std
to compile their code for that OS.
text/3349-cfg-os-version.md
Outdated
} | ||
``` | ||
|
||
Crate authors can set the API requirements of their Cargo configuration file under the [`target key`](https://doc.rust-lang.org/cargo/reference/config.html#target) like so (suggested variable name/syntax only): |
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 it would be nice to specify the error case here: If crate foo
specifies a higher version than crate bar
, where bar
has a dependency on foo
, then an error should be emitted.
Related to this, I would like to see is interaction with the existing platform's mechanisms for specifying this (might only be relevant for Apple platforms?).
E.g. when invoking either rustc
or cargo
from Xcode, it would be nice if we could just pick up the {MACOSX,IPHONEOS,TVOS,WATCHOS}_DEPLOYMENT_TARGET
that Xcode sets, instead of the application developer having to specify this both in Cargo.toml
and in Info.plist
. And similarly, we should error here if a crate requires a higher version than is specified with these environment variables.
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 added a sentence mentioning the dependency min-version mismatch error. As for the environment variables, I think it would be up to the platform teams to use those when implementing this feature.
text/3349-cfg-os-version.md
Outdated
|
||
```toml | ||
[target.x86_64-pc-windows-msvc] | ||
min_os_version_windows = "6.0.6000" # Vista |
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.
It would be nice to have specified the cases where raising min_os_version_*
in a crate is a breaking change (e.g. if the standard library has raised theirs first, it probably isn't, but in other cases it may be).
48537d7
to
5ce29ce
Compare
|
||
As previously stated, a mechanism which tries to bridge cross-platform differences under one `min_target_api_version` predicate [was suggested](https://github.com/rust-lang/rfcs/blob/b0f94000a3ddbd159013e100e48cd887ba2a0b54/text/0000-min-target-api-version.md) but was rejected due to different platforms having divergent needs. | ||
|
||
# Prior art |
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.
Additional prior art: The Swift package manager has a way to specify the supported platforms for a given package, which basically works how the proposed feature here works.
I'm going to propose to postpone this RFC. I think we all agree that this would be a great thing to have, but I think there are some big questions, particularly around how version support of pre-built std works, how it might tie into supporting target requirements, how the version information is determined, etc. Primarily, there isn't anyone on the team who has the capacity at this time to champion this feature. @rfcbot fcp postpone |
Team member @ehuss has proposed to postpone 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! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
As an alternative to a full postpone, I think it would be good to start by downscoping this RFC (or making a successor?) implementing something like the proposed This is a very needed feature and Rust lags behind Objective-C, Swift, and Kotlin around of providing a good migration path for users wanting to port platform and OS-version specific code to Rust. Apple and Android's platform features are built around the assumption this is available in the language of choice. |
I remember pushing for this change all the way back in 2020, helping with some of the initial design way back then. Please strongly consider adjusting priorities so that someone can have the capacity to take this on. This feature isn't some nice to have that can be postponed indefinitely but rather an essential aspect of being able to write code that can take advantage of new OS versions in a sane way. |
I like the suggestion by @BlackHoleFox that the best way to move forward is to downscope the RFC to just the lang parts. @chriswailes do you think that it would be possible, and still valuable, to start by nailing those down? |
This RFC is largely the work of @rylev. I've worked with him to update some of the language and then added some additional context from Android.
This is my first RFC PR so I apologize for any mistakes and look forward to working on this proposal with the community.
Rendered