Override config.toml options from command line

[clubby789]

https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Running.20tests.20on.20precompiled.20rustc/near/357763280

cc @jyn514

[rustbot]

r? @ozkanonur

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

[onur-ozkan]

We should do the overriding in bootstrap::config module without changing the behaviour of the file deserialization. This will not help what's been said in the zulip thread at https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/Running.20tests.20on.20precompiled.20rustc/near/358250357 since this logic needs toml file to be deserialized.

[onur-ozkan]

Looks roughly good, can we have test for this?

[jyn514]

This seems broadly good 👍 good call on slotting this into the existing Merge trait.

I would like to use this new mechanism in ./configure; I think it will both be a lot more maintainable (easier to test, the existing hacky regex in bootstrap.py only has to handle options it's actually using instead of all options) and a good way to verify that your new code is doing the right thing.

[jyn514]

I would like to use this new mechanism in ./configure

I think this shouldn't be too hard to add if you change

rust/src/bootstrap/configure.py

Lines 557 to 559 in 787f3fe

	with bootstrap.output('Makefile') as f:
	contents = os.path.join(rust_dir, 'src', 'bootstrap', 'mk', 'Makefile.in')
	contents = open(contents).read()

to something like

 with bootstrap.output('Makefile') as f: 
     template = os.path.join(rust_dir, 'src', 'bootstrap', 'mk', 'Makefile.in') 
     contents = "BOOTSTRAP_ARGS := " + " ".join(sys.argv[1:]) + "\n"
     contents += open(template).read() 

and then delete all the handling of config.toml except for the defaults.

[clubby789]

@rustbot author

[onur-ozkan]

last review notes from my side, thank you for the previous fixes :)

[onur-ozkan]

This LGTM. Feel free to r=me if your notes were resolved @jyn514

[clubby789]

I haven't yet implemented #111566 (comment), planning to work on that today

[clubby789]

Pushing to test if this is working with CI so far, but please let me know if you have any thoughts on the changes. I've not cleaned it up yet, so there's some unused functions

EDIT: newer, simpler approach

[clubby789]

Ah, @jyn514 scripts like checktools.sh don't know about BOOTSTRAP_ARGS

[jyn514]

ah hmm, I see - running ./configure doesn't necessarily mean that all the bootstrap invocations are going to go through make, we need to support running ./x.py directly too.

That's unfortunate. The only way I can think of to get configure to use the new set option is to allow it in config.toml itself, and embedding toml in toml seems ... weird, to say the least.

I think for now let's leave ./configure as-is and land the improvement to bootstrap itself and I'll think more about whether it makes sense to allow set in config.toml.

[clubby789]

What about setting BOOTSTRAP_ARGS as an env var, and having bootstrap append them to the end of argv?

[jyn514]

I don't see how that will work? How will they get set in the environment of the code that runs x.py?

[clubby789]

Ah yes, my mistake. I'll revert the configure changes

[jyn514]

@bors r=ozkanonur

[bors]

📌 Commit 7a7cbe0 has been approved by ozkanonur

It is now in the queue for this repository.

[bors]

⌛ Testing commit 7a7cbe0 with merge 70db836...

[bors]

☀️ Test successful - checks-actions
Approved by: ozkanonur
Pushing 70db836 to master...

[rust-timer]

Finished benchmarking commit (70db836): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.1%	[4.1%, 4.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 647.352s -> 647.458s (0.02%)