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

Add text for the CFG OS Version RFC #3379

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chriswailes
Copy link

@chriswailes chriswailes commented Jan 25, 2023

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

text/3349-cfg-os-version.md Outdated Show resolved Hide resolved
text/3349-cfg-os-version.md Outdated Show resolved Hide resolved
# 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.
Copy link
Contributor

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.

Copy link
Author

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.


```toml
[target.x86_64-pc-windows-msvc]
min_os_version_windows = "6.0.6000" # Vista
Copy link
Contributor

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?

Copy link
Author

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?

Copy link
Member

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.

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"'`.
Copy link
Contributor

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.

Copy link
Author

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.


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.
Copy link
Contributor

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?

Suggested change
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.

Copy link
Author

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.

Copy link
Author

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.)

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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.

# 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.
Copy link
Contributor

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?

Copy link
Author

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 Show resolved Hide resolved
Comment on lines 99 to 100
* Command line
* Cargo.toml target sections
Copy link
Contributor

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?

Copy link
Author

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 Show resolved Hide resolved
@ehuss ehuss added T-lang Relevant to the language team, which will review and decide on the RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. labels Jan 25, 2023
}
```

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):
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

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.

Copy link
Author

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.


## 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.
Copy link
Contributor

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.

Copy link
Author

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?


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.
Copy link
Contributor

@ehuss ehuss Jan 27, 2023

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.)

Copy link
Author

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.

Copy link

@madsmtm madsmtm left a 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 Show resolved Hide resolved

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.
Copy link

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

Copy link
Author

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?

Copy link

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.

}
```

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):
Copy link

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.

Copy link
Author

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.


```toml
[target.x86_64-pc-windows-msvc]
min_os_version_windows = "6.0.6000" # Vista
Copy link

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).


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
Copy link

@madsmtm madsmtm Mar 3, 2023

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.

@ehuss
Copy link
Contributor

ehuss commented Dec 11, 2023

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

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 11, 2023

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.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Dec 11, 2023
@BlackHoleFox
Copy link

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 os_version_min CFG inside of just the compiler/standard library. This avoids all the questions around Cargo support, the pre-built standard library (Rust defines minimum supported OS versions, this macro would just be used internally for clarity as opposed to the excessive dlsym-ing today), etc.

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.

@retep998
Copy link
Member

retep998 commented Jan 3, 2024

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.

@tmandry
Copy link
Member

tmandry commented Jan 4, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
Status: FCP postpone
Status: Final Comment
Development

Successfully merging this pull request may close these issues.

None yet