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

reduce deps for windows-msvc targets for backtrace #113432

Merged
merged 3 commits into from
Aug 11, 2023

Conversation

klensy
Copy link
Contributor

@klensy klensy commented Jul 7, 2023

(eventually) mirrors rust-lang/backtrace-rs#543

Some dependencies of backtrace don't used on windows-msvc targets, so exclude them:

miniz_oxide (+ adler)
addr2line (+ gimli)
object (+ memchr)

This saves about 30kb of std.dll + 17.5mb of rlibs

@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2023

r? @joshtriplett

(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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 7, 2023
@klensy klensy marked this pull request as draft July 7, 2023 09:02
@rust-log-analyzer

This comment has been minimized.

@klensy klensy force-pushed the ms-cut-backtrace branch from 5d8f56e to 6221b1d Compare July 7, 2023 09:20
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 7, 2023
@klensy klensy force-pushed the ms-cut-backtrace branch from d2b7721 to 0428b24 Compare July 8, 2023 09:52
@rust-log-analyzer

This comment has been minimized.

@klensy klensy force-pushed the ms-cut-backtrace branch 2 times, most recently from 85fa933 to 55aafaf Compare July 8, 2023 10:58
@rust-log-analyzer

This comment has been minimized.

@klensy klensy force-pushed the ms-cut-backtrace branch from 55aafaf to 97b46d5 Compare July 8, 2023 11:03
@rust-log-analyzer

This comment has been minimized.

@klensy klensy force-pushed the ms-cut-backtrace branch from 7b0e57f to 6eeff65 Compare July 8, 2023 11:29
@bors
Copy link
Collaborator

bors commented Jul 10, 2023

☔ The latest upstream changes (presumably #108485) made this pull request unmergeable. Please resolve the merge conflicts.

@klensy klensy force-pushed the ms-cut-backtrace branch from 6eeff65 to 8238361 Compare July 28, 2023 11:35
@klensy klensy marked this pull request as ready for review July 28, 2023 11:36
@workingjubilee workingjubilee added the O-windows Operating system: Windows label Jul 30, 2023
@klensy
Copy link
Contributor Author

klensy commented Jul 31, 2023

@rustbot ready this eventually be reviewed in backtrace-rs repo, but let's not block on it

@klensy
Copy link
Contributor Author

klensy commented Aug 11, 2023

Let's reroll r?

@rustbot
Copy link
Collaborator

rustbot commented Aug 11, 2023

Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

features = ['read_core', 'elf', 'macho', 'pe', 'unaligned', 'archive']

[target.'cfg(not(all(windows, target_env = "msvc", not(target_vendor = "uwp"))))'.dependencies]
miniz_oxide = { version = "0.7.0", optional = true, default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

Why was public = false removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember why ;-)
https://rust-lang.github.io/rfcs/1977-public-private-dependencies.html

Q: Where is public / private defined?
Dependencies are private by default and are made public through a public flag on the dependency in the Cargo.toml file. This also means that crates created before the implementation of this RFC will have all their dependencies private.

Probably some unintentional change, but it shouldn't affect anything, as by default it public = false. I can try revert this?

Copy link
Member

Choose a reason for hiding this comment

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

If it makes no difference then it's fine. But if so then I'm curious why it was added in the first place.

@ChrisDenton ChrisDenton self-assigned this Aug 11, 2023
@ChrisDenton
Copy link
Member

This looks great to me! I'm still a bit curious about the public=false thing but I don't see that as a reason to block this PR, which has been around awhile already.

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 11, 2023

📌 Commit 8238361 has been approved by ChrisDenton

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 Aug 11, 2023
@bors
Copy link
Collaborator

bors commented Aug 11, 2023

⌛ Testing commit 8238361 with merge 08691f0...

@bors
Copy link
Collaborator

bors commented Aug 11, 2023

☀️ Test successful - checks-actions
Approved by: ChrisDenton
Pushing 08691f0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 11, 2023
@bors bors merged commit 08691f0 into rust-lang:master Aug 11, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 11, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (08691f0): comparison URL.

Overall result: ❌ regressions - 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.6% [0.6%, 0.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
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
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.3% [-2.3%, -2.3%] 1
Improvements ✅
(secondary)
-1.4% [-1.4%, -1.4%] 1
All ❌✅ (primary) -2.3% [-2.3%, -2.3%] 1

Cycles

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

Binary size

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

Bootstrap: 632.73s -> 632.877s (0.02%)

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. O-windows Operating system: Windows 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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants