CI: Add FreeBSD, OpenBSD and NetBSD builds - #571
Conversation
Introduce CI builds for three BSD operating systems using cross-platform-actions, based on QEMU virtual machines accelerated with KVM for near-native performance. The FreeBSD build enables additional Clang hardening checks to detect issues such as integer overflow and to improve memory safety. Addresses davmac314#558
mobin-2008
added
P-BSD
| runs-on: ubuntu-latest | ||
| defaults: | ||
| run: | ||
| shell: cpa.sh {0} --environment-variables CXX |
There was a problem hiding this comment.
I had to look up documentation to find out what "cpa.sh" is and does. It should have comments to explain (at least briefly) what it is and where to find more information.
Is --environment-variables CXX necessary? Wouldn't it be cleaner to specify the environment_variables input to the action?
There was a problem hiding this comment.
Added a comment to explain what is cpa.sh and why it's needed.
environment_variables seems cleaner. Will be fixed.
| # Make "configure" always use clang++. Our NetBSD image has g++ 10 pre-installed, and | ||
| # "configure" prefers it over clang++, but g++ 10 is too old to build Dinit. | ||
| # Note that it's defined here for all the BSDs because it's cleaner than specifying it only | ||
| # for NetBSD. FreeBSD already uses clang++ due to the lack of g++ in its image and OpenBSD | ||
| # uses a static mconfig that specifies clang++ as the C++ compiler. | ||
| CXX: clang++ |
There was a problem hiding this comment.
Specifying CXX as clang++ via the build (and/or configure) command line would be simpler than forcing it into the environment, wouldn't it?
In "Note that it's defined here" why is this a "Note" and what is "it"?
There was a problem hiding this comment.
The only build that has a 'Configure' step in it is the FreeBSD build and that's for specifying compiler and linker flags. NetBSD also uses configure by default due to lack of static mconfig for NetBSD. For explicitly passing compiler to configure, we need to specify an extra step with a condition of ${{ matrix.name == 'NetBSD' }} which I think is not cleaner than specifying this environment variable here.
If we go with passing compiler to Make directly, every step invoking Make should change to include CXX=clang++ which also I think is not cleaner than this approach.
In "Note that it's defined here" why is this a "Note" and what is "it"?
That's because I use to have a "Note" for pointing out a thing (especially in my native language writings) but it's not needed here and only hurts the flow of the paragraph. I will remove the 'Note that'.
'it' is the environment variable here. Will change to explicitly say 'This environment variable is...'.
There was a problem hiding this comment.
For explicitly passing compiler to configure, we need to specify an extra step
No, make CXX=clang++ runs configure with CXX set to clang++, doesn't it? Once that's done you don't need to pass CXX=clang++ to further make invocations.
But, you can keep it in the environment if you prefer.
'it' is the environment variable here. Will change to explicitly say 'This environment variable is...'
Yes, you'll need to fix that. You can only use "it" to refer to something that is already being discussed.
| # uses a static mconfig that specifies clang++ as the C++ compiler. | ||
| CXX: clang++ | ||
| strategy: | ||
| fail-fast: false # Be able to upload src/igr-tests/igr-output/ files in igr-tests step |
There was a problem hiding this comment.
I don't think the comment accurately describes what this is needed for.
See documentation here: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#jobsjob_idstrategyfail-fast
| with: | ||
| operating_system: ${{ matrix.name }} | ||
| version: ${{ matrix.version }} | ||
| - name: Getting dependencies (FreeBSD) |
There was a problem hiding this comment.
Rather than "Getting dependencies" maybe "Installing required packages". It is more descriptive and more accurate. The packages being installed are not dependencies of Dinit.
| - name: Print dinit executable file architecture | ||
| run: file ./src/dinit |
There was a problem hiding this comment.
Isn't this redundant considering clang++ -dumpmachine/clang++ --version above?
There was a problem hiding this comment.
The original use case for this was to make sure that we are building 32-bit binary on Debian i386 target. Considering that g++ -dumpmachine or clang++ --version shows the architecture, Yeah, this can be removed and I will remove it.
| clang++ -dumpmachine | ||
| clang++ --version |
There was a problem hiding this comment.
Does clang++ -dumpmachine display anything that clang++ --version doesn't already?
There was a problem hiding this comment.
No. -dumpmachine outputs the target machine and the --version outputs that too with some other information as well. clang++ -dumpmachine Will be removed.
There was a problem hiding this comment.
(but g++ only outputs version and copyright information on --version flag)
There was a problem hiding this comment.
clang++ -dumpmachine Will be removed
Ok.
| run: gmake check-igr | ||
| - name: Upload igr-tests output file(s) on failure | ||
| uses: actions/upload-artifact@v6.0.0 | ||
| if: failure() |
There was a problem hiding this comment.
Doesn't this mean igr-tests output is uploaded if any prior step fails even if the integration tests pass?
There was a problem hiding this comment.
That is a odd case to have any prior step fail but integration tests pass. If build fails, that's not going to happen. If unit tests fail, that's unlikely to happen.
I can modify this condition to check for igr-tests outcome to be failure before trying to upload.
There was a problem hiding this comment.
That is a odd case to have any prior step fail but integration tests pass.
Well actually, as it is currently no steps will continue if a prior one fails - except this one. So if the build or unit tests fail for example, integration tests won't even run, but this step would still be run.
I can modify this condition
Yes.
2 participants
Introduce CI builds for three BSD operating systems using cross-platform-actions, based on QEMU virtual machines accelerated with KVM for near-native performance.
The FreeBSD build enables additional Clang hardening checks to detect issues such as integer overflow and to improve memory safety.
Changes are self-reviewed and tested in my fork: https://github.com/mobin-2008/dinit/actions/runs/30704553393/job/91381298660
Addresses #558