Add more context to quit_if_file_exists in configure.py & delete config.toml in CI

[tgross35]

If the obj directory isn't empty, the error message is subtle and not very helpful:

== clock drift check ==
  local time: Sun Jul  2 00:57:06 UTC 2023
  network time: Sun, 02 Jul 2023 00:57:06 GMT
== end clock drift check ==
sccache: Starting the server...
configure: error: Existing 'config.toml' detected.
== clock drift check ==
  local time: Sun Jul  2 00:57:06 UTC 2023
  network time: Sun, 02 Jul 2023 00:57:06 GMT
== end clock drift check ==

This makes it stand out and suggests how to resolve the issue:

== clock drift check ==
  local time: Sun Jul  2 02:11:30 UTC 2023
  network time: Sun, 02 Jul 2023 02:11:31 GMT
== end clock drift check ==
sccache: Starting the server...

configure: ERROR: Existing 'config.toml' detected. Exiting
Is objdir '/home/tmgross/projects/rust/obj' clean?

== clock drift check ==
  local time: Sun Jul  2 02:11:31 UTC 2023
  network time: Sun, 02 Jul 2023 02:11:31 GMT
== end clock drift check ==

[rustbot]

r? @Mark-Simulacrum

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

[the8472]

The guide stops at building the image, but doesn't say anything about how to run it.

That's not correct, the ./src/ci/docker/run.sh x86_64-gnu command in the previous section will run the build/test job in docker.

[tgross35]

Hm, it does not for me...

$ ./src/ci/docker/run.sh x86_64-gnu
Attempting with retry: docker build --rm -t rust-ci -f ...
[+] Building 139.4s (9/9) FINISHED                                                                             
...                                                                  0.0s
 => [internal] load metadata for docker.io/library/ubuntu:20.04                                           1.1s
 => [1/4] FROM docker.io/library/ubuntu:20.04@sha256:f8f658407c35733471596f25fdb4ed748b80e545ab57e84efbd  3.3s
...
 => [2/4] RUN apt-get update && apt-get install -y --no-install-recommends   g++   make   ninja-build   124.7s
 => [3/4] COPY scripts/sccache.sh /scripts/                                                               0.0s
 => [4/4] RUN sh /scripts/sccache.sh                                                                      3.7s
 => exporting to image                                                                                    6.5s
 => => exporting layers                                                                                   6.5s
 => => writing image sha256:ef3942f9859700ca0d7f35c69e76a2fc73097a63dd7fffc2d6a53fb6c1a3a09c              0.0s
 => => naming to docker.io/library/rust-ci                                                                0.0s
== clock drift check ==
  local time: Sun Jul  2 00:57:06 UTC 2023
  network time: Sun, 02 Jul 2023 00:57:06 GMT
== end clock drift check ==
sccache: Starting the server...
configure: error: Existing 'config.toml' detected.
== clock drift check ==
  local time: Sun Jul  2 00:57:06 UTC 2023
  network time: Sun, 02 Jul 2023 00:57:06 GMT
== end clock drift check ==

And that's it. I'll try to see what's going on with the script, maybe I'm just missing an environment variable or something

[the8472]

configure: error: Existing 'config.toml' detected.

That's the container running but encountering an error.

[tgross35]

I'm not sure what it's talking about

$ fd config.toml
src/tools/error_index_generator/book_config.toml

Trying to chase it down

[tgross35]

It was just a dirty obj directory. The error message is kind of subtle and doesn't provide any context, I'm surprised more people don't hit this issue.

So I adjusted this PR to make the error stand out more and suggest checking that directory. Is that suitable, or would it be better to verify this directory doesn't exist in run.sh?

[tgross35]

@rustbot ready

[bors]

☔ The latest upstream changes (presumably #112779) made this pull request unmergeable. Please resolve the merge conflicts.

[Mark-Simulacrum]

This doesn't seem right to me, it should be fine to re-run a given docker image in the same directory without purging the cache. Maybe the previous error message for an existing config.toml is misleading and (for example) should be dropped if we're going to create the same file?

[tgross35]

Do you or others also run into this error if running twice? It’s a bit weird that this hasn’t been addressed in some way or another, unless there’s something about my setup that makes me hit it for some reason.

I can adjust this to just delete the config file and whatever else is needed to not conflict.

[Mark-Simulacrum]

./src/ci/docker/run.sh mingw-check-tidy run twice for me seems to work without issues - so not sure if that command is special?

What command are you running?

[tgross35]

Same script but with x86_64-gnu, my output is in the comment above #112916 (comment)

[Mark-Simulacrum]

Ah, I see, I think I had an old branch. The error seems new as of #110123 - I'm not sure that it should be an error, as the last comment on that PR suggests -- at minimum, we should silence that error during CI docker builds.

[tgross35]

Hm, yeah that is interesting to see the context. What do you think would be best? I think having a nicer error message is nice at a minimum, but I’ll update this PR to whatever makes the most sense.

cc @jyn514 since you were involved in the PR that Mark linked

[jyn514]

i think we should change the CI image to run rm -f config.toml before running configure. the issue here seems limited to CI and i don't think we should change the general behavior of configure.py.

[jyn514]

(i think my ideal behavior here would be "edit the file in place, keeping any existing settings, and only error if a setting conflicts", but that's too complicated to do in python and we can't move it to rust because it happens too early.)

[Mark-Simulacrum]

I think it's also true that every other configure I've worked with has overwritten the configuration in place with no warning. I'm not convinced humans are often running configure and automation rarely wants to extend previous state I think.

Anyway, I'm fine with updating CI to delete configuration. That seems like a reasonable approach.

[Mark-Simulacrum]

@tgross35 Can you update this PR to remove the file just before this line: https://github.com/rust-lang/rust/blob/master/src/ci/run.sh#L174 ? I think that's the right place for this delta to work best.

[tgross35]

Whoops forgot about this one, yes will do that change soon

[tgross35]

@Mark-Simulacrum this should be all set

@rustbot ready

[Mark-Simulacrum]

@bors r+ rollup=iffy

[bors]

📌 Commit 8a2022b has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8a2022b with merge 2aae331...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 2aae331 to master...

[rust-timer]

Finished benchmarking commit (2aae331): 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.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.6%, 0.6%]	1

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)	-2.5%	[-2.8%, -2.1%]	2
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)	-	-	0
Improvements ✅ (primary)	-0.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-0.9%	[-0.9%, -0.9%]	1

Binary size

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

Bootstrap: 650.942s -> 650.283s (-0.10%)