Apple: Do not specify an SDK version in `rlib` object files #131016

madsmtm · 2024-09-29T12:12:33Z

This was added in #114114, but is unnecessary, since it ends up being overwritten when linking anyhow, and it feels wrong to embed some arbitrary SDK version in here. The object files produced by LLVM also do not set this, and the tooling shows n/a when it's 0, so it seems to genuinely be optional in object files.

I've also added a test for the different places the SDK version shows up, and documented a bit more in the code how SDK versions work.

See #129432 for the bigger picture.

Tested with (excludes the same few targets as in #130435):

./x test tests/run-make/apple-sdk-version --target aarch64-apple-darwin,aarch64-apple-ios,aarch64-apple-ios-macabi,aarch64-apple-ios-sim,aarch64-apple-tvos,aarch64-apple-tvos-sim,aarch64-apple-visionos,aarch64-apple-visionos-sim,aarch64-apple-watchos,aarch64-apple-watchos-sim,arm64_32-apple-watchos,armv7k-apple-watchos,armv7s-apple-ios,x86_64-apple-darwin,x86_64-apple-ios,x86_64-apple-ios-macabi,x86_64-apple-tvos,x86_64-apple-watchos-sim,x86_64h-apple-darwin
IPHONEOS_DEPLOYMENT_TARGET=10.0 ./x test tests/run-make/apple-sdk-version --target=i386-apple-ios

CC @BlackHoleFox, you originally commented on these values.

@rustbot label O-apple

rustbot · 2024-09-29T12:12:41Z

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-09-29T12:12:43Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

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

This is unnecessary, since it ends up being overwritten when linking anyhow, and it feels wrong to embed some arbitrary SDK version in here.

jieyouxu

Thanks, the changes look reasonable to me but I'll wait for other apple target maintainers to double-check. I have one question regarding the test.

jieyouxu · 2024-09-29T13:50:21Z

tests/run-make/apple-sdk-version/rmake.rs

+        // Test with clang
+        let file_name = format!("foo_cc{file_ext}");
+        rustc()
+            .target(target())
+            .crate_type("bin")
+            .arg("-Clinker-flavor=gcc")


Question: is this comment or arg accurate, the comment says clang, but linker-flavor is gcc?

The comment is mostly accurate, it tests with cc, which is basically always Clang on Apple platforms, it's just how the linker-flavor option works.

jieyouxu · 2024-09-29T13:52:42Z

Should this get relnotes? Even if it doesn't technically change how object file SDK version is treated if I understand this correctly.

madsmtm · 2024-09-29T14:59:15Z

Thanks, the changes look reasonable to me but I'll wait for other apple target maintainers to double-check. I have one question regarding the test.

I'll tag @thomcc as well then.

Should this get relnotes? Even if it doesn't technically change how object file SDK version is treated if I understand this correctly.

This is only used inside create_object_file, and that, in turn, is only used for constructing rlibs. So no, it doesn't make sense to relnote, the difference is only observable if you manually unarchive the rlib (and the format of that is explicitly unspecified).

BlackHoleFox

Nice PR, everything looks fine to me and more test coverage too.

I did indeed comment on the original values because Magic Numbers are odd, but it was harmless in removing a linker warning with no actual cost beyond a strangeness factor. I didn't dig in very far (should have looked at LLVM like you did). Since it turns out the SDK version is actually optional at this stage of linking, there doesn't appear to be any good reason left for rustc to carry that added strange.

jieyouxu · 2024-10-02T09:36:55Z

Thanks @BlackHoleFox . In that case,

@bors r+ rollup

bors · 2024-10-02T09:36:58Z

📌 Commit 6b06ceb has been approved by jieyouxu

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#130863 (Relax a debug assertion for dyn principal *equality* in codegen) - rust-lang#131016 (Apple: Do not specify an SDK version in `rlib` object files) - rust-lang#131140 (Handle `rustc_hir_analysis` cases of `potential_query_instability` lint) - rust-lang#131141 (mpmc doctest: make sure main thread waits for child threads) - rust-lang#131150 (only query `params_in_repr` if def kind is adt) - rust-lang#131151 (Replace zero-width whitespace with a visible `\` in the PR template) - rust-lang#131152 (Improve const traits diagnostics for new desugaring) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131016 - madsmtm:no-sdk-version-in-object, r=jieyouxu Apple: Do not specify an SDK version in `rlib` object files This was added in rust-lang#114114, but is unnecessary, since it ends up being overwritten when linking anyhow, and it feels wrong to embed some arbitrary SDK version in here. The object files produced by LLVM also do not set this, and the tooling shows `n/a` when it's `0`, so it seems to genuinely be optional in object files. I've also added a test for the different places the SDK version shows up, and documented a bit more in the code how SDK versions work. See rust-lang#129432 for the bigger picture. Tested with (excludes the same few targets as in rust-lang#130435): ```console ./x test tests/run-make/apple-sdk-version --target aarch64-apple-darwin,aarch64-apple-ios,aarch64-apple-ios-macabi,aarch64-apple-ios-sim,aarch64-apple-tvos,aarch64-apple-tvos-sim,aarch64-apple-visionos,aarch64-apple-visionos-sim,aarch64-apple-watchos,aarch64-apple-watchos-sim,arm64_32-apple-watchos,armv7k-apple-watchos,armv7s-apple-ios,x86_64-apple-darwin,x86_64-apple-ios,x86_64-apple-ios-macabi,x86_64-apple-tvos,x86_64-apple-watchos-sim,x86_64h-apple-darwin IPHONEOS_DEPLOYMENT_TARGET=10.0 ./x test tests/run-make/apple-sdk-version --target=i386-apple-ios ``` CC `@BlackHoleFox,` you [originally commented on these values](rust-lang#114114 (comment)). `@rustbot` label O-apple

rustbot assigned jieyouxu Sep 29, 2024

madsmtm mentioned this pull request Sep 29, 2024

Apple deployment target, SDK root and SDK version #129432

Open

madsmtm added 3 commits September 29, 2024 14:45

Document a bit more how the SDK version actually works

0bebedd

Add run-make test to check the SDK version(s) that rustc produces

8964c48

Do not specify an SDK version in object files

6b06ceb

This is unnecessary, since it ends up being overwritten when linking anyhow, and it feels wrong to embed some arbitrary SDK version in here.

madsmtm force-pushed the no-sdk-version-in-object branch from 7a6229b to 6b06ceb Compare September 29, 2024 12:45

jieyouxu approved these changes Sep 29, 2024

View reviewed changes

madsmtm changed the title ~~Apple: Do not specify an SDK version in object files~~ Apple: Do not specify an SDK version in rlib object files Sep 29, 2024

BlackHoleFox approved these changes Oct 2, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2024

matthiaskrgr mentioned this pull request Oct 2, 2024

Rollup of 7 pull requests #131158

Merged

bors merged commit 5fd6218 into rust-lang:master Oct 2, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Oct 2, 2024

madsmtm deleted the no-sdk-version-in-object branch October 2, 2024 20:01

simlay mentioned this pull request Oct 14, 2024

WathOS CI broken sonos/dinghy#238

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apple: Do not specify an SDK version in `rlib` object files #131016

Apple: Do not specify an SDK version in `rlib` object files #131016

madsmtm commented Sep 29, 2024

rustbot commented Sep 29, 2024

rustbot commented Sep 29, 2024

jieyouxu left a comment

jieyouxu Sep 29, 2024

madsmtm Sep 29, 2024

jieyouxu commented Sep 29, 2024

madsmtm commented Sep 29, 2024

BlackHoleFox left a comment •

edited

Loading

jieyouxu commented Oct 2, 2024

bors commented Oct 2, 2024

Apple: Do not specify an SDK version in rlib object files #131016

Apple: Do not specify an SDK version in rlib object files #131016

Conversation

madsmtm commented Sep 29, 2024

rustbot commented Sep 29, 2024

rustbot commented Sep 29, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu Sep 29, 2024

Choose a reason for hiding this comment

madsmtm Sep 29, 2024

Choose a reason for hiding this comment

jieyouxu commented Sep 29, 2024

madsmtm commented Sep 29, 2024

BlackHoleFox left a comment • edited Loading

Choose a reason for hiding this comment

jieyouxu commented Oct 2, 2024

bors commented Oct 2, 2024

Apple: Do not specify an SDK version in `rlib` object files #131016

Apple: Do not specify an SDK version in `rlib` object files #131016

BlackHoleFox left a comment •

edited

Loading