Skip to content

Commit

Permalink
Rollup merge of rust-lang#94848 - GuillaumeGomez:browser-ui-test-vers…
Browse files Browse the repository at this point in the history
…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`
  • Loading branch information
Dylan-DPC authored Mar 18, 2022
2 parents a62cf66 + c8158e9 commit de6c170
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 19 deletions.
58 changes: 41 additions & 17 deletions src/bootstrap/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -836,22 +836,39 @@ impl Step for RustdocJSNotStd {
}
}

fn check_if_browser_ui_test_is_installed_global(npm: &Path, global: bool) -> bool {
fn get_browser_ui_test_version_inner(npm: &Path, global: bool) -> Option<String> {
let mut command = Command::new(&npm);
command.arg("list").arg("--depth=0");
command.arg("list").arg("--parseable").arg("--long").arg("--depth=0");
if global {
command.arg("--global");
}
let lines = command
.output()
.map(|output| String::from_utf8_lossy(&output.stdout).into_owned())
.unwrap_or(String::new());
lines.contains(&" browser-ui-test@")
lines.lines().find_map(|l| l.split(":browser-ui-test@").skip(1).next()).map(|v| v.to_owned())
}

fn check_if_browser_ui_test_is_installed(npm: &Path) -> bool {
check_if_browser_ui_test_is_installed_global(npm, false)
|| check_if_browser_ui_test_is_installed_global(npm, true)
fn get_browser_ui_test_version(npm: &Path) -> Option<String> {
get_browser_ui_test_version_inner(npm, false)
.or_else(|| get_browser_ui_test_version_inner(npm, true))
}

fn compare_browser_ui_test_version(installed_version: &str, src: &Path) {
match fs::read_to_string(
src.join("src/ci/docker/host-x86_64/x86_64-gnu-tools/browser-ui-test.version"),
) {
Ok(v) => {
if v.trim() != installed_version {
eprintln!(
"⚠️ Installed version of browser-ui-test (`{}`) is different than the \
one used in the CI (`{}`)",
installed_version, v
);
}
}
Err(e) => eprintln!("Couldn't find the CI browser-ui-test version: {:?}", e),
}
}

#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
Expand All @@ -874,7 +891,7 @@ impl Step for RustdocGUI {
.config
.npm
.as_ref()
.map(|p| check_if_browser_ui_test_is_installed(p))
.map(|p| get_browser_ui_test_version(p).is_some())
.unwrap_or(false)
}))
}
Expand All @@ -892,16 +909,23 @@ impl Step for RustdocGUI {

// The goal here is to check if the necessary packages are installed, and if not, we
// panic.
if !check_if_browser_ui_test_is_installed(&npm) {
eprintln!(
"error: rustdoc-gui test suite cannot be run because npm `browser-ui-test` \
dependency is missing",
);
eprintln!(
"If you want to install the `{0}` dependency, run `npm install {0}`",
"browser-ui-test",
);
panic!("Cannot run rustdoc-gui tests");
match get_browser_ui_test_version(&npm) {
Some(version) => {
// We also check the version currently used in CI and emit a warning if it's not the
// same one.
compare_browser_ui_test_version(&version, &builder.build.src);
}
None => {
eprintln!(
"error: rustdoc-gui test suite cannot be run because npm `browser-ui-test` \
dependency is missing",
);
eprintln!(
"If you want to install the `{0}` dependency, run `npm install {0}`",
"browser-ui-test",
);
panic!("Cannot run rustdoc-gui tests");
}
}

let out_dir = builder.test_out(self.target).join("rustdoc-gui");
Expand Down
10 changes: 8 additions & 2 deletions src/ci/docker/host-x86_64/x86_64-gnu-tools/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,20 @@ RUN /scripts/cmake.sh
COPY host-x86_64/x86_64-gnu-tools/checktools.sh /tmp/

RUN curl -sL https://nodejs.org/dist/v14.4.0/node-v14.4.0-linux-x64.tar.xz | tar -xJ
ENV PATH="/node-v14.4.0-linux-x64/bin:${PATH}"
ENV NODE_FOLDER=/node-v14.4.0-linux-x64/bin
ENV PATH="$NODE_FOLDER:${PATH}"

COPY host-x86_64/x86_64-gnu-tools/browser-ui-test.version /tmp/

# For now, we need to use `--unsafe-perm=true` to go around an issue when npm tries
# to create a new folder. For reference:
# https://github.com/puppeteer/puppeteer/issues/375
#
# We also specify the version in case we need to update it to go around cache limitations.
RUN npm install -g browser-ui-test@0.8.3 --unsafe-perm=true
#
# The `browser-ui-test.version` file is also used by bootstrap to emit warnings in case
# the local version of the package is different than the one used by the CI.
RUN npm install -g browser-ui-test@$(head -n 1 /tmp/browser-ui-test.version) --unsafe-perm=true

ENV RUST_CONFIGURE_ARGS \
--build=x86_64-unknown-linux-gnu \
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
0.8.3
1 change: 1 addition & 0 deletions src/ci/scripts/should-skip-this.sh
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ if [[ -n "${CI_ONLY_WHEN_SUBMODULES_CHANGED-}" ]]; then
src/test/rustdoc-gui \
src/librustdoc \
src/ci/docker/host-x86_64/x86_64-gnu-tools/Dockerfile \
src/ci/docker/host-x86_64/x86_64-gnu-tools/browser-ui-test.version \
src/tools/rustdoc-gui); then
# There was a change in either rustdoc or in its GUI tests.
echo "Rustdoc was updated"
Expand Down

0 comments on commit de6c170

Please sign in to comment.