Minimum recommended toolchain - document and test it#1874
Conversation
Establish clear boundaries for the project's recommended build and testing environments.
Debian "oldoldstable" is the minimum recommended build environment.
0ba8a95 to
6dd486e
Compare
|
Rebased on top of the recent CI changes. |
real-or-random
left a comment
There was a problem hiding this comment.
Weak Concept ACK (if we remove the "recommendation part") -- I think it's a useful test and the maintenance burden will be okay
@fanquake I wonder what you think from with your maintainer hat on.
| Toolchains older than the default one shipped in the current Debian "oldoldstable" release | ||
| are not tested and are not recommended for building the project. |
There was a problem hiding this comment.
I think we should remove this sentence (and change the PR title accordingly).
- I don't think we should recommend a mininum compiler. People should use modern compilers, but this is nothing we need to state. What compiler we test with is an internal thing, and this should not become a "user-facing" guarantee (The wording does not imply a guarantee, but if it's not guarantee, it doesn't belong in the README. We could add it to CONTRIBUTING but I don't think it's necessary. The CI check is enough documentation.)
- That older than oldoldstable is not recommended could be interpreted to mean that oldoldstable and newer is recommend. But I wouldn't recommend oldoldstable to anyone.
There was a problem hiding this comment.
I agree. I'm not sure there's much value (for this project) in trying to establish a minimum supported compiler version, and it seems possible that tests for this are going to add more friction than value. As long as they are low-effort, and can be deleted/changed easily, probably fine.
I'd also say that if we were going to document something, just state the particular compiler, rather than make people look up what compiler/version pair happened to ship on an old version of some Linux distro.
There was a problem hiding this comment.
I'd also say that if we were going to document something, just state the particular compiler, rather than make people look up what compiler/version pair happened to ship on an old version of some Linux distro.
No strong opinion here but I still I tend to simply say "oldoldstable". Then we won't run into the need to bump the version regularly (e.g., because something breaks on an old compiler/docker image) and worse, use our brain cycles to make a decision whether the bump is justified without clear guidelines.
|
|
||
| Building with Autotools | ||
| ----------------------- | ||
| ## Building from source |
There was a problem hiding this comment.
nit:
| ## Building from source | |
| ## Building |
Maybe it's just me, but "building from source" sounds like this is a special thing and we also ship binaries.
There was a problem hiding this comment.
see my "code" suggestion: Just "Building"
| ENV VIRTUAL_ENV=/root/venv | ||
| RUN python3 -m venv $VIRTUAL_ENV | ||
| ENV PATH="$VIRTUAL_ENV/bin:$PATH" | ||
| RUN pip install lief |
There was a problem hiding this comment.
I think we should omit python from this job to simplify maintenance. We don't want to be restricted to whatever Python version oldoldstable ships (and who knows if lief will runs on that version in the future).
There was a problem hiding this comment.
Disabling Python would require more changes in this PR to handle the testvectors build target, similar to 071523b.
There was a problem hiding this comment.
Hm, yeah. I was thinking we can just set SYMBOL_CHECK=no, but I forgot about the test vectors. Though I can we could also have TEST_VECTORS variable that is enabled by default. That would just change a single line in the script.
|
@hebasto Have you checked if the new Docker image fits in the GHA cache (together with the existing cache artifacts)? |
This PR:
Establishes clear boundaries for the project's recommended build and testing environments.
Adds a CI job that tests the project's minimum recommended build environment.
This is useful for reviewing changes like #1865 or #1869.
For the context: