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

Drop time dependency from bootstrap #93685

Merged
merged 1 commit into from
Feb 13, 2022
Merged

Conversation

Mark-Simulacrum
Copy link
Member

This was only used for the inclusion of 'current' dates into our manpages, but
it is not clear that this is practically necessary. The manpage is essentially
never updated, and so we can likely afford to keep a manual date in these files.
It also seems possible to just omit it, but that may cause other tools trouble,
so avoid doing that for now.

This is largely done to reduce bootstrap complexity; the time crate is not particularly
small and in #92480 would have started pulling in num-threads, which does runtime
thread count detection. I would prefer to avoid that, so filing this to just drop the nearly
unused dependency entirely.

r? @pietroalbini

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 6, 2022
@rust-log-analyzer

This comment has been minimized.

@pietroalbini
Copy link
Member

Sounds good.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 6, 2022

📌 Commit 2f23624 has been approved by pietroalbini

@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 Feb 6, 2022
@Mark-Simulacrum
Copy link
Member Author

@bors rollup

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2022
…albini

Drop time dependency from bootstrap

This was only used for the inclusion of 'current' dates into our manpages, but
it is not clear that this is practically necessary. The manpage is essentially
never updated, and so we can likely afford to keep a manual date in these files.
It also seems possible to just omit it, but that may cause other tools trouble,
so avoid doing that for now.

This is largely done to reduce bootstrap complexity; the time crate is not particularly
small and in rust-lang#92480 would have started pulling in num-threads, which does runtime
thread count detection. I would prefer to avoid that, so filing this to just drop the nearly
unused dependency entirely.

r? `@pietroalbini`
@klensy
Copy link
Contributor

klensy commented Feb 7, 2022

Looks like #92480 (comment) hitted here too

@jhpratt
Copy link
Member

jhpratt commented Feb 7, 2022

While I support this change (I didn't even know rustc had a manpage), I'd like to note that the num_threads dependency is only required because std is unsound 🙂 I'll actually go so far as to say this should happen, as the manpage shouldn't show the current(ish) date when they're not actually current.

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 7, 2022
…albini

Drop time dependency from bootstrap

This was only used for the inclusion of 'current' dates into our manpages, but
it is not clear that this is practically necessary. The manpage is essentially
never updated, and so we can likely afford to keep a manual date in these files.
It also seems possible to just omit it, but that may cause other tools trouble,
so avoid doing that for now.

This is largely done to reduce bootstrap complexity; the time crate is not particularly
small and in rust-lang#92480 would have started pulling in num-threads, which does runtime
thread count detection. I would prefer to avoid that, so filing this to just drop the nearly
unused dependency entirely.

r? `@pietroalbini`
@m-ou-se
Copy link
Member

m-ou-se commented Feb 7, 2022

Rollup #93736 failed on this PR with:

error[E0432]: unresolved import `winapi::um::timezoneapi`
   --> src\bootstrap\bin/rustc.rs:236:48
    |
236 |     use winapi::um::{processthreadsapi, psapi, timezoneapi};
    |                                                ^^^^^^^^^^^ no `timezoneapi` in `um`

Looks like the time dependency was the only thing enabling the timezoneapi feature of winapi that boostrap/bin/rustc.rs needs.

@m-ou-se
Copy link
Member

m-ou-se commented Feb 7, 2022

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 7, 2022
This was only used for the inclusion of 'current' dates into our manpages, but
it is not clear that this is practically necessary. The manpage is essentially
never updated, and so we can likely afford to keep a manual date in these files.
It also seems possible to just omit it, but that may cause other tools trouble,
so avoid doing that for now.
@Mark-Simulacrum
Copy link
Member Author

@bors r+ rollup=iffy

Should be fixed.

@bors
Copy link
Contributor

bors commented Feb 7, 2022

📌 Commit 2a8c750 has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 7, 2022
@bors
Copy link
Contributor

bors commented Feb 13, 2022

⌛ Testing commit 2a8c750 with merge 05d1652...

@bors
Copy link
Contributor

bors commented Feb 13, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 05d1652 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 13, 2022
@bors bors merged commit 05d1652 into rust-lang:master Feb 13, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 13, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (05d1652): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.