Treat warnings as errors in build of cppjit - #45
Conversation
…er-research#41) * [cpyrt] Improve error reporting for method calls without C++ object Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * [test] Add test for method calls on an instance without a C++ object Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Grigori Rybkine <Grigori.Rybkine@cern.ch> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…esearch#40) * [cpyrt] Penalize void* arguments in overload priority as intended * [test] Add regression test for void* overload priority --------- Co-authored-by: Emery Conrad <emery.conrad@chicagotrading.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…rch#42) Fixes the long-standing header duplication (previously `Cppyy.h` and `cpp_cppyy.h`) between cpyrt and interop, so that there is only a single source of definitions for the `cppjit::interop` API and types.
aaronj0
left a comment
aaronj0
left a comment
There was a problem hiding this comment.
LGTM, thanks. Can you check if there is anything else thats stale in this MakeFile?
|
@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 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 You will need to merge this PR if happy with it, since I don't have permissions to do anything in this repo. |
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.
Regarding moving to a CMake file, that sounds like a good idea, but perhaps the simplicity of this MakeFile is desired :) |
|
I have added |
mcbarton
changed the title
Sounds good, thank you! |
42269c2 to
d90df13
Compare
Changed definition of a function to the test it is actually used, and added a missing import
…er-research#50) Drops wheel sizes by about half. Previously the entire install tree of CppInterOp was staged that included duplicate shared libs due to versioning. This is fixed by adding a stripped shared-lib option in CppInterOp, leveraged in this patch.
…ch#52) [cpyrt] Clear the error indicator only where a call failed A debug build of CPython asserts when a C API call is made with the error indicator already set, so the __cpp_cross__ annotation, the meta_setattro fallthrough to tp_setattro, and the VectorData alias need it cleared. Clear it only on the failing path: AddToClass reports failure, and the two CPPScope sites can test the value they just produced, so an error raised elsewhere still propagates. meta_getattro returns a new reference; release it instead of leaking it.
|
@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 |
* [test] Find eigen and boost under the Homebrew and MacPorts prefixes * [ci] Build and test manylinux and macOS arm64 wheels
…esearch#54) SetArg() created an element converter per call and appended it to fConverters, but Clear() frees only fBuffer, so the vector grew without bound across repeated std::initializer_list conversions. Create each element converter once, on first use of its index, and reuse it.
…iler-research#51) * Serialize and atomize test dictionary builds * Force loadfile scheduling for distributed test runs * Normalize the xfail marker keyword order * Correct the xfail markers and add missing reasons * Add the --run-crashing-xfails collection option * Enable strict xfail * Drop xfail markers that no longer fail on macOS and cling * Make the span tests include their own header
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.
3 participants
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_moduleis labelled as obsolete, so this PR removes the flag.