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.

Sign up for GitHub

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

Jump to bottom

Tracking Issue for Deprecate target_vendor #100343

Open
3 tasks
pnkfelix opened this issue Aug 9, 2022 · 5 comments
Open
3 tasks

Tracking Issue for Deprecate target_vendor #100343

pnkfelix opened this issue Aug 9, 2022 · 5 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Aug 9, 2022

This is a tracking issue for the MCP "Deprecate target_vendor" (rust-lang/lang-team#102).

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

  • @scottmcm asked "What, technically, do you expect "deprecated" to mean here?"
  • @thomcc wondered if adding more values to target_family might be problematic.

Implementation history
@pnkfelix pnkfelix added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Aug 9, 2022
@pnkfelix pnkfelix changed the title Tracking Issue for XXX Tracking Issue for Deprecate target_vendor Aug 9, 2022
@joshtriplett joshtriplett added the S-tracking-impl-incomplete Status: The implementation is incomplete. label Aug 10, 2022
@taiki-e taiki-e mentioned this issue Feb 13, 2023
Merged
Thomasdezeeuw added a commit to Thomasdezeeuw/socket2 that referenced this issue Feb 25, 2023
@Thomasdezeeuw
Remove usage of target_vendor = "apple"
a23db24 
Being deprecated, see:
 * <rust-lang/lang-team#102>
 * <rust-lang/rust#100343>
@Thomasdezeeuw Thomasdezeeuw mentioned this issue Feb 25, 2023
Merged
Thomasdezeeuw added a commit to Thomasdezeeuw/socket2 that referenced this issue Feb 25, 2023
@Thomasdezeeuw
Remove usage of target_vendor = "apple"
668ed40 
Being deprecated, see:
 * <rust-lang/lang-team#102>
 * <rust-lang/rust#100343>
Thomasdezeeuw added a commit to rust-lang/socket2 that referenced this issue Feb 25, 2023
@Thomasdezeeuw
Remove usage of target_vendor = "apple"
28c02ca 
Being deprecated, see:
 * <rust-lang/lang-team#102>
 * <rust-lang/rust#100343>
@notgull notgull mentioned this issue Feb 25, 2023
Merged
@Thomasdezeeuw Thomasdezeeuw mentioned this issue Apr 12, 2024
Open
@QuentinPerez QuentinPerez mentioned this issue Apr 16, 2024
Open
@workingjubilee
Copy link
Contributor

workingjubilee commented Apr 18, 2024 •
edited

With five "operating systems" (really, macOS, iOS, and different skins on iOS) released by Apple, we really ought to implement target_family = "apple" first. Either we can land that without destroying the ecosystem, or we need to reconsider this entire idea.

@thomcc
Copy link
Member

thomcc commented Apr 18, 2024 •
edited

target_family = "apple" first. Either we can land that without destroying the ecosystem...

Yeah, it's tricky to do this due to how much code does env::var("CARGO_CFG_TARGET_FAMILY").unwrap_or_default() == "unix" in build scripts as a way to detect unix (usually not verbatim, it's more common to see target family extracted, and then later matched on with arms for "unix" and "windows"). Technically this is already wrong, but only on a few fairly obscure targets (it also may not have been wrong when they wrote it, as target_family was initially single-valued).

It also can't be cratered, since crater tests only on Linux.

Edit: I've already said this, and it's even been linked to. Lmao.

@workingjubilee
Copy link
Contributor

Yeah. I don't think people should be expected to gouge their eyes out maintaining "every time an Apple-based OS is involved, use target_os for each one instead of target_vendor for all of them, despite the fact that they use basically the same tech". Consider the publishing dates for the current set of Apple OS:

  • 2001-03-24: macOS (FKA Mac OS X)
  • 2007-06-29: iOS
  • 2015-04-24: watchOS
  • 2015-10-29: tvOS (or 2007 Jan, depending on who you ask)
  • 2024-02-24: visionOS

It is entirely possible, at this rate, that we see another Apple OS released before the decade is out! ...But target_family = "apple" is equally fine.

We could consider cratering a different target_family that would affect Linux.

@workingjubilee workingjubilee mentioned this issue Apr 24, 2024
Draft
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 24, 2024
@bors
Auto merge of rust-lang#124355 - workingjubilee:add-target-family-lin…
39bbf14 
…ux, r=<try>

[EXPERIMENT] Crater adding `target_family = "linux"`

The lang team proposed, a while back, [deprecating `target_vendor`](rust-lang#100343).

With five operating systems all from Apple, using very similar, unifying characteristics for each, it is untenable to ask people to `cfg(any(target_os = "macos", target_os = "ios", target_os = "watchos", target_os = "tvos", target_os = "visionos"))`. It is commonly preferred to use `target_vendor = "apple"` for things that are true for all of these OS variants, and we have to offer a replacement that is equally parsimonious.

[The "obvious" choice is to add a `target_family = "apple"`](rust-lang/lang-team#102 (comment)). However, [there is a concern that adding a `target_family = "apple"` won't work without massive ecosystem breakage, because of poorly-designed build scripts](rust-lang/lang-team#102 (comment)). But we currently can't crater a test of that effectively, because most tests only test host compilation, and crater only runs on Linux.

We could, however, test adding a hypothetical `target_family = "linux"`! This would be a hypothetical `target_family` that unites both `target_os = "linux"` and `target_os = "android"` targets. This isn't necessarily an "apples to apples" comparison (more like "penguins to robots"?) but it seems worth trying.

Note that this is **not** a proposal to actually add such a `target_family`. I don't have any arguments about the merits beyond "it seems like a plausible target family we may one day add, it can be added in a way that disrupts relevant build scripts, and it impacts a 'high-value' target".

For maximum coverage I have in fact added the relevant `target_family = "apple"` and `target_family = "linux"` also just in case any test suites somehow run cross-compilation tests.
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 25, 2024
@bors
Auto merge of rust-lang#124355 - workingjubilee:add-target-family-lin…
99e7531 
…ux, r=<try>

[EXPERIMENT] Crater adding `target_family = "linux"`

The lang team proposed, a while back, [deprecating `target_vendor`](rust-lang#100343).

With five operating systems all from Apple, using very similar, unifying characteristics for each, it is untenable to ask people to `cfg(any(target_os = "macos", target_os = "ios", target_os = "watchos", target_os = "tvos", target_os = "visionos"))`. It is commonly preferred to use `target_vendor = "apple"` for things that are true for all of these OS variants, and we have to offer a replacement that is equally parsimonious.

[The "obvious" choice is to add a `target_family = "apple"`](rust-lang/lang-team#102 (comment)). However, [there is a concern that adding a `target_family = "apple"` won't work without massive ecosystem breakage, because of poorly-designed build scripts](rust-lang/lang-team#102 (comment)). But we currently can't crater a test of that effectively, because most tests only test host compilation, and crater only runs on Linux.

We could, however, test adding a hypothetical `target_family = "linux"`! This would be a hypothetical `target_family` that unites both `target_os = "linux"` and `target_os = "android"` targets. This isn't necessarily an "apples to apples" comparison (more like "penguins to robots"?) but it seems worth trying.

Note that this is **not** a proposal to actually add such a `target_family`. I don't have any arguments about the merits beyond "it seems like a plausible target family we may one day add, it can be added in a way that disrupts relevant build scripts, and it impacts a 'high-value' target".

For maximum coverage I have in fact added the relevant `target_family = "apple"` and `target_family = "linux"` also just in case any test suites somehow run cross-compilation tests.
@madsmtm madsmtm mentioned this issue Apr 28, 2024
Merged
@workingjubilee
Copy link
Contributor

Switching from individual target_os to a single target_vendor appears to have revealed several bugs or OS-based variations without clear reasoning in the implementations of several Apple OS for libstd. I no longer believe this is a mere aesthetics or conciseness issue, it is about correctness.

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 30, 2024
@bors
Auto merge of rust-lang#124491 - madsmtm:target_vendor-apple, r=worki…
cc65a5f 
…ngjubilee

Use `target_vendor = "apple"` instead of `target_os = "..."`

Use `target_vendor = "apple"` instead of `all(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos", target_os = "visionos")`.

The apple targets are quite close to being identical, with iOS, tvOS, watchOS and visionOS being even closer, so using `target_vendor` when possible makes it clearer when something is actually OS-specific, or just Apple-specific.
Note that `target_vendor` will [be deprecated in the future](rust-lang#100343), but not before an alternative (like `target_family = "apple"`) is available.

While doing this, I found various inconsistencies and small mistakes in the standard library, see the commits for details. Will follow-up with an extra PR for a similar issue that need a bit more discussion. EDIT: rust-lang#124494

Since you've talked about using `target_vendor = "apple"` in the past:
r? workingjubilee

CC `@simlay,` `@thomcc`
`@rustbot` label O-macos O-ios O-tvos O-watchos O-visionos
bors added a commit to rust-lang-ci/rust that referenced this issue May 1, 2024
@bors
Auto merge of rust-lang#124491 - madsmtm:target_vendor-apple, r=worki…
2e88e9e 
…ngjubilee

Use `target_vendor = "apple"` instead of `target_os = "..."`

Use `target_vendor = "apple"` instead of `all(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos", target_os = "visionos")`.

The apple targets are quite close to being identical, with iOS, tvOS, watchOS and visionOS being even closer, so using `target_vendor` when possible makes it clearer when something is actually OS-specific, or just Apple-specific.
Note that `target_vendor` will [be deprecated in the future](rust-lang#100343), but not before an alternative (like `target_family = "apple"`) is available.

While doing this, I found various inconsistencies and small mistakes in the standard library, see the commits for details. Will follow-up with an extra PR for a similar issue that need a bit more discussion. EDIT: rust-lang#124494

Since you've talked about using `target_vendor = "apple"` in the past:
r? workingjubilee

CC `@simlay,` `@thomcc`
`@rustbot` label O-macos O-ios O-tvos O-watchos O-visionos
@workingjubilee
Copy link
Contributor

In #124355 I ran the crater experiment I mentioned, and the trap did indeed get sprung! It has a number of regressions, but only 82 in total, and as usual, many of these source to a handful of crates.

I believe it may be possible to best-effort lint against this behavior. The arguments already advanced justify doing so:

  • It is already nonresilient due to prior decisions to make target_family an array
  • It is unfair to e.g. wasi to not be able to use these crates
  • It is unclear why we should not be able to add new target_family values going forward
  • There is definitely nothing stopping Windows from releasing "Windows on Wasm" or any other platform doing similar stuff to bring a plague on our head.

An alternative path forward is to persuade Cargo to change the env var they use, and to migrate everyone to the new behavior because they are relying on CARGO_CFG_TARGET_FAMILY working the way they currently expect. This seems strictly worse, as people will likely develop the habit of doing exact comparisons anyways inside their build scripts with the new env var, and we will be churning the entire damn ecosystem for no reason.

A final path forward, the path of least resistance, is, of course, to simply not deprecate target_vendor after all. However, I believe fixing these obstacles has other merits, so I don't really buy it.

github-actions bot pushed a commit to rust-lang/miri that referenced this issue May 3, 2024
@bors
Auto merge of #124491 - madsmtm:target_vendor-apple, r=workingjubilee
81a0c7b 
Use `target_vendor = "apple"` instead of `target_os = "..."`

Use `target_vendor = "apple"` instead of `all(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos", target_os = "visionos")`.

The apple targets are quite close to being identical, with iOS, tvOS, watchOS and visionOS being even closer, so using `target_vendor` when possible makes it clearer when something is actually OS-specific, or just Apple-specific.
Note that `target_vendor` will [be deprecated in the future](rust-lang/rust#100343), but not before an alternative (like `target_family = "apple"`) is available.

While doing this, I found various inconsistencies and small mistakes in the standard library, see the commits for details. Will follow-up with an extra PR for a similar issue that need a bit more discussion. EDIT: rust-lang/rust#124494

Since you've talked about using `target_vendor = "apple"` in the past:
r? workingjubilee

CC `@simlay,` `@thomcc`
`@rustbot` label O-macos O-ios O-tvos O-watchos O-visionos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@joshtriplett @pnkfelix @thomcc @workingjubilee