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

More work on zstd compression #128935

Merged
merged 8 commits into from
Aug 27, 2024
Merged

More work on zstd compression #128935

merged 8 commits into from
Aug 27, 2024

Conversation

lqd
Copy link
Member

@lqd lqd commented Aug 10, 2024

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

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 10, 2024
@lqd
Copy link
Member Author

lqd commented Aug 10, 2024

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
Copy link
Member Author

lqd commented Aug 10, 2024

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

@bors
Copy link
Contributor

bors commented Aug 10, 2024

⌛ Trying commit f15a539 with merge 90f11bc...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 10, 2024
More work on `zstd` compression

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
@bors
Copy link
Contributor

bors commented Aug 10, 2024

☀️ Try build successful - checks-actions
Build commit: 90f11bc (90f11bc6d7b9457dfc29afa8e43838143aea08a7)

@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Aug 10, 2024
@lqd
Copy link
Member Author

lqd commented Aug 10, 2024

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

@bors
Copy link
Contributor

bors commented Aug 10, 2024

⌛ Trying commit 72139a9 with merge ae6b0ab...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 10, 2024
More work on `zstd` compression

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
@bors
Copy link
Contributor

bors commented Aug 11, 2024

☀️ Try build successful - checks-actions
Build commit: ae6b0ab (ae6b0ab9ec955e1a5a3ac68b5f00b27ca4461a1b)

@lqd
Copy link
Member Author

lqd commented Aug 11, 2024

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

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

This looks great! However, I'm not sure if we can ignore the LLVM build in test runners everytime. The runner uses llvm.download-ci-llvm=if-unchanged, so when there's a change to LLVM (or rustc_llvm), it will actually rebuild LLVM. In that case, we can either ignore the test (but it's not ideal, as the test would only run in the next merge), or make sure that we actually build LLVM with ZSTD support even in the test runner.

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

@lqd
Copy link
Member Author

lqd commented Aug 11, 2024

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 11, 2024
More work on `zstd` compression

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
@bors
Copy link
Contributor

bors commented Aug 11, 2024

⌛ Trying commit 8f595ce with merge c8f5f26...

@Kobzol
Copy link
Contributor

Kobzol commented Aug 11, 2024

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

@lqd
Copy link
Member Author

lqd commented Aug 11, 2024

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

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 11, 2024
More work on `zstd` compression

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
@bors
Copy link
Contributor

bors commented Aug 11, 2024

⌛ Trying commit bfecb35 with merge 18b5811...

@lqd
Copy link
Member Author

lqd commented Aug 11, 2024

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

@bors try

@bors
Copy link
Contributor

bors commented Aug 11, 2024

⌛ Trying commit 7427683 with merge 66e31b8...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 11, 2024
More work on `zstd` compression

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
@bors
Copy link
Contributor

bors commented Aug 11, 2024

☀️ Try build successful - checks-actions
Build commit: 66e31b8 (66e31b850181539d68f3abdee30564a7f303c0f7)

@tgross35
Copy link
Contributor

@bors r-

The distcheck compressed-debuginfo test failed at #129547 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 25, 2024
@lqd
Copy link
Member Author

lqd commented Aug 25, 2024

It seems distcheck doesn’t even have zlib enabled…

@lqd
Copy link
Member Author

lqd commented Aug 25, 2024

I've removed the commit with the unconditional test change. We'll clean up zlib being randomly enabled later.

@bors try

@bors
Copy link
Contributor

bors commented Aug 25, 2024

⌛ Trying commit 5d83cb2 with merge 9a12bb9...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 25, 2024
More work on `zstd` compression

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
@bors
Copy link
Contributor

bors commented Aug 26, 2024

☀️ Try build successful - checks-actions
Build commit: 9a12bb9 (9a12bb9f8ef3d7437bc991d00c7cbb82fa727d51)

@lqd
Copy link
Member Author

lqd commented Aug 26, 2024

@bors r=Kobzol

@bors
Copy link
Contributor

bors commented Aug 26, 2024

📌 Commit 5d83cb2 has been approved by Kobzol

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 26, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 27, 2024
More work on `zstd` compression

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
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
…kingjubilee

Rollup of 9 pull requests

Successful merges:

 - rust-lang#126985 (Implement `-Z embed-source` (DWARFv5 source code embedding extension))
 - rust-lang#127922 (Add unsafe to extern blocks in style guide)
 - rust-lang#128731 (simd_shuffle intrinsic: allow argument to be passed as vector)
 - rust-lang#128935 (More work on `zstd` compression)
 - rust-lang#128942 (miri weak memory emulation: put previous value into initial store buffer)
 - rust-lang#129418 (rustc: Simplify getting sysroot library directory)
 - rust-lang#129490 (Add Trusty OS as tier 3 target)
 - rust-lang#129559 (float types: document NaN bit pattern guarantees)
 - rust-lang#129642 (Bump backtrace to rust-lang/backtrace@fc37b22)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#126985 (Implement `-Z embed-source` (DWARFv5 source code embedding extension))
 - rust-lang#127922 (Add unsafe to extern blocks in style guide)
 - rust-lang#128731 (simd_shuffle intrinsic: allow argument to be passed as vector)
 - rust-lang#128935 (More work on `zstd` compression)
 - rust-lang#128942 (miri weak memory emulation: put previous value into initial store buffer)
 - rust-lang#129418 (rustc: Simplify getting sysroot library directory)
 - rust-lang#129490 (Add Trusty OS as tier 3 target)
 - rust-lang#129536 (Add `f16` and `f128` inline ASM support for `aarch64`)
 - rust-lang#129559 (float types: document NaN bit pattern guarantees)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e209b05 into rust-lang:master Aug 27, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
Rollup merge of rust-lang#128935 - lqd:needs-zstd, r=Kobzol

More work on `zstd` compression

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 lqd deleted the needs-zstd branch August 27, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc 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
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants