rust icon indicating copy to clipboard operation
rust copied to clipboard

More work on `zstd` compression

Open lqd opened this issue 1 year ago • 58 comments

r? @Kobzol as we've discussed this.

This is a draft to show the current approach of supporting zstd in compiletest, and making the tests using it unconditional.

Knowing whether llvm/lld was built with LLVM_ENABLE_ZSTD is quite hard, so there are two strategies. There are details in the code, and we can discuss this approach. Until we know the config used to build CI artifacts, it seems our options are somewhat limited in any case.

zlib compression seems always enabled, so we only check this in its dedicated test, allowing the test to ignore errors due to zstd not being supported.

The zstd test is made unconditional in what it tests, by relying on needs-llvm-zstd to be ignored when llvm.libzstd isn't enabled in config.toml.

try-job: x86_64-gnu try-job: x86_64-msvc try-job: x86_64-gnu-distcheck

lqd avatar Aug 10 '24 17:08 lqd

My understanding is that test builders use download-ci-llvm, which has llvm.libzstd enabled by default on x64 linux.

The x86_64-gnu builder already has libzstd-dev installed but it probably shouldn't be needed if it indeed uses CI artifacts (we can check that later).

That builder should now have needs-llvm-libzstd tests enabled even without enabling the config. Let's check that.

@bors try

lqd avatar Aug 10 '24 17:08 lqd

(Let me know what tests and try builds we'd want to double-check.)

lqd avatar Aug 10 '24 17:08 lqd

:hourglass: Trying commit f15a53921c1ff96d9298df90d4da969379c33d61 with merge 90f11bc6d7b9457dfc29afa8e43838143aea08a7...

bors avatar Aug 10 '24 17:08 bors

:sunny: Try build successful - checks-actions Build commit: 90f11bc6d7b9457dfc29afa8e43838143aea08a7 (90f11bc6d7b9457dfc29afa8e43838143aea08a7)

bors avatar Aug 10 '24 19:08 bors

zlib

test [run-make] tests/run-make/compressed-debuginfo ... ok

zstd

test [run-make] tests/run-make/compressed-debuginfo-zstd ... ok


I've removed the dependency from the test runner image, let's see what happens now. @bors try

lqd avatar Aug 10 '24 22:08 lqd

:hourglass: Trying commit 72139a914b537d99ccfed378f0e4dbf28119a2ee with merge ae6b0ab9ec955e1a5a3ac68b5f00b27ca4461a1b...

bors avatar Aug 10 '24 22:08 bors

:sunny: Try build successful - checks-actions Build commit: ae6b0ab9ec955e1a5a3ac68b5f00b27ca4461a1b (ae6b0ab9ec955e1a5a3ac68b5f00b27ca4461a1b)

bors avatar Aug 11 '24 00:08 bors

Cool, the two tests (that now fail when a compression algorithm isn't found) still pass

test [run-make] tests/run-make/compressed-debuginfo ... ok test [run-make] tests/run-make/compressed-debuginfo-zstd ... ok

lqd avatar Aug 11 '24 00:08 lqd

Could you try to make a fake LLVM change to see what happens?

I pushed changes to rustc_llvm as it's easier than messing with the llvm submodule.

Let's see what happens: @bors try

lqd avatar Aug 11 '24 14:08 lqd

:hourglass: Trying commit 8f595ceb82da4416e26ca3a14c5dca928b724a81 with merge c8f5f2662e7d003f1ae4fa7c8959df6fdf09a57e...

bors avatar Aug 11 '24 14:08 bors

Looks like it downloaded LLVM anyway :laughing: rustc_llvm isn't really cached on CI I think, so probably you'll actually need to modify LLVM.

Kobzol avatar Aug 11 '24 14:08 Kobzol

I sure hope switching to another repo won't download LLVM again!

@bors try

lqd avatar Aug 11 '24 15:08 lqd

:hourglass: Trying commit bfecb35c960e29085fa8b677ffbd633d8ade1b3f with merge 18b5811b2263545c3d2d888a132c80b66715edf2...

bors avatar Aug 11 '24 15:08 bors

Alright I pushed a different commit to the llvm fork, hopefully the builder will pick it now ....

@bors try

lqd avatar Aug 11 '24 16:08 lqd

:hourglass: Trying commit 7427683817911926f64cca9161491126276b8771 with merge 66e31b850181539d68f3abdee30564a7f303c0f7...

bors avatar Aug 11 '24 16:08 bors

:sunny: Try build successful - checks-actions Build commit: 66e31b850181539d68f3abdee30564a7f303c0f7 (66e31b850181539d68f3abdee30564a7f303c0f7)

bors avatar Aug 11 '24 18:08 bors

CI was green, but the test was skipped (as expected) :/ This is a bit annoying, we'd need to keep the dist and test builders in sync.. But probably it's the most straightforward way for now (enable llvm.libzstd and install the zstd package on the test runner).

Kobzol avatar Aug 11 '24 18:08 Kobzol

Sure we can install the package on the test builder and enable zstd, but still manually build a specific version on the dist builder? Or should we build-ztsd.sh on both?

lqd avatar Aug 11 '24 18:08 lqd

Let's install the package on the test builder, we'll at least get more coverage out of that (test that it also works with a repository-provided zstd).

Kobzol avatar Aug 11 '24 18:08 Kobzol

This should be the last try to see the tests pass 🤞

@bors try

lqd avatar Aug 11 '24 19:08 lqd

:hourglass: Trying commit 749b3f690259b77367d1d76075eb49c66ee17817 with merge 875b09f276741802ee5135283229e71eb7caecc3...

bors avatar Aug 11 '24 19:08 bors

:sunny: Try build successful - checks-actions Build commit: 875b09f276741802ee5135283229e71eb7caecc3 (875b09f276741802ee5135283229e71eb7caecc3)

bors avatar Aug 11 '24 21:08 bors

Both passed.

test [run-make] tests/run-make/compressed-debuginfo ... ok test [run-make] tests/run-make/compressed-debuginfo-zstd ... ok

lqd avatar Aug 11 '24 23:08 lqd

Awesome! Thanks, feel free to r=me.

Kobzol avatar Aug 12 '24 06:08 Kobzol

This PR modifies tests/run-make/. If this PR is trying to port a Makefile run-make test to use rmake.rs, please update the run-make port tracking issue so we can track our progress. You can either modify the tracking issue directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

Some changes occurred in src/tools/compiletest

cc @jieyouxu

rustbot avatar Aug 12 '24 12:08 rustbot

alright let's see what bors has to say

@bors r=Kobzol

lqd avatar Aug 12 '24 12:08 lqd

:pushpin: Commit a566252ac92184a5443245390da794eaddcbef1b has been approved by Kobzol

It is now in the queue for this repository.

bors avatar Aug 12 '24 12:08 bors

Ah I was wondering what happened to this PR 😅. I rebased and fixed the silent conflicts on the tests.

@bors r=Kobzol

lqd avatar Aug 14 '24 14:08 lqd

:pushpin: Commit f27f3a3345e302db7229cfbc23b6a3c8f175c9c8 has been approved by Kobzol

It is now in the queue for this repository.

bors avatar Aug 14 '24 14:08 bors

This PR failed in https://github.com/rust-lang/rust/pull/129126.

@bors r-

GuillaumeGomez avatar Aug 15 '24 16:08 GuillaumeGomez