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 test for Apple's -weak_framework linker argument #118644

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Dec 5, 2023

The -weak_framework linker argument can sometimes be useful to reduce startup times, and to link newer frameworks while still having older deployment targets.

So I made a test to ensure that it continues to work.

Discussed in #99427.

@rustbot
Copy link
Collaborator

rustbot commented Dec 5, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 5, 2023
@madsmtm
Copy link
Contributor Author

madsmtm commented Dec 5, 2023

I guess a test that runs otool -L and checks the output could also be useful, are those kinds of tests desired?

@madsmtm madsmtm marked this pull request as ready for review December 5, 2023 15:01
@BlackHoleFox
Copy link
Contributor

I guess a test that runs otool -L and checks the output could also be useful, are those kinds of tests desired?

Not your reviewer, but I added tests like that for deployment targets so I don't see why not.

@madsmtm
Copy link
Contributor Author

madsmtm commented Dec 12, 2023

I've added such tests now, thanks for the input

@b-naber
Copy link
Contributor

b-naber commented Dec 16, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 16, 2023

📌 Commit d63d1a1 has been approved by b-naber

It is now in the queue for this repository.

@bors 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 Dec 16, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 16, 2023
…=b-naber

Add test for Apple's `-weak_framework` linker argument

The [`-weak_framework`](https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html) linker argument can sometimes be useful to reduce startup times, and to link newer frameworks while still having older deployment targets.

So I made a test to ensure that it continues to work.

Discussed in rust-lang#99427.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2023
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#118644 (Add test for Apple's `-weak_framework` linker argument)
 - rust-lang#118828 (Remove dead codes in rustc_codegen_gcc)
 - rust-lang#119001 (rustdoc-search: remove parallel searchWords array)

r? `@ghost`
`@rustbot` modify labels: rollup
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 16, 2023
…=b-naber

Add test for Apple's `-weak_framework` linker argument

The [`-weak_framework`](https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html) linker argument can sometimes be useful to reduce startup times, and to link newer frameworks while still having older deployment targets.

So I made a test to ensure that it continues to work.

Discussed in rust-lang#99427.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2023
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#118644 (Add test for Apple's `-weak_framework` linker argument)
 - rust-lang#118828 (Remove dead codes in rustc_codegen_gcc)
 - rust-lang#118830 (Add support for `--env` on `tracked_env::var`)
 - rust-lang#119001 (rustdoc-search: remove parallel searchWords array)
 - rust-lang#119020 (remove `hex` dependency in bootstrap)

r? `@ghost`
`@rustbot` modify labels: rollup
@GuillaumeGomez
Copy link
Member

I think it failed in #119027.

@bors
Copy link
Contributor

bors commented Dec 17, 2023

⌛ Testing commit d63d1a1 with merge a7e25d7...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2023
…-naber

Add test for Apple's `-weak_framework` linker argument

The [`-weak_framework`](https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html) linker argument can sometimes be useful to reduce startup times, and to link newer frameworks while still having older deployment targets.

So I made a test to ensure that it continues to work.

Discussed in rust-lang#99427.
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 17, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 17, 2023
@bors 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 Mar 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2024
…ompiler-errors

Add test for Apple's `-weak_framework` linker argument

The [`-weak_framework`](https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/WeakLinking.html) linker argument can sometimes be useful to reduce startup times, and to link newer frameworks while still having older deployment targets.

So I made a test to ensure that it continues to work.

Discussed in rust-lang#99427.
@bors
Copy link
Contributor

bors commented Mar 18, 2024

⌛ Testing commit 47cab49 with merge 23fb079...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 18, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Some changes occurred in run-make tests.

cc @jieyouxu

@madsmtm
Copy link
Contributor Author

madsmtm commented Mar 18, 2024

The test, as I'd written it before, replaced the string based on the architecture name arm64, which is why it fails on x86_64. I've made the test generic now, and tested locally with ./x test tests/ui/linkage-attr/framework.rs --target=aarch64-apple-darwin,x86_64-apple-darwin with 3 different Xcode versions.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 26, 2024

📌 Commit 440fce1 has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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 Mar 26, 2024
@@ -0,0 +1,23 @@
# only-macos
Copy link
Member

@jieyouxu jieyouxu Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Since this seems like a new run-make test) Does this test need to be written in Makefile? Can it be written in rmake.rs?

Copy link
Contributor Author

@madsmtm madsmtm Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably could - didn't know about rmake.rs, I think I made this PR before that was available

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the timing was a bit unfortunate, but that's okay, we added a tidy check now

@bors
Copy link
Contributor

bors commented Mar 26, 2024

⌛ Testing commit 440fce1 with merge 47ecded...

@bors
Copy link
Contributor

bors commented Mar 26, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 47ecded to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 26, 2024
@bors bors merged commit 47ecded into rust-lang:master Mar 26, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 26, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (47ecded): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.4% [-3.4%, -3.4%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.0% [-0.5%, 0.5%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.5% [0.4%, 0.6%] 2
Regressions ❌
(secondary)
4.9% [2.4%, 7.4%] 4
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.4%, 0.6%] 3

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 671.136s -> 669.555s (-0.24%)
Artifact size: 315.73 MiB -> 315.66 MiB (-0.02%)

madsmtm added a commit to madsmtm/rust that referenced this pull request Apr 6, 2024
This test was introduced in rust-lang#118644, but was over-specified in that it assumed the path of the linker was always `cc`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 7, 2024
…st, r=fmease

Relax framework linking test

This test was introduced by myself in rust-lang#118644, but was over-specified in that it assumed the path of the linker was always `cc`, which [causes a test failure for Chromium](https://issues.chromium.org/issues/332562251).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 7, 2024
Rollup merge of rust-lang#123410 - madsmtm:relax-framework-linking-test, r=fmease

Relax framework linking test

This test was introduced by myself in rust-lang#118644, but was over-specified in that it assumed the path of the linker was always `cc`, which [causes a test failure for Chromium](https://issues.chromium.org/issues/332562251).
@madsmtm madsmtm deleted the macos-weak-linking-test branch April 16, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.