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

Set CMAKE_SYSTEM_NAME when cross-compiling #75376

Merged
merged 1 commit into from
Aug 15, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Aug 10, 2020

Configure CMAKE_SYSTEM_NAME when cross-compiling in configure_cmake,
to tell CMake about target system. Previously this was done only for
LLVM step and now applies more generally to steps using cmake.

Helps with #74576.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Aug 10, 2020
@tmiasko tmiasko force-pushed the cmake-system-name branch from 7697599 to d57b923 Compare August 10, 2020 20:37
@tmiasko tmiasko marked this pull request as ready for review August 10, 2020 20:37
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 13, 2020

@Mark-Simulacrum I would appreciate the review of this, to unblock #74576, where incorrect evaluation of CMAKE_SYSTEM_NAME breaks the tsan build. Thanks.

@Mark-Simulacrum
Copy link
Member

https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_NAME.html seems to indicate that we should also be setting CMAKE_SYSTEM_VERSION - is that not the case?

@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 13, 2020

Yes, I would expect that should be done as well.

Fixing this might be non-trivial since rustc targets do not include version.
After running a FreeBSD cross-build on CI there is no indication that it is
necessary and looking at LLVM CMakeFiles checks for system versions are mostly
limited to tests. So I would rather avoid attempts to add version in this PR.

@Mark-Simulacrum
Copy link
Member

Seems reasonable. r=me with a comment added about the version not being set and our belief that not setting it should be fine (basically your last comment inserted into the code).

Configure CMAKE_SYSTEM_NAME when cross-compiling in `configure_cmake`,
to tell CMake about target system. Previously this was done only for
LLVM step and now applies more generally to steps using cmake.
@tmiasko tmiasko force-pushed the cmake-system-name branch from d57b923 to 91f87bc Compare August 13, 2020 12:52
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 13, 2020

Added a comment about CMAKE_SYSTEM_VERSION.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 13, 2020

📌 Commit 91f87bc 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 Aug 13, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2020
Rollup of 4 pull requests

Successful merges:

 - rust-lang#75376 (Set CMAKE_SYSTEM_NAME when cross-compiling)
 - rust-lang#75448 (merge `as_local_hir_id` with `local_def_id_to_hir_id`)
 - rust-lang#75513 (Recover gracefully from `struct` parse errors)
 - rust-lang#75545 (std/sys/unix/time: make it easier for LLVM to optimize `Instant` subtraction.)

Failed merges:

 - rust-lang#75514 (Replaced `log` with `tracing`)

r? @ghost
@bors bors merged commit 29b6b5f into rust-lang:master Aug 15, 2020
@tmiasko tmiasko deleted the cmake-system-name branch August 15, 2020 08:04
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants