diffkemp icon indicating copy to clipboard operation
diffkemp copied to clipboard
verified publisher icon viktormalik

Modernize python builds

Open DanielKriz opened this issue 1 year ago • 4 comments

This pull requests addresses issue #338 and updates our python infrastructure.

DanielKriz avatar Jun 06 '24 09:06 DanielKriz

I still have to test this out on our full-fledged unit tests, to check if I haven't broken anything.

I should also test how it behaves in proper release build.

DanielKriz avatar Jun 06 '24 09:06 DanielKriz

Thanks for a great review, there are some really good points and advancement, I shall incorporate them.

DanielKriz avatar Jul 15 '24 07:07 DanielKriz

Thanks for the detailed analysis and testing @PLukas2018!

I didn't have time to review the PR, yet, but I can confirm that the above is the correct set of configurations that we want to work.

I also like the list of required docs updates, these should be done as a part of this PR.

viktormalik avatar Aug 01 '24 12:08 viktormalik

@viktormalik it should be ready for the review. It seems that it is working. In the meantime I will read the documentation once again if something is not missing.

DanielKriz avatar Mar 05 '25 11:03 DanielKriz

For some reason, the development build is not working for me. I'm getting failures during compilation.

Build log: 
$ cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug
-- The C compiler identification is GNU 12.3.0
-- The CXX compiler identification is GNU 12.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /nix/store/ihhhd1r1a2wb4ndm24rnm83rfnjw5n0z-gcc-wrapper-12.3.0/bin/gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /nix/store/ihhhd1r1a2wb4ndm24rnm83rfnjw5n0z-gcc-wrapper-12.3.0/bin/g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Could NOT find FFI (missing: FFI_LIBRARIES HAVE_FFI_CALL) 
-- Performing Test Terminfo_LINKABLE
-- Performing Test Terminfo_LINKABLE - Success
-- Found Terminfo: /nix/store/lhfi31nbyv62rw5szbfrh3pdnvcfgxmc-ncurses-6.4/lib/libtinfo.so  
-- Found ZLIB: /nix/store/v27dxnsw0cb7f4l1i3s44knc7y9sw688-zlib-1.3/lib/libz.so (found version "1.3")  
-- Could NOT find LibXml2 (missing: LIBXML2_LIBRARY LIBXML2_INCLUDE_DIR) 
-- Found LLVM 16.0.6
-- Setting SIMPLL_BUILD_DIR to /home/vmalik/src/diffkemp/diffkemp/build-llvm16
-- Generating bin/diffkemp executable
-- Creating pytest tests executable
-- Configuring done (1.4s)
-- Generating done (0.0s)
-- Build files have been written to: /home/vmalik/src/diffkemp/diffkemp/build-llvm16

$ ninja -j4
[41/58] Linking CXX executable diffkemp/simpll/diffkemp-simpll
FAILED: diffkemp/simpll/diffkemp-simpll 
: && /nix/store/ihhhd1r1a2wb4ndm24rnm83rfnjw5n0z-gcc-wrapper-12.3.0/bin/g++ -fno-rtti -fpic -Wall -Wextra -g  diffkemp/simpll/CMakeFiles/simpll.dir/SimpLL.cpp.o -o diffkemp/simpll/diffkemp-simpll  -Wl,-rpath,/usr/lib64  diffkemp/simpll/libsimpll-lib.a  -lLLVM-16  /usr/lib64/libz3.so.4.13.4.0 && :
/nix/store/3z013mdl9cvpgvavpj19rbilihz4clvi-binutils-2.40/bin/ld: /nix/store/im40dz3167rw55lbg3r5p1f8n4c46x3z-llvm-16.0.6-lib/lib/libLLVM-16.so: undefined reference to `tigetnum@NCURSES6_TINFO_5.0.19991023'
/nix/store/3z013mdl9cvpgvavpj19rbilihz4clvi-binutils-2.40/bin/ld: /nix/store/im40dz3167rw55lbg3r5p1f8n4c46x3z-llvm-16.0.6-lib/lib/libLLVM-16.so: undefined reference to `del_curterm@NCURSES6_TINFO_5.0.19991023'
/nix/store/3z013mdl9cvpgvavpj19rbilihz4clvi-binutils-2.40/bin/ld: /nix/store/im40dz3167rw55lbg3r5p1f8n4c46x3z-llvm-16.0.6-lib/lib/libLLVM-16.so: undefined reference to `set_curterm@NCURSES6_TINFO_5.0.19991023'
/nix/store/3z013mdl9cvpgvavpj19rbilihz4clvi-binutils-2.40/bin/ld: /nix/store/im40dz3167rw55lbg3r5p1f8n4c46x3z-llvm-16.0.6-lib/lib/libLLVM-16.so: undefined reference to `setupterm@NCURSES6_TINFO_5.0.19991023'
/nix/store/3z013mdl9cvpgvavpj19rbilihz4clvi-binutils-2.40/bin/ld: /usr/lib64/libz3.so.4.13.4.0: undefined reference to `std::ios_base_library_init()@GLIBCXX_3.4.32'
/nix/store/3z013mdl9cvpgvavpj19rbilihz4clvi-binutils-2.40/bin/ld: /usr/lib64/libz3.so.4.13.4.0: undefined reference to `__cxa_call_terminate@CXXABI_1.3.15'
collect2: error: ld returned 1 exit status
[44/58] Building CXX object tests/unit_tests/simpll/CMakeFiles/runTests.dir/DFCLlvmIrTest.cpp.o
ninja: build stopped: subcommand failed.

This is Nix development env, happens for both LLVM 16 and 17.

Am I doing something wrong here?

viktormalik avatar Jul 04 '25 11:07 viktormalik

For some reason, the development build is not working for me. I'm getting failures during compilation.

Build log: This is Nix development env, happens for both LLVM 16 and 17.

Am I doing something wrong here?

Turns out it's a bug in how we handle Z3. Opened #383 to address that.

viktormalik avatar Jul 04 '25 11:07 viktormalik

I tested building and comparing on clean Fedora and discovered two problems. Both of these depend on how we want to use DiffKemp for local development without Nix (if we even want it):

  1. I originally thought that we do not want to use pip install -e . and use the build/bin/diffkemp binary the same as in the Nix.
  2. The other option is still using the pip install -e..

Yes, we want to support option 1. If option 2 works, then it's all good but it doesn't need to be supported.

If we want to use the first option, the problems are:

  1. How to install Python dependencies when they are now placed in pyproject.toml -- I guess for e.g. the user can copy them and install manually but it should be somehow mentioned in the installation documentation.

Yeah, this is tricky, as we no longer have requirements.txt.

I guess a bit hacky the solution could be

$ pip install .
$ pip uninstall diffkemp

But I wouldn't consider it a blocker if you had to install them manually by looking into pyproject.toml.

viktormalik avatar Jul 04 '25 13:07 viktormalik

I created a PR resolving the issues mentioned in the comments. I created the PR on @DanielKriz' fork so he can review it and possibly clean up the commits and update this PR with it.

PLukas2018 avatar Jul 30 '25 12:07 PLukas2018

@viktormalik It seems that it is working, now is the time to either squash the commits or clean up the history. What would you want to change in the history to make it cleaner and mergeable?

DanielKriz avatar Sep 09 '25 15:09 DanielKriz

My personal guess would be something like this:

  1. CMake: always rebuild simpll CFFI and move build to build dir: OK
  2. CMake: always define SIMPLL_BUILD_DIR: OK
  3. CMake: use python3 executable with SimpLL build script squash into 2 (reason: fixes previous commit)
  4. CMake: generate development diffkemp executable: OK
  5. simpll_lib: remove the need to check build directory: probably OK
  6. simpll_build: use implicit import squash to something (probably 1) as it is fix
  7. pyproject: move to pyproject from setuptools: OK
  8. tests: add testing executable: OK
  9. Make get_callstack public symbol: probably OK
  10. Remove old diffkemp executable probably squash into 7
  11. Remove deprecated pkg_resources: probably OK
  12. Simplify SimpLL importing squash with 5
  13. Update documentation: OK
  14. Allow using generated executable without nix squash into 4
  15. CI: use diffkemp executable in builds: problematic as the CI should pass after each commit, probably squash into 7 (or 10)
  16. CI: add python CFFI as a dependency squash into 15
  17. CMake: explicitly add path to the simpll_build in cmake squash into 2
  18. simpll: add explicit link to z3: not sure maybe move at the top if it is needed for migration to pyproject
  19. CI: Use virtualenv in local builds when installing python squash into 15
  20. SimpLL build: Add comment -- determining root dir squash into 1
  21. Update documentation of simpll_lib.py squash into 1
  22. CI: Update and fixes squash into 15
  23. Set SIMPLL_BUILD_DIR environ for development redistribute to 15, 4 and 8
  24. Docs: Updated installation of DiffKemp squash into 13
  25. pyproject: move optional dependencies to correct section redistribute to 13, 15 and 7
  26. CC WRAPPER: Passing PYTHONPATH to the wrapper should be squashed into something (probably 4)
  27. pyproject: add missing CC wrapper package squash into 7

PLukas2018 avatar Sep 09 '25 17:09 PLukas2018

Wow, excellent analysis @PLukas2018. I can only agree, obviously if something cannot be easily squashed, it can stay as a separate commit (with a reasonable commit message). Also if you don't have time to do that, we can probably merge it as it is since the PR has already taken plenty of time (and most of the commits are reasonable even though they are fixes).

viktormalik avatar Sep 09 '25 19:09 viktormalik

The analysis was perfect and extremely well done, good job. I have changed the commit history in accordance to it. However, I am not sure about squashing the commits as it seems that we should rewrite a lot of commit message (because now they contain useless names of squashed commits). According to the git diff the resulting code should be the same, but let's wait for the CI.

DanielKriz avatar Sep 09 '25 21:09 DanielKriz

Let's go :rocket: :rocket: :rocket:

viktormalik avatar Sep 10 '25 06:09 viktormalik