x setup generates config.toml into root even when run outside of root.

[pnkfelix]

One of our supported use cases is for when people want to put their build tree somewhere other than the root of the rust checkout.

I tried this code:

% git clone git@github.com:rust-lang/rust.git rust
[...]
% mkdir rust-build
% cd rust-build
% ../rust/x setup
[...]
Welcome to the Rust project! What do you want to do with x.py?
a) library: Contribute to the standard library
b) compiler: Contribute to the compiler itself
c) codegen: Contribute to the compiler, and also modify LLVM or codegen
d) tools: Contribute to tools which depend on the compiler, but do not modify it directly (e.g. rustdoc, clippy, miri)
e) user: Install Rust from source
Please choose one (a/b/c/d/e): b
[...]
Would you like to install the git hook?: [y/N]
Ok, skipping installation!

To get started, try one of the following commands:
- `x.py check`
- `x.py build`
- `x.py test`
For more suggestions, see https://rustc-dev-guide.rust-lang.org/building/suggested.html
Build completed successfully in 0:01:05

I expected to see this happen: A config.toml generated in the local directory that reflects the profile I selected.

Instead, this happened: x.py setup generated a config.toml into the root of the rust.git checkout. As a user, I have no indication that I can locally copy that config.toml into my current working directory in order to impose settings that are local to this build in this working directory.

(also, it was a little surprising to me, though understandable, that I was able to subsequently do a build in this local directory, even though the config.toml was sitting in the root of the source tree instead. We may want to issue a note at the beginning of the x.py output when it does that, i.e. something saying "local config.toml not found; using instead confg file found at ")

((i mistakenly thought that the situation was worse in that the script would even overwrite pre-existing config.toml files, but I no longer believe that to be the case; I have now seen useful errors when you try to setup a new config file but one already exists at the target location.))

[jyn514]

This happens because of a weird combination of features:

1. x setup uses the selected config path:

   rust/src/bootstrap/setup.rs

   Line 85 in 452cf4f

   let path = &config.config;

2. bootstrap falls back to {src_root}/config.toml if there's no config.toml in the current directory, even if that file doesn't exist:

   rust/src/bootstrap/config.rs

   Lines 915 to 923 in 452cf4f

   	// Read from `--config`, then `RUST_BOOTSTRAP_CONFIG`, then `./config.toml`, then `config.toml` in the root directory.
   	let toml_path = flags
   	.config
   	.clone()
   	.or_else(|| env::var_os("RUST_BOOTSTRAP_CONFIG").map(PathBuf::from));
   	let using_default_path = toml_path.is_none();
   	let mut toml_path = toml_path.unwrap_or_else(|| PathBuf::from("config.toml"));
   	if using_default_path && !toml_path.exists() {
   	toml_path = config.src.join(toml_path);

3. bootstrap doesn't distinguish "found a default file that exists" from "using the global defaults even though no file exists":

   rust/src/bootstrap/config.rs

   Lines 82 to 83 in 452cf4f

   	/// defaults to `config.toml`
   	pub config: PathBuf,

I think 3 is the bug. The fix is to turn that from config: PathBuf to config: Option<PathBuf>, and then setup.rs can use config.unwrap_or("config.toml") to make sure it treats the current working directory as the default, not the source directory.

Note that currently we determine this by checking config.exists():

rust/src/bootstrap/bin/main.rs

Lines 36 to 38 in 452cf4f

	// NOTE: Since `./configure` generates a `config.toml`, distro maintainers will see the
	// changelog warning, not the `x.py setup` message.
	let suggest_setup = !config.config.exists() && !matches!(config.cmd, Subcommand::Setup { .. });

I think that's a mistake, since it's easy to forget the check; after changing to use an Option, that check should be config.is_none() instead.

[ted-tanner]

This seems like an easy first issue (@jyn514 has all but fixed the issue already). May I be assigned to this one?

[jyn514]

@ted-tanner go for it :)

[ted-tanner]

@jyn514, these lines seem to indicate to me that {src_root} is chosen intentionally as the default location for the config.toml (toml_path becomes the path of config.toml):

rust/src/bootstrap/config.rs

Lines 922 to 924 in 452cf4f

	if using_default_path && !toml_path.exists() {
	toml_path = config.src.join(toml_path);
	}

These lines appeared in commit 4d56f09, with the following commit message: "Fallback to top-level config.toml if not present in current directory." By "top-level config.toml," the commit message probably means {src_root}. I could remove lines 922-924 from config.rs and the config.toml will be placed in the current working directory. However, I am hesitant to do that because the current behavior seems intentional.

@pnkfelix suggested simply displaying a message saying that config.toml is being placed in X directory (which would be {src_root}). That is the best option imo. If that sounds to you like the right thing to do, I would simply check if the current directory is different from config.config in setup.rs and display a message is if it is. What are your thoughts?

[jyn514]

@ted-tanner it's intentional that we load the config.toml from there. but if it's not present at that path, it's not intentional that we create a config.toml there.

(I'm the author of the commit you linked.)

[ted-tanner]

@jyn514 Thanks for the clarification!