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.

Sign up for GitHub

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

bootstrap: make cmake executable configurable with config.toml #85728

Closed
wants to merge 1 commit into from
+47 −13
Closed

bootstrap: make cmake executable configurable with config.toml #85728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions config.toml.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,9 @@ changelog-seen = 2
# Defaults to the Python interpreter used to execute x.py
#python = "python"

# Path to (or name of) the CMake 3 executable to build LLVM.
#cmake = "cmake"

# Force Cargo to check that Cargo.lock describes the precise dependency
# set that all the Cargo.toml files create, instead of updating it.
#locked-deps = false
Expand Down
3 changes: 3 additions & 0 deletions src/bootstrap/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ pub struct Config {
pub npm: Option<PathBuf>,
pub gdb: Option<PathBuf>,
pub python: Option<PathBuf>,
pub cmake: Option<PathBuf>,
pub cargo_native_static: bool,
pub configure_args: Vec<String>,

Expand Down Expand Up @@ -474,6 +475,7 @@ define_config! {
nodejs: Option<String> = "nodejs",
npm: Option<String> = "npm",
python: Option<String> = "python",
cmake: Option<String> = "cmake",
locked_deps: Option<bool> = "locked-deps",
vendor: Option<bool> = "vendor",
full_bootstrap: Option<bool> = "full-bootstrap",
Expand Down Expand Up @@ -793,6 +795,7 @@ impl Config {
config.npm = build.npm.map(PathBuf::from);
config.gdb = build.gdb.map(PathBuf::from);
config.python = build.python.map(PathBuf::from);
config.cmake = build.cmake.map(PathBuf::from);
nodakai marked this conversation as resolved.
Show resolved Hide resolved
config.submodules = build.submodules;
set(&mut config.low_priority, build.low_priority);
set(&mut config.compiler_docs, build.compiler_docs);
Expand Down
3 changes: 3 additions & 0 deletions src/bootstrap/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,9 @@ impl Build {
unsafe {
job::setup(self);
}
if let Some(cmake) = self.config.cmake.as_ref() {
env::set_var("CMAKE", cmake); // https://docs.rs/cmake/0.1.48/src/cmake/lib.rs.html#515
}

self.maybe_update_submodules();

Expand Down
51 changes: 38 additions & 13 deletions src/bootstrap/sanity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,31 @@ pub fn check(build: &mut Build) {
.unwrap_or(true)
})
.any(|build_llvm_ourselves| build_llvm_ourselves);
let need_cmake = building_llvm || build.config.any_sanitizers_enabled();
if need_cmake {
cmd_finder.must_have("cmake");
}
let need_default_cmake = if building_llvm || build.config.any_sanitizers_enabled() {
let cmake_env_var = env::var_os("CMAKE");
if let Some(explicit_name) = build.config.cmake.take() {
if let Some(cmake_from_env) = cmake_env_var {
if explicit_name != cmake_from_env {
eprintln!(
"env var CMAKE = {cmake_from_env:?} != {explicit_name:?} from config.toml"
)
}
}
build.config.cmake = cmd_finder.must_have(explicit_name).into();
None
} else {
// _very_ simplified version of getenv_target_os("CMAKE") from
// https://docs.rs/cmake/0.1.48/src/cmake/lib.rs.html#515
if let Some(cmake_from_env) = cmake_env_var {
cmd_finder.must_have(cmake_from_env)
} else {
cmd_finder.must_have("cmake")
}
.into()
}
} else {
None
};
Copy link
Member

Choose a reason for hiding this comment

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

Most of this logic probably belongs in config.rs, not here. We should always have build.config.cmake as the source of truth for the cmake path, and support setting it only via config.toml. (If it is set in the environment and not equal to the config.toml value, it should be a panic! at config parse time).

Here, we should just have the if need_cmake { cmd_finder.must_have(&build.config.cmake); }, with that defaulting to just "cmake" if nothing is set in the config.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a couple of comments, let me start with minor ones (since major ones are going to take more time to write)

  1. (If it is set in the environment and not equal to the config.toml value, it should be a panic! at config parse time)

    Last time, you wrote
    bootstrap: make cmake executable configurable with config.toml #85728 (comment)

    If it is set and different, I think we should probably prefer config.toml still but issue a warning (just eprintln! is fine).

    Warn or panic? Let me know of your decision.

  2. We should always have build.config.cmake as the source of truth for the cmake path, and support setting it only via config.toml.

    Are you suggesting to set CMAKE env var unconditionally based on what's in config.toml (or its default value defined in config.rs > define_config!)? Is that your opinion? You don't mind breaking a hypothetical existing build env which relies on a custom CMAKE env var (which is passed through to cmake crate as of today)? Let's say someone has CMAKE=mycmake as of today, expecting it to be passed through to cmake crate, and your suggestion is that they should set cmake = "mycmake" instead, and if they don't, their build should fail with a panic saying sth like "cmake from config.toml wants to use cmake but you have a contradictory env var CMAKE=mycmake`"?

    Caveat: from what I see, cmake crate accepts a very wide variety of env vars,
    https://docs.rs/cmake/0.1.48/src/cmake/lib.rs.html#857-861
    like CMAKE_x86_64-unknown-linux-gnu, and I don't think it's realistic for bootstrap to cover all of them.

I'll find time to add more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mark-Simulacrum

Copy link
Member

Choose a reason for hiding this comment

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

Warn or panic? Let me know of your decision.

I think panic! is probably best.

[...] cmake env

Looking into our use of cmake a little more, it looks like we only ever actually invoke it through the cmake crate, so if users are supplying configuration to us via env variables, the cmake crate would already be picking that up appropriately. In sanity checking, we do use "cmake" without doing the same env-variable lookup that the cmake crate does. (Target-specific cmake binaries do not feel particularly likely to be actually necessary, as it's really a host tool).

I'm not too worried about env-variables overriding bootstrap's configuration in the cmake crate - that seems unlikely to be an issue in practice, given how bootstrap itself isn't using cmake directly (except for sanity checks). Given that CMAKE isn't a "standard" env variable (or at least I've seen no evidence of that), I'm not really worried about supporting setting it as an alternative to editing config.toml, either.

So I think the value of config.cmake probably comes down to:

  • config.toml, if set
  • cmake, if not

After we load that, we want to put it in our own environment (set_var("CMAKE")) so that the cmake crate picks it up, at least as things stand today. Just before doing that I think it makes sense to check that it's not set to a different value (mostly just as a sanity check, but it doesn't make sense for this to be in sanity.rs).

Pretty much all of that logic should go into config.rs.


build.config.python = build
.config
Expand Down Expand Up @@ -201,14 +222,17 @@ pub fn check(build: &mut Build) {
}
}

if need_cmake && target.contains("msvc") {
// There are three builds of cmake on windows: MSVC, MinGW, and
// Cygwin. The Cygwin build does not have generators for Visual
// Studio, so detect that here and error.
let out = output(Command::new("cmake").arg("--help"));
if !out.contains("Visual Studio") {
panic!(
"
if target.contains("msvc") {
if let Some(ref cmake_path) =
need_default_cmake.as_ref().or(build.config.cmake.as_ref())
{
// There are three builds of cmake on windows: MSVC, MinGW, and
// Cygwin. The Cygwin build does not have generators for Visual
// Studio, so detect that here and error.
let out = output(Command::new(cmake_path).arg("--help"));
if !out.contains("Visual Studio") {
panic!(
"
cmake does not support Visual Studio generators.
This is likely due to it being an msys/cygwin build of cmake,
Expand All @@ -220,7 +244,8 @@ package instead of cmake:
$ pacman -R cmake && pacman -S mingw-w64-x86_64-cmake
"
);
);
}
}
}
}
Expand Down