FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Treat warnings as errors in build of cppjit by mcbarton · Pull Request #45 · compiler-research/cppjit · GitHub

Treat warnings as errors in build of cppjit - #45

Draft
mcbarton wants to merge 35 commits into
compiler-research:mainfrom
mcbarton:main
Draft

Treat warnings as errors in build of cppjit#45
mcbarton wants to merge 35 commits into
compiler-research:mainfrom
mcbarton:main

Conversation

Copy link
Copy Markdown

If you look at the nightly builds (here for example https://github.com/compiler-research/cppjit/actions/runs/32926841974/job/98051372290#step:8:695), you'll see that the single_module is labelled as obsolete, so this PR removes the flag.

aaronj0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks. Can you check if there is anything else thats stale in this MakeFile?

mcbarton commented Aug 27, 2026
edited
Loading

Copy link
Copy Markdown
Author

@aaronj0 I cannot see anything taking a quick look at the ci. If you update the reusable action here https://github.com/compiler-research/ci-workflows/blob/main/actions/build-and-test-cppjit/action.yml to do the cmake build with CMAKE_COMPILE_WARNING_AS_ERROR=ON you should catch any leftover warnings (if there is any) when building cppjit. To catch them when building the tests you'll want to add CXXFLAGS="-Werror" to the make command (and maybe a linker flag since I don't think this catches linker warnings).

To match Clad, xeus-cpp, CppInterOp I think it would make sense to remove the handwritten makefile and replace it with a cmake file which the main cmake can call, but that is just personal choice. You could also change it so the tests are built with Wall like the cppjit library for some extra checks.

You will need to merge this PR if happy with it, since I don't have permissions to do anything in this repo.

aaronj0 commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

@aaronj0 I cannot see anything taking a quick look at the ci. If you update the reusable action here https://github.com/compiler-research/ci-workflows/blob/main/actions/build-and-test-cppjit/action.yml to do the cmake build with CMAKE_COMPILE_WARNING_AS_ERROR=ON you should catch any leftover warnings (if there is any) when building cppjit. To catch them when building the tests you'll want to add CXXFLAGS="-Werror" to the make command (and maybe a linker flag since I don't think this catches linker warnings).

That sounds like a good idea, but we should probably first inject the flag in the CMakeLists.txt and test MakeFile here and see what fails. We can then address the remaining issues as a part of this PR, before making that default in the ci-workflows yml.

To match Clad, xeus-cpp, CppInterOp I think it would make sense to remove the handwritten makefile and replace it with a cmake file which the main cmake can call, but that is just personal choice. You could also change it so the tests are built with Wall like the cppjit library for some extra checks.

Regarding moving to a CMake file, that sounds like a good idea, but perhaps the simplicity of this MakeFile is desired :)
One point is we cannot make building these dictionaries and test-time artifacts part of the higher level scikit-driven CMake, rather driven by the pytest driver when a user runs the Python tests on their system. An improved pytest driver that automates this instead of a user having to run make is a part of my incoming changes referred to in #46

Copy link
Copy Markdown
Author

I have added Werror and Wextra to the build of cppjit, and Wall, Werror and Wextra to the tests. These have raised some new warnings. I will make my way through them, but I might not start until next week.

mcbarton marked this pull request as draft August 27, 2026 15:54
mcbarton changed the title Remove obsolete single_module flag Treat warnings as errors in build of cppjit Aug 27, 2026

aaronj0 commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

I have added Werror and Wextra to the build of cppjit, and Wall, Werror and Wextra to the tests. These have raised some new warnings. I will make my way through them, but I might not start until next week.

Sounds good, thank you!

This file contains hidden or 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
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.

2 participants


Back | FazBrowse Home | New Git URL