Skip to content

Clean up dependency tracking in Rustbuild [1/2] - #50904

Merged
bors merged 5 commits into
rust-lang:masterfrom
collin5:b50509
Jun 3, 2018
Merged

Clean up dependency tracking in Rustbuild [1/2]#50904
bors merged 5 commits into
rust-lang:masterfrom
collin5:b50509

Conversation

@collin5

Copy link
Copy Markdown
Contributor

Initial refactor of the Mode enum. Still a WIP
Ref #50509

r? @Mark-Simulacrum

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2018
@collin5
collin5force-pushed the b50509 branch 2 times, most recently from c73b239 to 5f40201CompareMay 19, 2018 20:54
Comment threadsrc/bootstrap/lib.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This comment should be modified for Codegen since we place its output in -codegen directory.

Comment threadsrc/bootstrap/lib.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's instead combine all of the tools into one pattern or have them after each other so it's clearer what's going on

Comment threadsrc/bootstrap/tool.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe we're calling CleanTools with Mode::Rustc instead of RustcTool in at least some places. Let's change the field to cause: Mode (or a different name if you can come up with something better) and switch these back to Mode::Std, Mode::Test and Mode::Rustc.

@collin5collin5May 22, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hi, @Mark-Simulacrum Just to make sure I'm getting this right. Do you mean we should change the field mode: Mode on CleanTools to cause: Mode? Also, should the values just remain as they were? Thanks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, change the field to cause, and change values to Mode::Std vs. Mode::ToolStd since the cause was Std, not a tool.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Got it. Thanks again!

Comment threadsrc/bootstrap/tool.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be self.mode instead

Comment threadsrc/bootstrap/tool.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We'll want prepare_tool_cargo to take the mode as an argument; that'll make future changes easier. Generally speaking we should be threading tool modes through so that we never just pick "ToolRustc" for no reason.

Comment threadsrc/bootstrap/tool.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above, this should thread mode through from arguments

@collin5

Copy link
Copy Markdown
ContributorAuthor

Hi @Mark-Simulacrum, this should now be ready for another look. Thanks!

Comment threadsrc/bootstrap/test.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be Mode::ToolRustc as we're building a tool

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! let me fix this ASAP

Comment threadsrc/bootstrap/test.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here, Mode::ToolRustc

Comment threadsrc/bootstrap/builder.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's implement an is_tool method on Tool.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

By this, do you mean we should use a closure is_tool that checks if mode is in ToolRustc, ToolStd, etc?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not quite -- method on Tool, not a closure.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It makes sense now. Thanks!

@rust-highfive

Copy link
Copy Markdown
Contributor

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:45:16] ...............................................................i....................................
[00:45:20] ....................................................................................................
[00:45:25] ....................................................................................................
[00:45:31] ............................................................................................i.......
[00:45:34] ..........iiiiiiiii...................................................
[00:45:34] [00:45:34] travis_fold:start:test_ui_nll
travis_time:start:test_ui_nll
Check compiletest suite=ui mode=ui compare_mode=nll (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
[00:46:21] ...............................................................i....................................
[00:46:25] ....................................................................................................
[00:46:30] ....................................................................................................
[00:46:36] ............................................................................................i.......
[00:46:38] ..........iiiiiiiii...................................................
[00:46:38] [00:46:38] finished in 64.166
[00:46:38] travis_fold:end:test_ui_nll
---
[01:21:37] Compiling rand v0.4.2
[01:21:38] Compiling aho-corasick v0.6.4
[01:21:44] Compiling tempdir v0.3.7
[01:22:16] Compiling minifier v0.0.11
[01:22:19] error[E0523]: found two different crates with name `rand` that are not distinguished by differing `-C metadata`. This will result in symbol conflicts between the two.
[01:22:19] |
[01:22:19] 48 | extern crate tempdir;
[01:22:19] | ^^^^^^^^^^^^^^^^^^^^^
[01:22:19] [01:22:19] [01:22:19] error[E0523]: found two different crates with name `rand` that are not distinguished by differing `-C metadata`. This will result in symbol conflicts between the two.
[01:22:19] |
[01:22:19] 48 | extern crate tempdir;
[01:22:19] | ^^^^^^^^^^^^^^^^^^^^^
[01:22:19] ---
[01:22:19] [01:22:19] To learn more, run the command again with --verbose.
[01:22:19] [01:22:19] [01:22:19] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/rustdoc/Cargo.toml" "-p" "rustdoc:0.0.0" "--" "--quiet"
[01:22:19] [01:22:19] [01:22:19] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:22:19] Build completed unsuccessfully in 0:39:15
[01:22:19] Build completed unsuccessfully in 0:39:15
[01:22:19] Makefile:58: recipe for target 'check' failed
[01:22:19] make: *** [check] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0e26c3ee
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@collin5
collin5force-pushed the b50509 branch 2 times, most recently from 7f47fb6 to 2c9ded9CompareMay 28, 2018 00:17
@rust-highfive

Copy link
Copy Markdown
Contributor

The job mingw-check of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:07:29] Checking minifier v0.0.11
[00:07:30] Checking rustdoc v0.0.0 (file:///checkout/src/librustdoc)
[00:07:38] Checking rustdoc-tool v0.0.0 (file:///checkout/src/tools/rustdoc)
[00:07:39] Finished release [optimized] target(s) in 17.28s
[00:07:39] thread 'main' panicked at 'target_deps_dir.read_dir() failed with No such file or directory (os error 2)', bootstrap/compile.rs:1097:20
[00:07:39] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:07:39] travis_time:end:stage0-rustdoc:start=1527467194686142517,finish=1527467211997598076,duration=17311455559

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

bors commented May 28, 2018

Copy link
Copy Markdown
Collaborator

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

@rust-highfive

Copy link
Copy Markdown
Contributor

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:05:20] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:21] tidy error: /checkout/src/bootstrap/doc.rs:808: line longer than 100 chars
[00:05:22] some tidy checks failed
[00:05:22] [00:05:22] [00:05:22] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:22] [00:05:22] [00:05:22] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:22] Build completed unsuccessfully in 0:02:03
[00:05:22] Build completed unsuccessfully in 0:02:03
[00:05:22] Makefile:79: recipe for target 'tidy' failed
[00:05:22] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:012c8946
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@collin5

Copy link
Copy Markdown
ContributorAuthor

Fixed! cc @Mark-Simulacrum

Comment threadsrc/bootstrap/builder.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we instead make this inherent, so mode.is_tool()? I think I may have previously mislead you; I intended this to be implemented on Mode.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No worries! Going to fix this ASAP. Thanks

@bors

bors commented May 31, 2018

Copy link
Copy Markdown
Collaborator

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

@Mark-SimulacrumMark-Simulacrum 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 Jun 2, 2018
@collin5

Copy link
Copy Markdown
ContributorAuthor

Hi @Mark-Simulacrum, this should be fixed now. Thanks!

@kennytmkennytm 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 3, 2018

@Mark-SimulacrumMark-Simulacrum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is the last thing!

Comment threadsrc/bootstrap/builder.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be !mode.is_tool().

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops! missed this.

@collin5

Copy link
Copy Markdown
ContributorAuthor

Fixed! Thank you

@Mark-Simulacrum

Copy link
Copy Markdown
Member

If you can clean up the commit history a little bit -- maybe merging in the is_tool changes into one commit -- then I'll r+. Thanks!

make is_tool inherent prop of mode
fix errors from rebase
resolve issues from review
@collin5

Copy link
Copy Markdown
ContributorAuthor

Done! thank you.

@Mark-SimulacrumMark-Simulacrum changed the title [WIP] Clean up dependency tracking in RustbuildClean up dependency tracking in RustbuildJun 3, 2018
@Mark-SimulacrumMark-Simulacrum changed the title Clean up dependency tracking in RustbuildClean up dependency tracking in Rustbuild [1/2]Jun 3, 2018
@Mark-Simulacrum

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 3, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit 36eafe5 has been approved by Mark-Simulacrum

@borsbors 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 3, 2018
@bors

bors commented Jun 3, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 36eafe5 with merge be5f17c...

bors added a commit that referenced this pull request Jun 3, 2018
Clean up dependency tracking in Rustbuild [1/2]
Initial refactor of the `Mode` enum. Still a WIP
Ref #50509
r? @Mark-Simulacrum
@bors

bors commented Jun 3, 2018

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing be5f17c to master...

@bors
bors merged commit 36eafe5 into rust-lang:masterJun 3, 2018
@collin5
collin5 deleted the b50509 branch June 4, 2018 14:59
bors added a commit that referenced this pull request Jul 23, 2018
Clean up dependency tracking in Rustbuild [2/2]
Make `clear_if_dirty` calls in `Builder::cargo` with stamp dependencies for the given Mode.
Continuation of #50904
Ref issue #50509
r? @Mark-Simulacrum
bors added a commit that referenced this pull request Sep 10, 2018
Clean up dependency tracking in Rustbuild [2/2]
Make `clear_if_dirty` calls in `Builder::cargo` with stamp dependencies for the given Mode.
Continuation of #50904
Ref issue #50509
r? @Mark-Simulacrum
bors added a commit that referenced this pull request Sep 17, 2018
Clean up dependency tracking in Rustbuild [2/2]
Make `clear_if_dirty` calls in `Builder::cargo` with stamp dependencies for the given Mode.
Continuation of #50904
Ref issue #50509
r? @Mark-Simulacrum
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: 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.

5 participants

@collin5@rust-highfive@bors@Mark-Simulacrum@kennytm