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

Make use of pyproject.toml for static project metadata #1133

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

pelson
Copy link
Contributor

@pelson pelson commented May 2, 2023

Uses the PEP-518 pyproject.toml to declaratively define as much project metadata as possible, and keeps the project closer aligned with the current norms.

The upside of this approach is that we get declarative metadata, and can start to leverage other tools on top of pyproject.toml in very standard ways (e.g. we could be using setuptools-scm today, but with pyproject.toml, it becomes the defacto-documented approach to using setuptools-scm). In general, pyproject.toml is clearly the direction of travel in the Python community.

Some downsides:

  • It doesn't (and will never) remove the need for setup.py. If you python setup.py ... it might not pick up the metadata correctly (this invocation approach is deprecated and discouraged)
  • It isn't clear when metadata should be in pyproject.toml and when it should be in setup.py (gradually, more metadata components are moving to pyproject.toml)

setup.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b603ff6) 87.44% compared to head (b9472a3) 87.46%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
+ Coverage   87.44%   87.46%   +0.01%     
==========================================
  Files         113      113              
  Lines       10236    10236              
  Branches     4057     4057              
==========================================
+ Hits         8951     8953       +2     
+ Misses        692      691       -1     
+ Partials      593      592       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marscher
Copy link
Member

marscher commented May 2, 2023

For trivial (in the sense of building) projects, pyproject.toml covers all relevant metadata and tool configuration (like setup.cfg).

Since lot of effort went into the building system of JPype I'd discourage transitioning away from it, as long as we are not forced to.
I'm aware that the modern building system could be advantageous some day, but we'd need to analyze the benefits carefully.

As of the usage of only static metadata, I also currently discourage its use, because as you already noticed yourself you need to invoke the installation by pip so pyproject.toml will be used.

@pelson
Copy link
Contributor Author

pelson commented May 3, 2023

Since lot of effort went into the building system of JPype I'd discourage transitioning away from it, as long as we are not forced to.

I agree completely. The modern Python packaging tools are not entirely moving away from the setuptools/distutils concepts - Extensions still require a setup.py with setuptools, for example. This project will need specialisation of Extensions for a very long time to come (from what I understand there are no current proposals to do anything else in the packaging community).

The benefit is pretty low, but it essentially boils down to moving some of the config to a declarative form. In turn, this could have knock on benfits like having greater visibility of what the metadata actually declares (such as noticing a maintainer who retired 8 years ago 😜).

@pelson
Copy link
Contributor Author

pelson commented May 3, 2023

The benefit is pretty low, but it essentially boils down to moving some of the config to a declarative form

The other significant benefit that comes to mind is that we can properly declare build-time dependencies. Currently we don't do that, and assume that setuptools (our only build-time dependency at present) is installed already.

@pelson
Copy link
Contributor Author

pelson commented May 3, 2023

The one hold-up I have is that I am not confident that we can even use python -m build with the way we use the (deprecated) global options in setuptools (or is it distutils).

I will look into this now to figure out what the situation is, and then update here.

setupext/platform.py Dismissed Show dismissed Hide dismissed
@pelson pelson force-pushed the feature/pyproject branch from ae98aec to 7668593 Compare May 3, 2023 19:25
@pelson
Copy link
Contributor Author

pelson commented May 3, 2023

The one hold-up I have is that I am not confident that we can even use python -m build with the way we use the (deprecated) global options in setuptools (or is it distutils).

I will look into this now to figure out what the situation is, and then update here.

In the end, I implemented the recommended approach of adding per-command options. It means that I had to also implement the develop command so that we could support pip install --editable, but the outcome is that we can now use the modern packaging stack build and pip to do our builds/installs, and we get declarative metdata + build-time dependencies too.

All in all, I'm happy with the outcome, although it was more work than I'd planned.

@Thrameos
Copy link
Contributor

Thrameos commented May 3, 2023

Sorry about the general state of setupext. The state of python disttools was very poor when I worked on it and I was forced to work around a lot of broken stuff in Python 2 and Python 3.4/3.5.

The important parts that we need to maintain are

  • Packaging only the stuff needed
  • Ability for the user to compile a trace version.
  • Ability to write a makefile (because debugging this project is a huge pain and the lack of any incremental building in python packaging tools makes it a nightmare.)
  • The coverage/test builds

@pelson pelson force-pushed the feature/pyproject branch from d31631f to d424c80 Compare May 4, 2023 07:34
@pelson
Copy link
Contributor Author

pelson commented May 4, 2023

The important parts that we need to maintain are

  • Packaging only the stuff needed
  • Ability for the user to compile a trace version.
  • Ability to write a makefile (because debugging this project is a huge pain and the lack of any incremental building in python packaging tools makes it a nightmare.)
  • The coverage/test builds

Yes, all maintained as far as I understand.

Ability to write a makefile

I was going to ask about this option. In what way is it not incremental? This is the behaviour I see (it only re-compiles what setuptools/distutils thinks has changed). Indeed, I do see a problem with it being overly zealous with its caching and fails to pick up some things (like macro changes), at which point you have to --force it to recompile everything.

Furthermore, ccache works nicely too - this speeds up the case where a full re-compilation is needed.

It isn't hard to maintain the Makefile creation - I was just curious about its benefits.

@marscher
Copy link
Member

marscher commented May 4, 2023

👍 for the ccache ability. This is for instance broken in pip install (because the source is copied to a temporary location).

@Thrameos
Copy link
Contributor

Thrameos commented May 4, 2023

I haven't ever seen setup.py build anything other than completely from scratch. I was forced to move files to a directory called aaaa to test compiling, hence adding a makefile option. Perhaps there has been a change since 3.4.

What option do you use to get it to perform incremental builds?

@pelson
Copy link
Contributor Author

pelson commented May 4, 2023

What option do you use to get it to perform incremental builds?

It is the default, but it has always been the case (even before Python 2.6). It is possible that it is per extension - and since we only have one extension we don't get any benefit from it. I can double-double check though - I extensively use ccache to make the experience more manageable, so would see it if it wasn't working.

@pelson
Copy link
Contributor Author

pelson commented May 9, 2023

What option do you use to get it to perform incremental builds?

It seems you are right. airspeed-velocity/asv#201 (comment) highlights the problem (and potential solutions).

In the end, to get this working correctly, I had to disable build isolation (so you lose build-time dependency installation). When doing this, I get ccache hits, and a fast build:

pip install --editable ./ --no-build-isolation

I had initially thought this was related to the fact that ccache includes the CWD in its cache, and when you pip wheel it builds in an isolated (unique) location. It is possible to turn this off with the CCACHE_NOHASHDIR=1 environment variable (docs: https://ccache.dev/manual/latest.html#config_hash_dir). This works in non-editable mode with pip from what I can tell (so CC=ccache gcc CCACHE_NOHASHDIR=1 pip wheel /path/to/source).

It would be interesting to explore why the --no-build-isolation is necessary - I suspect the fact that our includes and libraries are being installed into a uniquely named environment. Some interesting details in pypa/pip#6505. Looks like setting debug-prefix-map in the underlying tool (perhaps setuptools) might be necessary... perhaps we could do something like that in our own build extensions... 🤔

For now, I think pip install --editable ./ --no-build-isolation and CCACHE_NOHASHDIR=1 pip wheel ./ are already fairly good options.

@Thrameos
Copy link
Contributor

Thanks for the insights. This is a problem I struggled with for a long time especially when I am debugging a complex issue.

I will likely continue to use the --makefile option for linux development as it is currently working. This new option if it works for Visual Studio should save me time on windows debugging. Not sure what the impact of no-build-isolation will be though when I am trying to debug in multiple architectures.

@pelson
Copy link
Contributor Author

pelson commented Feb 8, 2024

How to proceed on this one? I think it makes a good step forwards, but can understand that this is a bit of churn, and may lead to a few issues cropping up (not ideal, given the limited cycles available on the project). Having said that, I don't believe we are going to encounter anything that cannot be solved by the approach taken, and am willing to follow-up in such a situation.

setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
@marscher
Copy link
Member

@pelson If you feel like resolving the merge conflicts, I think we are ready to merge this.

@pelson
Copy link
Contributor Author

pelson commented Feb 13, 2024

I've rebased. I did a final check that iterative rebuilds are working well. My conclusions:

  • rebuild with no changes 👍 (2s)
  • rebuild with minor change 👎 (30s) [this is unchanged from master]
  • rebuild with minor change using ccache 👍 (2s)
  • rebuild using the Makefile 👍 (<2s) [using python setup.py build_ext --makefile; make]
  • rebuild using pip editable mode (<5s)

For the record, my ccache setup:

CC="ccache ${CC:-gcc}" CXX="ccache ${CXX:-g++}" python setup.py develop --enable-tracing

And

CC="ccache ${CC:-gcc}" CXX="ccache ${CXX:-g++}" pip install --editable ./ --config-setting="--install-option=--enable-tracing"

No ccache needed for the Makefile approach.

@marscher marscher merged commit 904fc43 into jpype-project:master Feb 13, 2024
25 checks passed
@marscher
Copy link
Member

Thanks for this modernization @pelson!

@pelson pelson deleted the feature/pyproject branch February 13, 2024 12:06
@Thrameos
Copy link
Contributor

It appears this change broke the jpython exe build I was working on because it removed the custom distribution object. Is there some replacement to add extra keywords to the distribution object so that I can pass those arguments through to the appropriate build script?

@pelson
Copy link
Contributor Author

pelson commented Feb 26, 2024

Sorry for the delay, was away last week.

Is there some replacement to add extra keywords to the distribution object so that I can pass those arguments through to the appropriate build script?

The options moved to the specific commands. Sounds like this would be an option to go on build_ext?

user_options = build_ext.user_options + [

@Thrameos
Copy link
Contributor

Unfortunately that didn't help with my efforts to construct a binary as I needed a new build type. For now I did the same thing that I did with the jar and just add more functionality to build_ext rather than setting up a new target for build_exe. The new system that Python is going for it a real downgrade from the old disttools. They seem to want all custom stuff to use thirdparty launchers. But that would be a huge challenge to support cross platform without installing some cross platform install tool where disttools provides the needed support.

Unfortunately, my jpython binary is stuck on a corrupt stack frame when the main is launched from Java. I will take another stab at it next week.

@Thrameos Thrameos mentioned this pull request Nov 10, 2024
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.

3 participants