Skip to content

Ensure compilers are installed#7

Merged
bthomee merged 17 commits intomainfrom
error-out
Jul 10, 2025
Merged

Ensure compilers are installed#7
bthomee merged 17 commits intomainfrom
error-out

Conversation

@bthomee
Copy link
Contributor

@bthomee bthomee commented Jul 9, 2025

This PR adds additional checks to ensure the compilers are correctly installed.

  • A version check is performed to verify that the desired GCC or Clang version is present.
  • Each built Docker image compile and execute an example Conan CMake project.
    • Note that due to how we configured the multi-architecture Docker action, it was challenging to perform the test as a separate job step; the test is therefore performed during the image building and the files are removed afterwards.

Several additional changes are made to (i) make the images consistent with each other, (ii) remove extraneous files, and (iii) remove packages that will be moved into the tools image(s).

This change supersedes #6.

@bthomee bthomee requested review from Bronek and legleux July 9, 2025 16:48
@bthomee bthomee requested a review from legleux July 9, 2025 22:52
@legleux
Copy link
Collaborator

legleux commented Jul 9, 2025

How are you using these images that the entrypoint being bash is useful?

Comment on lines +57 to +63
update-alternatives \
--install /usr/bin/gcc gcc /opt/rh/gcc-toolset-${GCC_VERSION}/root/usr/bin/gcc ${GCC_VERSION} \
--slave /usr/bin/g++ g++ /opt/rh/gcc-toolset-${GCC_VERSION}/root/usr/bin/g++ \
--slave /usr/bin/cpp cpp /opt/rh/gcc-toolset-${GCC_VERSION}/root/usr/bin/cpp \
--slave /usr/bin/gcov gcov /opt/rh/gcc-toolset-${GCC_VERSION}/root/usr/bin/gcov \
--slave /usr/bin/gcov-dump gcov-dump /opt/rh/gcc-toolset-${GCC_VERSION}/root/usr/bin/gcov-dump-${GCC_VERSION} \
--slave /usr/bin/gcov-tool gcov-tool /opt/rh/gcc-toolset-${GCC_VERSION}/root/usr/bin/gcov-tool-${GCC_VERSION}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had suggested this before being reminded about the SCL toolset enabling.

To me it seems like an either/or. If this works, I guess it's ok but I'm suspicious and feel like this is just what enabling the toolset does... and possibly more magic.

Now none of the other binutils are tracked and updated with this (which I'm not sure how much that really matters).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is strictly better, because coverage target will detect gcov which will now be the right version to match the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are other binutils we need, we can follow the same process to make them globally available without SCL.

An alternative would be to edit the SHELL and ENTRYPOINT to run the SCL command, but I prefer the current approach as its behavior is more consistent with what I'd expect in an image.

Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

Someone will have to review my commits, but this aside it's great !

@bthomee
Copy link
Contributor Author

bthomee commented Jul 10, 2025

How are you using these images that the entrypoint being bash is useful?

By not setting the entrypoint, you get what's defined in the base image, and that's not what I wanted / expected when I just did "docker run" without overriding the entrypoint. Each image did something different, one presented a non-Bash shell with a long preamble, another one just did nothing. This way the experience is consistent.

I was using the shell to manually compile the rippled binary, so having the shell set like it is now is convenient.

@bthomee
Copy link
Contributor Author

bthomee commented Jul 10, 2025

Someone will have to review my commits, but this aside it's great !

I reviewed them - thanks!

@bthomee bthomee merged commit 0ec5578 into main Jul 10, 2025
39 checks passed
@bthomee bthomee deleted the error-out branch July 10, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants