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

Added the --temps-dir option #83846

Merged
merged 10 commits into from
Nov 11, 2021
Merged

Added the --temps-dir option #83846

merged 10 commits into from
Nov 11, 2021

Conversation

torhovland
Copy link
Contributor

Fixes #10971.

The new --temps-dir option puts intermediate files in a user-specified directory. This provides a fix for the issue where parallel invocations of rustc would overwrite each other's intermediate files.

No files are kept in the intermediate directory unless -C save-temps=yes.

If additional files are specifically requested using --emit asm,llvm-bc,llvm-ir,obj,metadata,link,dep-info,mir, these will be put in the output directory rather than the intermediate directory.

This is a backward-compatible change, i.e. if --temps-dir is not specified, the behavior is the same as before.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

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

This comment has been minimized.

@torhovland torhovland marked this pull request as ready for review April 4, 2021 13:56
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me. Could you submit a major change proposal? The compiler team's process for adding new rustc options requires it - just to make sure that it's something we want to support going forward :)

@davidtwco davidtwco 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2021
@torhovland torhovland marked this pull request as draft April 5, 2021 21:17
@bors
Copy link
Contributor

bors commented May 12, 2021

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

@torhovland torhovland marked this pull request as ready for review May 12, 2021 17:35
@bstrie bstrie added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 2, 2021
@davidtwco
Copy link
Member

Apologies, this slipped past me after the major change proposal was accepted. Thanks for your patience.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 16, 2021

📌 Commit 857b19d has been approved by davidtwco

@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 Jun 16, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 16, 2021
Added the --temps-dir option

Fixes rust-lang#10971.

The new `--temps-dir` option puts intermediate files in a user-specified directory. This provides a fix for the issue where parallel invocations of rustc would overwrite each other's intermediate files.

No files are kept in the intermediate directory unless `-C save-temps=yes`.

If additional files are specifically requested using `--emit asm,llvm-bc,llvm-ir,obj,metadata,link,dep-info,mir`, these will be put in the output directory rather than the intermediate directory.

This is a backward-compatible change, i.e. if `--temps-dir` is not specified, the behavior is the same as before.
@JohnTitor
Copy link
Member

Failed in rollup: #86373 (comment)
@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 Jun 16, 2021
@crlf0710
Copy link
Member

crlf0710 commented Jul 4, 2021

@torhovland Ping from triage! Seems this pr's change doesn't work as expected on some platform. Do you want to investigate? Thanks!

@torhovland
Copy link
Contributor Author

Not sure, but it could be that this test command doesn't run as expected on Windows:

$(RUSTC) --crate-type=lib --temps-dir=$(TMPDIR)/temp1 --out-dir=$(TMPDIR) $(TMPDIR)/lib.rs \
		& $(RUSTC) --crate-type=cdylib --temps-dir=$(TMPDIR)/temp2 --out-dir=$(TMPDIR) $(TMPDIR)/lib.rs

https://github.com/rust-lang/rust/pull/86373/files#diff-03a597d35d09133542008990925ff15740cc1781c0bf2c82d21af21712ac1eff

@torhovland
Copy link
Contributor Author

Well, this is becoming a bit of a saga. The Windows tests should run fine, and they also run fine on my personal x86_64-unknown-linux-gnu. But on dist-various-1, which is also x86_64-unknown-linux-gnu it fails now, because it has added -Clinker='arm-none-eabi-gcc', and apparently that linker doesn't understand -m64.

Some other tests do unexport RUSTC_LINKER. My best bet right now is to try that.

@torhovland
Copy link
Contributor Author

@davidtwco This needs another full test run.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2021

📌 Commit 13dbdc6 has been approved by davidtwco

@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 Nov 9, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 9, 2021
Added the --temps-dir option

Fixes rust-lang#10971.

The new `--temps-dir` option puts intermediate files in a user-specified directory. This provides a fix for the issue where parallel invocations of rustc would overwrite each other's intermediate files.

No files are kept in the intermediate directory unless `-C save-temps=yes`.

If additional files are specifically requested using `--emit asm,llvm-bc,llvm-ir,obj,metadata,link,dep-info,mir`, these will be put in the output directory rather than the intermediate directory.

This is a backward-compatible change, i.e. if `--temps-dir` is not specified, the behavior is the same as before.
@bors
Copy link
Contributor

bors commented Nov 10, 2021

⌛ Testing commit 13dbdc6 with merge 262238f20e923810c59dab24dc00ed9c1d8649b2...

@camelid
Copy link
Member

camelid commented Nov 10, 2021

@bors rollup=never (just to make sure it doesn't accidentally get rolled up and cause the rollup to fail)

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 10, 2021

💔 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 Nov 10, 2021
@torhovland
Copy link
Contributor Author

@davidtwco I was able to reproduce this test failure locally, and verify that it works better with staticlib rather than cdylib. Can you please give this another go?

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 10, 2021

📌 Commit 1793a7a has been approved by davidtwco

@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 Nov 10, 2021
@bors
Copy link
Contributor

bors commented Nov 11, 2021

⌛ Testing commit 1793a7a with merge 9dbbbb1...

@bors
Copy link
Contributor

bors commented Nov 11, 2021

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 9dbbbb1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 11, 2021
@bors bors merged commit 9dbbbb1 into rust-lang:master Nov 11, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 11, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9dbbbb1): comparison url.

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

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. 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.

Names of intermediate files collide when running rustc in parallel