Compare installed browser-ui-test version to the one used in CI #94848

GuillaumeGomez · 2022-03-11T14:33:15Z

I happened a few times to run into (local) rustdoc GUI tests errors because I forgot to update my browser-ui-test version. I know at least two others who encountered the same problem so I think emitting a warning to let us know about this version mismatch would make it easier to figure out.

So now, I'm not too sure that this PR is the right approach because it requires to parse a Dockerfile, which feels pretty bad. I had the idea to instead store the browser-ui-test version into a docker ARG like:

ARG BROWSER_UI_TEST_VERSION=0.8.0

And then use it as such in the command to make the parsing more reliable.

Or we could store this version into a file and import this file into the Dockerfile and read it from the builder.

Any preference or maybe another solution?

r? @Mark-Simulacrum

Mark-Simulacrum · 2022-03-11T15:16:19Z

A file seems OK, maybe in src/test/rustdoc-gui -- though I'm not sure if we have access to that from within Docker, so you'll have to check. If we can't do that then reading a concrete version number from the CI directory (but still in a separate file) is likely fine.

GuillaumeGomez · 2022-03-11T16:57:20Z

Seems like it's working like this. Does it seem acceptable to you?

bors · 2022-03-13T03:06:27Z

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

GuillaumeGomez · 2022-03-13T14:32:10Z

Fixed the conflict.

src/bootstrap/test.rs

Mark-Simulacrum · 2022-03-13T16:08:11Z

Did the file in src/test/rustdoc-gui not work? If not, please put the file in src/ci/docker/host-x86_64/x86_64-gnu-tools, rather than /scripts -- it is not really a global file at least right now and isn't a script.

src/ci/docker/host-x86_64/x86_64-gnu-tools/Dockerfile

GuillaumeGomez · 2022-03-13T19:13:14Z

Did the file in src/test/rustdoc-gui not work? If not, please put the file in src/ci/docker/host-x86_64/x86_64-gnu-tools, rather than /scripts -- it is not really a global file at least right now and isn't a script.

Seems like it's "out of docker scope". I'll move it to the other folder then.

src/test/rustdoc-gui/mobile.goml

GuillaumeGomez · 2022-03-13T22:40:03Z

Perfect, seems like I was able to make it work! \o/ Removing the GUI test changes then.

GuillaumeGomez · 2022-03-13T22:41:23Z

Should be ready now.

GuillaumeGomez · 2022-03-16T14:19:00Z

@Mark-Simulacrum Anything else to be done?

bors · 2022-03-18T08:19:08Z

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

GuillaumeGomez · 2022-03-18T09:51:49Z

I added a second commit to run rustdoc GUI tests when browser-ui-test version is updated too.

Mark-Simulacrum · 2022-03-18T14:59:39Z

@bors r+ rollup

bors · 2022-03-18T14:59:41Z

📌 Commit c8158e9 has been approved by Mark-Simulacrum

…ion, r=Mark-Simulacrum Compare installed browser-ui-test version to the one used in CI I happened a few times to run into (local) rustdoc GUI tests errors because I forgot to update my browser-ui-test version. I know at least two others who encountered the same problem so I think emitting a warning to let us know about this version mismatch would make it easier to figure out. So now, I'm not too sure that this PR is the right approach because it requires to parse a Dockerfile, which feels pretty bad. I had the idea to instead store the browser-ui-test version into a docker ARG like: ```docker ARG BROWSER_UI_TEST_VERSION=0.8.0 ``` And then use it as such in the command to make the parsing more reliable. Or we could store this version into a file and import this file into the Dockerfile and read it from the builder. Any preference or maybe another solution? r? `@Mark-Simulacrum`

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#94115 (Let `try_collect` take advantage of `try_fold` overrides) - rust-lang#94295 (Always evaluate all cfg predicate in all() and any()) - rust-lang#94848 (Compare installed browser-ui-test version to the one used in CI) - rust-lang#94993 (Add test for >65535 hashes in lexing raw string) - rust-lang#95017 (Derive Eq for std::cmp::Ordering, instead of using manual impl.) - rust-lang#95058 (Add use of bool::then in sys/unix/process) - rust-lang#95083 (Document that `Option<extern "abi" fn>` discriminant elision applies for any ABI) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

GuillaumeGomez added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Mar 11, 2022

rust-highfive assigned Mark-Simulacrum Mar 11, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2022

GuillaumeGomez force-pushed the browser-ui-test-version branch from 8f725fc to 93104ac Compare March 11, 2022 15:52

GuillaumeGomez marked this pull request as ready for review March 11, 2022 16:57

GuillaumeGomez force-pushed the browser-ui-test-version branch from 93104ac to f6db6c5 Compare March 13, 2022 13:22

Mark-Simulacrum reviewed Mar 13, 2022

View reviewed changes

src/bootstrap/test.rs Outdated Show resolved Hide resolved

Mark-Simulacrum reviewed Mar 13, 2022

View reviewed changes

src/bootstrap/test.rs Outdated Show resolved Hide resolved

Mark-Simulacrum reviewed Mar 13, 2022

View reviewed changes

src/ci/docker/host-x86_64/x86_64-gnu-tools/Dockerfile Outdated Show resolved Hide resolved

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2022

GuillaumeGomez force-pushed the browser-ui-test-version branch from f6db6c5 to 29d7358 Compare March 13, 2022 19:05