Bump Docker image to Debian 12 for pure-rust-build
#1664
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This tests with various additional packages (tools and libraries) installed whose absence affected the `pure-rust-build` job, to establish the conditions under which it fails; then bumps the Debian 10 buster image to Debian 12 bookworm, becuase Debian 10 is past long-term support, and its Docker image is no longer being updated (see below for details); then redoes the original tests in reverse order to verify that each combination of packages had the same effect. This is several commit squashed together. (The shown reverted commit hashes may at some point become unavailable on the remote.) * Temporarily break `pure-rust-build` to verify that it can fail This makes the `pure-rust-build` CI job attempt to install gitoxide from `.` with implicit `max`, rather than explicit `max-pure`. This job, introduced in ed4deac (GitoxideLabs#624), is intended to identify unintended C toolchain dependencies. If, without the current breakage, it is (still) capable of doing this, then as temporarily modified here, it should fail. If it fails and the error messages clearly relate to the absence of components necessary to build/use C libraries, then it is probably (still) working. Assuming it fails as expected, the Debian version can then be increased, and then the changes here (and in any follow-up testing adjustments) undone: if that fails, then succeeds, respectively, then the job's functionality is most likely preserved across the upgrade. * Temporarily add `cmake` to `pure-rust-build` job This needs to be undone, since `max-pure` definitely should not require `cmake`. The reason for this temporary change is to figure out how robust the test is: currently, it fails on `max` as it should, but reports the absence of `cmake` as the reason. For the test to be robust, its failure should be for a deeper reason, and preferably even implicate the `minimal` Rust profile. * Temporarily add `build-essential` to `pure-rust-build` job This still installs `gcc` and `libc-dev` because `build-essential` depends on them, but this als installs packages such as `g++` and `make`, the absence of which seems to trigger failure before the effects of installing the `minimal` toolchain do (if they do). As in the previous change, this will of course need to be undone. * Temporarily add `pkgconf` and `libssl-dev` to `pure-rust-build` job This too must of course be undone. Like the last couple temporary changes, this is to see if the significance of using the `minimal` Rust toolchain can be identified. Note: This causes the test to pass, even with the `minimal` Rust toolchain. That's okay. These effects can be compared to those of the reverse incremental changes to be made after increasing the Debian image version. * Bump Docker image to Debian 12 for `pure-rust-build` It was previously Debian 10 buster. That version of Debian is past long term support (LTS), and although third-party Freexian ELTS updates are publicly available, the image on Docker Hub has not received any updates since the end of its LTS period. In contrast, the Debian 11 bullseye and Debian 12 bookworm images continue to be regularly updated, as of this writing. * Revert "Temporarily add `pkgconf` and `libssl-dev` to `pure-rust-build` job" This reverts commit 2ed0a37. * Revert "Temporarily add `build-essential` to `pure-rust-build` job" This reverts commit d9e1228. * Revert "Temporarily add `cmake` to `pure-rust-build` job" This reverts commit 26ebbe6. * Revert "Temporarily break `pure-rust-build` to verify that it can fail" This reverts commit b2e9289.
EliahKagan
force-pushed
the
run-ci/pure-rust-build
branch
from
ab5d772 to
7580e50
Compare
Byron
approved these changes
Nov 9, 2024
There was a problem hiding this comment.
Thanks a lot for the update and the thorough testing!
And I think you are right, the test doesn't document at all what's important to it, as it is 'absence' that matters.
Maybe it would be sufficient to add a few lines of script that validate that the mentioned packages, like g++, indeed aren't installed. This seems very valuable as nobody knows if in future, installing one of the prerequisites won't also pull in the dependencies that we explicitly don't want to see.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 9, 2024
This verifies the absence of utilities and libraries `max-pure` should not need, but that are needed for or, in the `libcurl*-dev` case, that may aid or modify the effect of, building `max`. When `pure-rust-build` was introduced in ed4deac (GitoxideLabs#624), the goal was to test that a C toolchain was not needed. Currently, we are installing a C toolchain, by installing `gcc` and `libc-dev`, so that the Rust toolchain will use the linker, which it may invoke through `cc`/`gcc`. Nonetheless, the test is effective, as verified in GitoxideLabs#1664, becuase it uses an environment free of several packages that `max-pure` would likely inadverently require for building, if it failed to be "pure".
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 9, 2024
This verifies the absence of utilities and libraries `max-pure` should not need, but that are needed for building `max`. When `pure-rust-build` was introduced in ed4deac (GitoxideLabs#624), the goal was to test that a C toolchain was not needed. Currently, we are installing a C toolchain, by installing `gcc` and `libc-dev`, so that the Rust toolchain will use the linker, which it may invoke through `cc`/`gcc`. Nonetheless, the test is effective, as verified in GitoxideLabs#1664, becuase it uses an environment free of several packages that `max-pure` would likely inadverently require for building, if it failed to be "pure".
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 9, 2024
This verifies the absence of utilities and libraries `max-pure` should not need, but that are needed for building `max`. When `pure-rust-build` was introduced in ed4deac (GitoxideLabs#624), the goal was to test that a C toolchain was not needed. Currently, we are installing a C toolchain, by installing `gcc` and `libc-dev`, so that the Rust toolchain will use the linker, which it may invoke through `cc`/`gcc`. Nonetheless, the test is effective, as verified in GitoxideLabs#1664, becuase it uses an environment free of several packages that `max-pure` would likely inadverently require for building, if it failed to be "pure". Utilities could, in principle, be installed as part of a package other than the package(s) that usually provide them. So a `$PATH` search is performed. However, `libssl-dev` is a library (and more libraries might be listed in the future), with no executable tool to do a `$PATH` search for. Furthermore, it may be possible for a utility to be installed, such that software in a Rust toolchain might find and use it, while not being in a `$PATH` directory. So this checks for known DEB packages as well as searching `$PATH`.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 9, 2024
This verifies the absence of utilities and libraries `max-pure` should not need, but that are needed for building `max`. When `pure-rust-build` was introduced in ed4deac (GitoxideLabs#624), the goal was to test that a C toolchain was not needed. Currently, we are installing a C toolchain, by installing `gcc` and `libc-dev`, so that the Rust toolchain will use the linker, which it may invoke through `cc`/`gcc`. Nonetheless, the test is effective, as verified in GitoxideLabs#1664, becuase it uses an environment free of several packages that `max-pure` would likely inadverently require for building, if it failed to be "pure". Utilities could, in principle, be installed as part of a package other than the package(s) that usually provide them. So a `$PATH` search is performed. However, `libssl-dev` is a library (and more libraries might be listed in the future), with no executable tool to do a `$PATH` search for. Furthermore, it may be possible for a utility to be installed, such that software in a Rust toolchain might find and use it, while not being in a `$PATH` directory. So this checks for known DEB packages as well as searching `$PATH`.
I've opened #1665 for this, and also included other changes that I think may be reasonable in view of the greater protection afforded by the new step. (I'm curious if this kind of CI job is common. If so, perhaps there is an even more minimal solution. I am guessing that the Rust toolchain won't work without |
|
It would certainly be nice for the Rust toolchain to be more independent of the C toolchain, particularly a compiler which seems to be used as a wrapper for |
|
I wish rust has something like zig-cc honestly, having it distributed via rustup would be great |
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 17, 2024
This adds more checks to the `pure-rust-build` CI job to reveal what dependencies of `max-pure` are building C code. The new checks are failing. They relate to GitoxideLabs#1681 and should pass once that bug is fixed. The main goal is to produce information helpful for GitoxideLabs#1681. As currently written, the new steps do not fail the job, because the failing steps have `continue-on-error: true`. This is so that regressions (that would fail the preexisting steps) remain readily detected. This should ideally be temporary; the new steps, if kept, should eventually be strengthened so they can fail the job. Currently this includes both regular and build dependencies. There are two new checks: 1. Search the `max-pure` dependency tree for packages that require a C or C++ compiler, or that are in practice only likely to be used to build C or C++ code. If any are found, display the whole tree, with matching lines highlighted, and fail the step. 2. After all steps that need it to be in working order, break GCC in the container by removing the `cc1` binary, then attempt a clean `max-pure` build, to reveal the first failure, if any, and let it fail the step. The `gcc` command itself is needed, because `rustc` calls the linker through it, even when no non-Rust code is compiled as part of the build. As discussed in GitoxideLabs#1664 comments, installing GCC in a way that is not broken but omits the ability to compile even C code, while it may be possible, is not commonly done and may require long running build steps or a new Docker image. Fortunately, the `gcc` command is just a "compiler driver"; the C compiler in GCC is `cc1`, and this can be removed without breaking most uses of GCC that don't involve actual compilation. This likewise removes `cc1plus` if found, even though it may not be present since we already verified that `g++` is not installed. (The deletion of `cc1` is the important part.) Since the job now cleans and starts a build that fails due to the absence of `cc1`, this removes the `rust-cache` step. (Caching would probably still confer some benefit, since it caches dependencies. Even after running `cargo clean` between the builds, most of these should be reacquired when building with `cc1`. However, to make the whole thing easier to reason about, the step is removed. It can be re-added if the job runs too slowly.) This considers any `-sys` crate to be a dependency that needs to build C. This is in principle not always the case, since they may use existing shared library binaries, though there may still be stub code that has to be built in C. The relevance of the new steps varies depending on precisely how one conceptualizes "pure" in `max-pure`, which is another reason for them to start out as `continue-on-error`. The `-sys` crates this finds in the `max-pure` dependency tree are: - `libsqlite3-sys` via `rusqlite`. This was reported in GitoxideLabs#1681 and strongly seems to be unintended for `max-pure`. - `linux-raw-sys` via `rustix` and `xattr`. It's less clear whether this should be considered impure, since its purpose is to interact with an operating system facility, and it may be comparable to the use of `libc` (on Unix-like systems). However, this does seem like it would not be able to build without a C compiler. The dependency tree also has an occurrence of the `cc` crate without a related `-sys` dependency, as required by `ring`. The `ring` crate contains a C test program that is built when building `ring`. That is currently the first build error shown in the output of the "Do max-pure build without being able to compile C or C++" step.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the major version of the Debian image used in the
pure-rust-buildCI job from Debian 10 (buster) to Debian 12 (bookworm).Debian 10 is past long term support (LTS). See the "Index of releases" table on the releases page. Although third-party Freexian ELTS updates, in which some packages are maintained, are publicly available, the image on Docker Hub has not received any updates since the end of its LTS period. In contrast, the Debian 11 (bullseye) image and Debian 12 bookworm image continue to be regularly updated, as of this writing.
This uses Debian 12 (bookworm) since it's the current stable version, but it should also be fine to use Debian 11 (bullseye). The main disadvantage is that it would have to be updated sooner. This newer version might also be faster, but I have not benchmarked and this job is not excessively slow. If it is preferable to check compatibility with an older release, then this can be changed to Debian 11.
This squashes a number of commits together because I made four commits first to test that the test can fail with
maxinstead ofmax-pure, and to check the effect of various toolchain and library related packages on the outcome; then bumped the Debian image; then reverted each commit one by one to verify that the effect of those packages on the job, and more broadly the ability of the job to fail under conditions where it ought to, is preserved with no changes in Debian 12.I am pleased to report that this is so. However, it may be that a comment should be added to clarify exactly what this job is for. This is because, on both Debian 10 and Debian 12, the packages that cause the job to fail when modified to build features that pull in native-code dependencies are not really failing due to a need for a C toolchain. This is what I would expect, since installing
gccandlibc-dev, which this has job has always done since being introduced in ed4deac (#624), is installing a C toolchain. The packages whose absence makes this test work along the intended lines areg++,cmake,make,pkgconf, andlibssl-dev. I am not completely sure what the comment should say, or if the test is testing quite what it is meant to. But none of that varies between Debian 10 and Debian 12, so I think it shouldn't block this.I don't usually squash the entirety of a feature branch into a single commit, but in this case it seemed the best thing to do. But I've force-pushed the squashed commits immediately after opening this PR, so the pre-squash hashes are shown in an event here. They have CI status that can be observed on my fork. (The detailed CI results will go away after a while on GitHub, and the commit messages make fairly clear what they indicate as well as what changes were made, so I think having nine commits instead of one here would not be better.)