Skip to content
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

Change Python module build (setup.py / setuptools config) to make it more widely compatible #1137

Merged
merged 7 commits into from
Mar 23, 2024

Conversation

johnbartholomew
Copy link
Collaborator

@johnbartholomew johnbartholomew commented Feb 28, 2024

This moves us toward a Windows & MSVC-compatible Python module build, and should obsolete #784, and move toward a resolution for #734.

  • Change setup.py to compile the code using built in C/C++ extension build system, not using the Makefile.
  • Minor fix in python/_jsonnet.c to use Py_ssize_t instead of ssize_t, for compatibility with some MSVC version(s) that don't have ssize_t available.
  • Initial GitHub action workflow to build Python wheel packages for Linux and Windows, for various CPython versions. This uses cibuildwheel which - as far as I can judge as an ecosystem outsider - is the current supported/widely used tool for building a set of version-compatible wheel packages.
  • Runs python/_jsonnet_test.py for each build as a smoke test.

Things not in this PR:

  • Publishing the wheel packages anywhere.
  • Automatically running the workflow on relevant events like tagging a release version. The workflow is just set up to allow it to be manually triggered. Note that manually triggered GitHub actions are kind of weird - in particular, there's no way to run them (as far as I know) until the workflow config has been included in the main (HEAD) branch of the repo. So it's possible this workflow could be broken and we probably won't be able to tell for sure until after it's merged to master. However I did try it on my fork (example run: https://github.com/johnbartholomew/jsonnet/actions/runs/8069203253)

Is it tested? Kinda. I don't have a Windows machine to test on, so I'm relying on the GitHub workflow to build & run the Python test.

@johnbartholomew johnbartholomew force-pushed the new-python-build branch 2 times, most recently from 7d4e583 to 01d1a0e Compare February 28, 2024 21:53
@johnbartholomew
Copy link
Collaborator Author

Adjusted the workflow configuration to give cibuildwheel a test command to run - this just runs the _jsonnet_test.py script. Currently it's failing for the Windows wheel build. Looks like a problem with relative import paths (perhaps the same or partially the same problem as #867, though that particular report is a little unclear).

Unfortunately it seems all the path handling code only expects / as a directory separator.

   E...EEEE..
  ======================================================================
  ERROR: test_double_import (__main__.JsonnetTests)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "D:\a\jsonnet\jsonnet\python\_jsonnet_test.py", line 172, in test_double_import
      json_str = _jsonnet.evaluate_snippet(
  RuntimeError: RUNTIME ERROR: couldn't open import "trivial.jsonnet": File not found
  	D:\a\jsonnet\jsonnet\python\testdata\test.jsonnet:1:11-35	thunk <x>
  	D:\a\jsonnet\jsonnet\python\testdata\test.jsonnet:3:1	
  
  
  ======================================================================
  ERROR: test_import_binary_encode (__main__.JsonnetTests)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "D:\a\jsonnet\jsonnet\python\_jsonnet_test.py", line 136, in test_import_binary_encode
      json_str = _jsonnet.evaluate_snippet(
  RuntimeError: RUNTIME ERROR: couldn't open import "binary123.bin": File not found
  	D:\a\jsonnet\jsonnet\python\testdata\test.jsonnet:1:1-26	
  
  
  ======================================================================
  ERROR: test_import_binary_sentinel_encode (__main__.JsonnetTests)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "D:\a\jsonnet\jsonnet\python\_jsonnet_test.py", line 145, in test_import_binary_sentinel_encode
      json_str = _jsonnet.evaluate_snippet(
  RuntimeError: RUNTIME ERROR: couldn't open import "binary1230123.bin": File not found
  	D:\a\jsonnet\jsonnet\python\testdata\test.jsonnet:1:1-30	
  
  
  ======================================================================
  ERROR: test_import_encode (__main__.JsonnetTests)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "D:\a\jsonnet\jsonnet\python\_jsonnet_test.py", line 118, in test_import_encode
      json_str = _jsonnet.evaluate_snippet(
  RuntimeError: RUNTIME ERROR: couldn't open import "trivial.jsonnet": File not found
  	D:\a\jsonnet\jsonnet\python\testdata\test.jsonnet:1:1-25	
  
  
  ======================================================================
  ERROR: test_import_no_eol_encode (__main__.JsonnetTests)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "D:\a\jsonnet\jsonnet\python\_jsonnet_test.py", line 127, in test_import_no_eol_encode
      json_str = _jsonnet.evaluate_snippet(
  RuntimeError: RUNTIME ERROR: couldn't open import "trivial_no_eol.jsonnet": File not found
  	D:\a\jsonnet\jsonnet\python\testdata\test.jsonnet:1:1-32	
  
  
  ----------------------------------------------------------------------
  Ran 10 tests in 0.625s
  
  FAILED (errors=5)
Error: Command python D:\a\jsonnet\jsonnet/python/_jsonnet_test.py

(Build action log: https://github.com/johnbartholomew/jsonnet/actions/runs/8087521792/job/22099656631)

@johnbartholomew
Copy link
Collaborator Author

I did the naive thing and just added an #if switch on _WIN32 to additionally look for backslash as a directory separator. We can consider switching to the C++17 lib, but I'd prefer to start with a smaller change like this that should be no worse than what we have right now.

In GitHub actions the Python test suite now passes on Windows: https://github.com/johnbartholomew/jsonnet/actions/runs/8098334188/job/22131487672

  + pip install 'C:\Users\runneradmin\AppData\Local\Temp\cibw-run-e8_jxk3n\cp312-win_amd64\repaired_wheel\jsonnet-0.20.0-cp312-cp312-win_amd64.whl'
  Processing c:\users\runneradmin\appdata\local\temp\cibw-run-e8_jxk3n\cp312-win_amd64\repaired_wheel\jsonnet-0.20.0-cp312-cp312-win_amd64.whl
  Installing collected packages: jsonnet
  Successfully installed jsonnet-0.20.0
  + python D:\a\jsonnet\jsonnet/python/_jsonnet_test.py
  ..........
  ----------------------------------------------------------------------
  Ran 10 tests in 0.505s
  OK

johnbartholomew and others added 7 commits March 23, 2024 14:49
The Makefile is problematic because it needs shell and the `od` command.
But Python setuptools is perfectly capable of building some .cpp files
on its own, so we can use that, it just needs a minor extension to
generate core/std.jsonnet.h.

Also update MANIFEST.in after the recent changes to third_party:
The bundled RapidYAML was switched to the single-header version, and
nlohmann json.hpp was moved to a subdirectory.
@johnbartholomew johnbartholomew merged commit cb18fec into google:master Mar 23, 2024
6 checks passed
@johnbartholomew johnbartholomew deleted the new-python-build branch March 23, 2024 15:02
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.

1 participant