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

CMake: Do not print installation messages for up-to-date files #60832

Merged
merged 1 commit into from
May 15, 2019

Conversation

petrochenkov
Copy link
Contributor

Closes #60830

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2019
@Mark-Simulacrum
Copy link
Member

@bors r+ p=1 since this may cause PRs to fail

@bors
Copy link
Contributor

bors commented May 14, 2019

📌 Commit ec2f3e13e035d13985abf2c6f6aee8059b1bf0c2 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-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2019
@alexcrichton
Copy link
Member

I would personally prefer if this were only applied to the location necessary for llvm, all the others should be inconsequential and/or handled already. Could the comment be expanded as well as to why it is applied, not just what it is?

@petrochenkov
Copy link
Contributor Author

@alexcrichton

Could the comment be expanded as well as to why it is applied, not just what it is?

The reason is that CMAKE_INSTALL_MESSAGE=LAZY is almost always what you want, during local development or on CI, doesn't matter.
It's not the default in CMake because CMAKE_INSTALL_MESSAGE is a relatively recent addition (introduced in CMake 3.1 https://cmake.org/pipermail/cmake/2014-December/059418.html).
Perhaps cmake-rs can even set it by default.

The Travis issue with overflowing logs is a trigger for enabling it rather than the reason.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 14, 2019

The option is useless if the install directory is certainly fresh and empty.
I don't know whether that's the case for cmake that's run from src/ci, so I enabled it everywhere.

For cmake that's run from *.rs that's not the case.

@alexcrichton
Copy link
Member

It makes sense yeah that it's a better default, and I'm not disagreeing at all. All our build scripts are already complicated and this is just one more thing to understand whenever reading them, and it's basically easier to read zero code than it is to reason later "did someone add this for correctness?" or having to preserve this through all future changes. I would rather than this be surgical and only apply, with a comment, to where it matters. Changing cmake-rs itself also seems totally fine by me.

@petrochenkov
Copy link
Contributor Author

OK, the messages in the failed job were produced by LLVM and LLD builds, so I reduced the changes to those only.

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 14, 2019

📌 Commit 3646b3c has been approved by Mark-Simulacrum

Centril added a commit to Centril/rust that referenced this pull request May 15, 2019
CMake: Do not print installation messages for up-to-date files

Closes rust-lang#60830
Centril added a commit to Centril/rust that referenced this pull request May 15, 2019
CMake: Do not print installation messages for up-to-date files

Closes rust-lang#60830
@bors
Copy link
Contributor

bors commented May 15, 2019

⌛ Testing commit 3646b3c with merge 7158ed9...

bors added a commit that referenced this pull request May 15, 2019
CMake: Do not print installation messages for up-to-date files

Closes #60830
@bors
Copy link
Contributor

bors commented May 15, 2019

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 15, 2019
@bors bors merged commit 3646b3c into rust-lang:master May 15, 2019
@petrochenkov petrochenkov deleted the CLazy branch June 5, 2019 16:37
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.

Set CMAKE_INSTALL_MESSAGE to LAZY for code built with CMake, LLVM in particular
5 participants