Skip to content

Treat warnings as errors in build of cppjit - #45

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

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

Conversation

@mcbarton

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 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.

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

@mcbarton

mcbarton commented Aug 27, 2026

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

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

@mcbarton

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
mcbarton marked this pull request as draft August 27, 2026 15:54
@mcbarton mcbarton changed the title Remove obsolete single_module flag Treat warnings as errors in build of cppjit Aug 27, 2026
@aaronj0

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!

@mcbarton
mcbarton force-pushed the main branch 6 times, most recently from 42269c2 to d90df13 Compare August 29, 2026 09:00
@mcbarton

mcbarton commented Aug 31, 2026

Copy link
Copy Markdown
Author

@Vipul-Cariappa @vgvassilev @aaronj0 How would you advise me proceeding with this PR now? I worked my way through the warnings outputted by adding -Wextra -Werror to the library build. About 99% of the errors were about PyCFunction and PyObject being incompatible when it comes to casting (I've 90% sure I didn't fix this error in the correct way, and probably the reason tests are failing). There was a few unused variable errors taken care of in another PR recently upstreamed from root, and one about fallthrough in interop_wrapper.cxx

Try to fix more warnings

Try to fix more warnings

Try to fix error

Try fixes

Revert "Try fixes"

This reverts commit 1adc6fb.

Try partial fix

Try fix

Attempt partial fix

Attempt partial fix

Try partial fix

Try fix

Try fix

Attempt fix

Fix

Test fix

Test fix

Test

Test fix

Revert "Test fix"

This reverts commit e4eff56.

Test fix

Test fix

Test fix

Test fix

Test fix

Test fix

Try fix

Test fix

Try fix

Partial fix

Revert "Partial fix"

This reverts commit 1bee1a8.

Revert test makefile changes

Try fixing  test

Try to fix

Revert "Try fixing  test"

This reverts commit 09db7cd.

Revert "Try fix"

This reverts commit f0f32c1.

Revert "Test fix"

This reverts commit 88a03b1.

Revert "Try fix"

This reverts commit e32f292.
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