Skip to content

Use depinfo to discover UI test dependencies #11045

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

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

Alexendoo
Copy link
Member

changelog: none

context: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Building.20test.20dependencies

This restores the old EXTERN_FLAGS method of passing --extern flags for building UI tests with minor changes

  • Unused deps were removed
  • It's now a Vec of args instead of a command string
  • It uses a BTreeMap so the extern flags are in alphabetical order and deterministic

I don't know if the HOST_LIBS part is still required, but I figured it best to leave it in for now. If the change is accepted we can take a look if it's needed in rust-lang/rust after the next sync

This isn't as pleasant as having a Cargo.toml, though there is something satisfying about knowing the dependencies are already built and not needing to invoke cargo

r? @flip1995

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 28, 2023
@Alexendoo
Copy link
Member Author

Some numbers, TESTNAME=___ means we build the test deps but don't run any tests

$ hyperfine --warmup 2 \
    --runs 5 \
    --parameter-list commit master,extern-flags \
    --parameter-list package clippy_lints,clippy_utils \
    --setup "git checkout {commit}" \
    --prepare "touch {package}/src/lib.rs" \
    --command-name "{commit}, rebuild {package}" \
    "TESTNAME=___ cargo uitest"

Benchmark 1: master, rebuild clippy_lints
  Time (mean ± σ):     21.468 s ±  0.152 s    [User: 16.322 s, System: 3.989 s]
  Range (min … max):   21.268 s … 21.645 s    5 runs

Benchmark 2: extern-flags, rebuild clippy_lints
  Time (mean ± σ):     11.418 s ±  0.084 s    [User: 8.734 s, System: 2.397 s]
  Range (min … max):   11.309 s … 11.536 s    5 runs

Benchmark 3: master, rebuild clippy_utils
  Time (mean ± σ):     24.386 s ±  0.173 s    [User: 19.307 s, System: 5.706 s]
  Range (min … max):   24.127 s … 24.561 s    5 runs

Benchmark 4: extern-flags, rebuild clippy_utils
  Time (mean ± σ):     12.984 s ±  0.160 s    [User: 10.173 s, System: 3.316 s]
  Range (min … max):   12.791 s … 13.233 s    5 runs

For a completely clean build there's also quite a noticeable difference so this would also reduce CI times a bit. Measuring with

cargo clean; time TESTNAME=___ cargo uitest

On master it took 3:21.04
On extern-flags it took 1:52.02

It also shaves 2.2GB from the target directory

Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already made my opinions clear, but I'm 100% in favor of this. It's a bit unfortunate that we can't use Cargo.toml but it really makes testing a ton more time consuming.

@flip1995
Copy link
Member

Thanks! I will test this also in the rustc repo after the sync, to make sure that it also works out there.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sad, but makes sense. This also fixes rust-lang/rust#113585, so it may be good to do a sync to rustc quickly afterwards

@flip1995
Copy link
Member

Sync is tomorrow, so lets get this in.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 12, 2023

📌 Commit 4340a8a has been approved by flip1995

It is now in the queue for this repository.

@Alexendoo
Copy link
Member Author

re: HOST_LIBS, I tested it in the rust-lang/rust repo and it is still required

@bors
Copy link
Contributor

bors commented Jul 12, 2023

⌛ Testing commit 4340a8a with merge 53d2938...

@bors
Copy link
Contributor

bors commented Jul 12, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 53d2938 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants