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

{compiler-builtins,libc} shim cleanup #44515

Merged
merged 1 commit into from
Oct 7, 2017
Merged

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Sep 12, 2017

Depends on rust-lang/libc#764; opening early for feedback. r? @alexcrichton

@tamird
Copy link
Contributor Author

tamird commented Sep 12, 2017

Hm, seems like I should amend the new comments to reference rust-lang/rfcs#1133. WDYT?

@alexcrichton
Copy link
Member

Using cargo features sounds fine to me!

@tamird
Copy link
Contributor Author

tamird commented Sep 12, 2017

Sounds find to me too! But it doesn't quite work out, which this PR is really about documenting at this point

@tamird
Copy link
Contributor Author

tamird commented Sep 12, 2017

Alright, this is ready for a real look!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 12, 2017

📌 Commit d10c760 has been approved by alexcrichton

@aidanhs aidanhs added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 13, 2017
@bors
Copy link
Contributor

bors commented Sep 14, 2017

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

@tamird
Copy link
Contributor Author

tamird commented Sep 14, 2017

Rebased.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 14, 2017

📌 Commit 509982c has been approved by alexcrichton

@shepmaster
Copy link
Member

[00:03:44] tidy error: /checkout/src/test/compile-fail/rmeta_lib.rs:13: line longer than 100 chars

@shepmaster shepmaster 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 Sep 15, 2017
@tamird
Copy link
Contributor Author

tamird commented Sep 15, 2017

@shepmaster did you mean to post that elsewhere? that file didn't change in this PR and I don't see that failure anywhere.

@shepmaster shepmaster 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 Sep 15, 2017
@shepmaster
Copy link
Member

did you mean to post that elsewhere? that file didn't change in this PR and I don't see that failure anywhere.

It was in the CI run; but it appears someone has restarted it. Perhaps it's an unrelated thing that someone has already fixed...

@alexcrichton
Copy link
Member

@bors: rollup

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 16, 2017
{compiler-builtins,libc} shim cleanup

~~Depends on rust-lang/libc#764; opening early for feedback.~~ r? @alexcrichton
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Sep 16, 2017
{compiler-builtins,libc} shim cleanup

~~Depends on rust-lang/libc#764; opening early for feedback.~~ r? @alexcrichton
@tamird
Copy link
Contributor Author

tamird commented Sep 29, 2017 via email

@tamird
Copy link
Contributor Author

tamird commented Oct 1, 2017

@alexcrichton I edited appveyor.yml as you suggested, but how do I trigger single appveyor builds?

@alexcrichton
Copy link
Member

@tamird you'll also need to enable appveyor for the auto branch

@alexcrichton
Copy link
Member

Looking at the logs there, the fact that the build script is running is actually a bug I believe, maybe try setting RUST_LOG=cargo::ops::cargo_rustc::fingerprint=info to see why the build script is being rerun?

@tamird
Copy link
Contributor Author

tamird commented Oct 6, 2017

Thanks for the hint!

INFO:cargo::ops::cargo_rustc::fingerprint: stale: C:\projects\rust\src\rustc\compiler_builtins_shim\build.rs -- missing
INFO:cargo::ops::cargo_rustc::fingerprint: fingerprint error for compiler_builtins v0.0.0 (file:///C:/projects/rust/src/rustc/compiler_builtins_shim): mtime based components have changed: previously Some(FileTime { seconds: 13151797507, nanos: 852703300 }) now None, paths are "C:\\projects\\rust\\build\\x86_64-pc-windows-gnu\\stage1-std\\x86_64-pc-windows-gnu\\release\\build\\compiler_builtins-6a0431bf00803b6f\\output" and "C:\\projects\\rust\\build\\x86_64-pc-windows-gnu\\stage1-std\\x86_64-pc-windows-gnu\\release\\build\\compiler_builtins-6a0431bf00803b6f\\output"
INFO:cargo::ops::cargo_rustc::fingerprint:   cause: mtime based components have changed: previously Some(FileTime { seconds: 13151797507, nanos: 852703300 }) now None, paths are "C:\\projects\\rust\\build\\x86_64-pc-windows-gnu\\stage1-std\\x86_64-pc-windows-gnu\\release\\build\\compiler_builtins-6a0431bf00803b6f\\output" and "C:\\projects\\rust\\build\\x86_64-pc-windows-gnu\\stage1-std\\x86_64-pc-windows-gnu\\release\\build\\compiler_builtins-6a0431bf00803b6f\\output"

Not sure what to make of this, though. mtime is now None? Does that mean the file was removed?

@alexcrichton
Copy link
Member

The bug I think is this line because with the presence of src/rustc/compiler-builtins-shim/build.rs it's say "rerun if that file changed" but that file's being deleted in this PR, so it's saying "rerun if this file that doesn't exist changed" which means it's always being rerun!

It's probably fine to just make build.rs an empty file with a comment as to why.

@tamird
Copy link
Contributor Author

tamird commented Oct 6, 2017

We could just remove that from upstream, right? The docs at http://doc.crates.io/build-script.html state:

Note that if the build script itself (or one of its dependencies) changes, then it's rebuilt and rerun unconditionally, so cargo:rerun-if-changed=build.rs is almost always redundant (unless you want to ignore changes in all other files except for build.rs).

@alexcrichton
Copy link
Member

I commented on the associated PR, but to comment here as well, we want this directive when developing the compiler-builtins crate because only modifications to the build script should rerun the build script.

@tamird
Copy link
Contributor Author

tamird commented Oct 6, 2017

Yup, thanks for your help. Done!

@alexcrichton
Copy link
Member

@bors: r+

Ok hopefully we're good to go now! Sort of a crazy coincidence this ever worked.

@bors
Copy link
Contributor

bors commented Oct 6, 2017

📌 Commit 138d5ad has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 7, 2017

⌛ Testing commit 138d5add2688f2fa5a335c6a1c7ece83d35e82af with merge 62a870877060f4779e5618e5ff8c4aa05fe67667...

@bors
Copy link
Contributor

bors commented Oct 7, 2017

💔 Test failed - status-appveyor

@tamird
Copy link
Contributor Author

tamird commented Oct 7, 2017 via email

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 7, 2017

📌 Commit 7d6c3da has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 7, 2017

⌛ Testing commit 7d6c3da with merge cce9343...

bors added a commit that referenced this pull request Oct 7, 2017
{compiler-builtins,libc} shim cleanup

~~Depends on rust-lang/libc#764; opening early for feedback.~~ r? @alexcrichton
@bors
Copy link
Contributor

bors commented Oct 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing cce9343 to master...

@bors bors merged commit 7d6c3da into rust-lang:master Oct 7, 2017
@tamird tamird deleted the clean-shims branch October 7, 2017 08:22
bors added a commit to rust-lang/cargo that referenced this pull request Oct 10, 2017
cargo_rustc: remove workaround for fixed upstream issue

Fixed in rust-lang/rust#25411. Also, the
removed code is implicated in test failures observed in
rust-lang/rust#44515.

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants