Skip to content

#36680 add warning when compliation cache fails to hard link #36821

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Oct 4, 2016

Conversation

pweaver
Copy link
Contributor

@pweaver pweaver commented Sep 29, 2016

#36821

I am just starting to learn rust. Feedback would be appreciated.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (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.

@michaelwoerister
Copy link
Member

I just saw this now. Sorry for the delay. I'll give you feedback ASAP :)

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

Looks good to me. You're using if let and Result pretty much how I would have used them.

If you fix the whitespace issue in the warning message, I'm happy to approve this.

if !allows_links {
tcx.sess.warn(&format!("Hard linking files in the incremental compilation
cache failed. Copying files instead. Consider moving the cache directory to a
file system which supports hard linking in session dir `{}`"
Copy link
Member

Choose a reason for hiding this comment

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

If you have a string that spans multiple lines, you can place a \ at the end of the line. That will (iirc) cause the parser to ignore the leading whitespace in the next line:

// This...
let s = "123
         456"
// ... is equivalent to this:
let s = "123\n         456"
// while this:
let s = "123\
         456"
// is equivalent to this:
let s = "123456"

Copy link
Member

@michaelwoerister michaelwoerister Oct 3, 2016

Choose a reason for hiding this comment

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

The above call would then become the following:

tcx.sess.warn(&format!("Hard linking files in the incremental \
                        compilation cache failed. Copying files \
                        instead. Consider moving the cache \
                        directory to a file system which supports \
                        hard linking in session dir `{}`"

tcx.sess.warn(&format!("Hard linking files in the incremental compilation
cache failed. Copying files instead. Consider moving the cache directory to a
file system which supports hard linking in session dir `{}`"
, session_dir.display())
Copy link
Member

Choose a reason for hiding this comment

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

It would be more idiomatic to place the comma at the end of previous line.

@pweaver
Copy link
Contributor Author

pweaver commented Oct 3, 2016

Thanks, pushed some fixes.

@michaelwoerister
Copy link
Member

@bors r+

Thanks for the PR!

@bors
Copy link
Collaborator

bors commented Oct 3, 2016

📌 Commit 7cf90d0 has been approved by michaelwoerister

@bors
Copy link
Collaborator

bors commented Oct 3, 2016

⌛ Testing commit 7cf90d0 with merge 9c31d76...

bors added a commit that referenced this pull request Oct 3, 2016
 #36821

I am just starting to learn rust. Feedback would be appreciated.
@bors bors merged commit 7cf90d0 into rust-lang:master Oct 4, 2016
@michaelwoerister
Copy link
Member

🎉

@bytesnail
Copy link

bytesnail commented Oct 10, 2021

Some warnings are better, but it still seems weird, I tried rust today, rustup installation and incremental compilation have encountered this problem, why the compiler is strongly dependent on specific file system feature, there are no relevant prompts before compiler installation, which is very strange.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants