-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Use official Clang/GCC containers #3703
Conversation
@@ -30,7 +30,7 @@ execute_process(COMMAND ${CPPCHECK_TOOL} --version OUTPUT_VARIABLE CPPCHECK_TOOL | |||
string(REGEX MATCH "[0-9]+(\\.[0-9]+)+" CPPCHECK_TOOL_VERSION "${CPPCHECK_TOOL_VERSION}") | |||
message(STATUS "🔖 Cppcheck ${CPPCHECK_TOOL_VERSION} (${CPPCHECK_TOOL})") | |||
|
|||
find_program(GCC_TOOL NAMES g++-latest g++-HEAD g++-11 g++-10) | |||
find_program(GCC_TOOL NAMES g++-latest g++-HEAD g++-12 g++-11 g++-10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCC compiler detection needs some love. This doesn't work on Gentoo and I always have to remember to specify GCC_TOOL
manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I would be able to remove this altogether once we clean up the CI image. I could not use gcc:latest btw. due to incompatible compiler flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler flags are also a challenge for the CMake preset file. I was really hoping for a convenient way to use the CI warning flags in local builds. But that might resolve your issue, as I have to sort all flags by compiler version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, the CI targets are meant to work in the context of this project's CI. If they are useful on a different machine, then this is fine, but only a coincidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment about future improvements, otherwise looks good to me.
@@ -667,8 +667,11 @@ add_custom_target(ci_cppcheck | |||
############################################################################### | |||
|
|||
add_custom_target(ci_cpplint | |||
COMMAND ${Python3_EXECUTABLE} ${CMAKE_SOURCE_DIR}/tools/cpplint/cpplint.py --filter=-whitespace,-legal,-runtime/references,-runtime/explicit,-runtime/indentation_namespace,-readability/casting,-readability/nolint --quiet --recursive ${SRC_FILES} | |||
COMMAND ${Python3_EXECUTABLE} -mvenv venv_cpplint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super happy about more venvs being hard-coded.
In general, I'd like to be able to specify a) don't use a venv at all, or b) create/use a venv at a specific location. (I'm experimenting with this approach here: https://github.com/falbrechtskirchinger/json-ci/blob/split3/Makefile) One more thing for the ever-growing to-do list, I guess. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what's bad about this venv. Like this, the target is independent and just needs a Python interpreter - and allows to execute this target independently of any Docker image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just like the option to use or not use a venv, and if I choose to use a venv, specify its location. There's no reason to have a separate venv for every working tree and in the case of cpplint, there's no reason to use any venv on my system at all.
venvs are a (small) reason why my json directory (with currently 31 working trees) is over 20GB in size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could set up the venv in the GitHub Actions job and assume in this target that cpplint
can be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most importantly, things should work consistently across Makefiles and CMake.
I like the idea of having two environment variables per venv:
JSON_<NAME>_USE_VENV
to enable or disable using a venv (default true
)
JSON_<NAME>_VENV
that specifies the path of the venv (with a default, if not set)
This works in the Makefile linked above and I'd want to repeat it for mkdocs
, reuse
, etc,
I haven't thought about how to apply that concept to GitHub workflows and CMake code yet but am aware of the Python3_FIND_VIRTUALENV
option of FindPython3
, for example.
Eventually, I envision adding the following to my .bashrc
or .profile
and avoid unnecessary duplication:
JSON_MKDOCS_USE_VENV=true
JSON_MKDOCS_VENV="$HOME/projects/json/.share/venvs/mkdocs"
JSON_REUSE_USE_VENV=true
JSON_REUSE_VENV="$HOME/projects/json/.share/venvs/reuse"
JSON_CPPLINT_USE_VENV=false
JSON_TEST_DATA_DIRECTORY="$HOME/projects/json/.share/json_test_data"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just hard-code .share/venv
to the Makefiles/CMakeLists.txt? Then the CI can install the required things there, and everybody else can bring their own venv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
projects/json
is not the git checkout. projects/json/develop
is and then I create additional working directories using git worktree add ../my-feature
which creates the branch my-feature
and the working tree projects/json/my-feature
. See https://github.com/nlohmann/json/tree/develop/tools/serve_header#serving-jsonhpp-from-multiple-project-directory-instances-or-working-trees.
The whole point is for the venv to be able to sit outside of the git checkout and to be shared by multiple working trees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry - I mistook the directory.
This PR tries to use the containers from https://hub.docker.com/r/silkeh/clang and https://hub.docker.com/_/gcc where possible instead of using the "big" json-ci container.
So far, I could use the official images for all Clang versions and GCC versions >= 7.
For GCC 4.8 to GCC 6, some fetch_content tests fail which I have not yet looked into in detail.
The runtime is much faster now, because the images only take ~1 minute to come up which goes down to few seconds when cached.
Todos:
lukka/get-cmake@latest
can help to move some jobs to GitHub images