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

Add pyproject.toml #471

Merged
merged 6 commits into from Dec 28, 2023
Merged

Add pyproject.toml #471

merged 6 commits into from Dec 28, 2023

Conversation

rossbar
Copy link
Contributor

@rossbar rossbar commented Jul 8, 2023

I took inspiration here from networkx/networkx#6774. I was particularly interested in testing out setuptools.build_meta as a build backend for binary extensions. In pygraphviz's case, this seems to have been entirely straightforward (on Linux at least). I think one of the main reasons why is that the build actually doesn't directly depend on SWIG - the SWIG-generated source files are actually tracked in the source repository, so SWIG is not actually required as a build dependency.

This means that updating SWIG doesn't actually have an effect on the package unless swig is actually run to re-generate the sources. This is something we can/probably should fix, but is a separate issue.

Aside from that, there are still plenty of other little TODO's (i.e. optional dependencies/templating pyproject.toml as was done in networkx/networkx#6774, etc.) but hopefully this is a decent starting point!

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0b466b6) 81.02% compared to head (be5b6f1) 81.02%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #471   +/-   ##
=======================================
  Coverage   81.02%   81.02%           
=======================================
  Files           5        5           
  Lines        1254     1254           
=======================================
  Hits         1016     1016           
  Misses        238      238           

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

@rossbar
Copy link
Contributor Author

rossbar commented Jul 8, 2023

The next major step is to figure out how best to workaround the deprecation of --global-options for builds on systems where the library path and/or header paths need to be specified (e.g. windows). One solution appears to be defining a custom backend - see e.g. python-pillow/Pillow#7171.

This feels like a lot of extra code to have to add for this feature... ideally the setuptools.backend_meta would add built-in support for these Extension options, but from my limited reading I can't get a good sense of what the current state of affairs is... more investigation required. FWIW this "just works" on the systems where graphviz is installed in a standard location with the necessary libraries/headers already installed in locations covered by LIBDIR and INCLUDEDIR.

@rossbar
Copy link
Contributor Author

rossbar commented Oct 3, 2023

Shoot... I just saw the other merge push and assumed it was just merge stuff so I rebased on main and force-pushed over it. Then it occurred to me that there may have been other, non-merge-conflict changes in there, in which case I just bulldozed them. Apologies if that was the case! I'm stepping AFK soon so feel free to push to this as necessary and sorry again if I overwrote changes!

@rossbar rossbar changed the title WIP: Add pyproject.toml Add pyproject.toml Dec 7, 2023
@rossbar
Copy link
Contributor Author

rossbar commented Dec 7, 2023

Okay I've rebased on main and this is now working on all platforms with the updated --config-settings flags from #484.

One of the most important things to double-check during review is that I ported over all the desired metadata from the setup.py file.

Copy link
Contributor

@jarrodmillman jarrodmillman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I will make a release candidate and double check that we didn't break anything or leave out any metadata.

@rossbar I will let you merge.

@jarrodmillman jarrodmillman merged commit c88c1e7 into pygraphviz:main Dec 28, 2023
19 checks passed
@jarrodmillman jarrodmillman added this to the 1.12 milestone Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants