Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: Upgrade non-dist Linux testers from ubuntu:16.04 to 22.04 #100606

Merged
merged 6 commits into from
Sep 1, 2022

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Aug 16, 2022

The main goal of updating to 22.04 is to get away from llvm.allow-old-toolchain.
A side benefit is that they can also use the system cmake instead of building one.

@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Aug 16, 2022
@rust-highfive
Copy link
Collaborator

r? @pietroalbini

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2022
@rust-log-analyzer

This comment has been minimized.

@cuviper
Copy link
Member Author

cuviper commented Aug 16, 2022

I don't know how rustdoc-gui tests work, but this seems confused:

⚠️ Installed version of browser-ui-test (`0.9.7:undefined`) is different than the one used in the CI (`0.9.7`)
You can install this version using `npm update browser-ui-test` or by using `npm install browser-ui-test@0.9.7`

@Mark-Simulacrum
Copy link
Member

I think I've seen that locally too. You can probably adjust the code in rustbuild(?) to just be OK with a :undefined suffix.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Aug 16, 2022
@cuviper
Copy link
Member Author

cuviper commented Aug 16, 2022

Ok, that looks like an additional field in the npm output, though always "undefined":

# npm list --parseable --long --depth=0 --global
/node-v14.4.0-linux-x64/lib::undefined
/node-v14.4.0-linux-x64/lib/node_modules/browser-ui-test:browser-ui-test@0.9.7:undefined
/node-v14.4.0-linux-x64/lib/node_modules/npm:npm@6.14.5:undefined

I've attempted to fix that up by splitting on just ':' first.

@cuviper
Copy link
Member Author

cuviper commented Aug 16, 2022

Here's the next failure:

Running 68 rustdoc-gui (16 concurrently) ...
FFERROR: puppeteer failed when trying to create a new page. Please try again with `--no-sandbox`

`browser-ui-test` crashed unexpectedly. Please try again with adding `--test-args --no-sandbox` at the end. For example: `x.py test src/test/rustdoc-gui --test-args --no-sandbox`

I can certainly add that --no-sandbox, if someone will second the suggestion as being reasonable in CI...

@rust-log-analyzer

This comment has been minimized.

@Urgau
Copy link
Member

Urgau commented Aug 16, 2022

Here's the next failure:

Running 68 rustdoc-gui (16 concurrently) ...
FFERROR: puppeteer failed when trying to create a new page. Please try again with `--no-sandbox`

`browser-ui-test` crashed unexpectedly. Please try again with adding `--test-args --no-sandbox` at the end. For example: `x.py test src/test/rustdoc-gui --test-args --no-sandbox`

I can certainly add that --no-sandbox, if someone will second the suggestion as being reasonable in CI...

cc @GuillaumeGomez (as maintainer of browser-ui-test in the rust repo)

@GuillaumeGomez
Copy link
Member

You need to add --test-args --no-sandbox where browser-ui-test is called (so in docker/host-x86_64/x86_64-gnu-tools/Dockerfile where we call test/rustdoc-gui).

@cuviper
Copy link
Member Author

cuviper commented Aug 17, 2022

Alright, thanks, I just wanted to make sure that was okay since we didn't need it before. I've added it now.

@GuillaumeGomez
Copy link
Member

That is a new error. Sometimes the page crashes apparently O.o

@bors
Copy link
Contributor

bors commented Aug 21, 2022

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

@cuviper
Copy link
Member Author

cuviper commented Aug 23, 2022

Page crashed again. Would it make sense to try with newer Node.js? Looks like the current LTS is 16.17.0.

@GuillaumeGomez
Copy link
Member

Updating nodejs is a good idea. Not sure if it will solve the issue though since it seems that chromium is the issue. You can try limiting the number of parallel jobs with --jobs (as you can see here).

@cuviper
Copy link
Member Author

cuviper commented Aug 23, 2022

OK, I see puppeteer follows the maintenance LTS, which is still v14, so I only updated that to node 14.20.0 for now. But adding rustdoc-gui --jobs 1 worked locally for me to avoid the page crashes, and there aren't so many to worry about time IMO.

Longer term: AIUI the chromium version is set by puppeteer -- you might want to update that in browser-ui-test?

npm WARN deprecated puppeteer@3.3.0: Version no longer supported. Upgrade to @latest

@GuillaumeGomez
Copy link
Member

The reason why I didn't update the puppeteer version is because for some unknown reason, starting the next version, you can't click on pseudo elements (so div::after for example). But I think I'll need to update it at some point because this version is just too old.

@cuviper
Copy link
Member Author

cuviper commented Aug 23, 2022

@pietroalbini this should be working and ready for review now.

@cuviper
Copy link
Member Author

cuviper commented Aug 30, 2022

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Sep 1, 2022

📌 Commit b96cde7 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2022
@bors
Copy link
Contributor

bors commented Sep 1, 2022

⌛ Testing commit b96cde7 with merge fb88811...

@bors
Copy link
Contributor

bors commented Sep 1, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 1, 2022
@bors bors merged commit fb88811 into rust-lang:master Sep 1, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 1, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fb88811): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
7.2% [7.2%, 7.2%] 1
Improvements ✅
(primary)
-4.7% [-4.7%, -4.7%] 1
Improvements ✅
(secondary)
-2.5% [-2.6%, -2.5%] 2
All ❌✅ (primary) -4.7% [-4.7%, -4.7%] 1

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.

mean1 range count2
Regressions ❌
(primary)
2.5% [2.0%, 3.1%] 4
Regressions ❌
(secondary)
3.3% [3.3%, 3.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.5% [2.0%, 3.1%] 4

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@cuviper cuviper deleted the upgrade-linux-ci branch October 15, 2022 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants