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

Modernize Python metadata #58

Merged
merged 4 commits into from
Feb 20, 2024
Merged

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Feb 19, 2024

Using pyproject.toml

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 19, 2024

I had to comment out the rather unusual atexit.register(build_json_table) at the end of setup.py. This does not work in builds with build isolation (pip install . or python -m build .), for example because click is not available at build time.

pyproject.toml Outdated

[project]
name = "Mathics_Scanner"
version = "1.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

How can we this read in from `mathics_scanner/version.py?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait. I see it getting set below. So does it need to be set here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed in 104c114

setup.py Outdated
@@ -145,4 +107,4 @@ def build_json_table() -> int:
return result.returncode


atexit.register(build_json_table)
# atexit.register(build_json_table) ## does not work with build isolation
Copy link
Member

Choose a reason for hiding this comment

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

In setup we need to build some tables.

Can this be done in that dynamic section like it is for "version"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In setup we need to build some tables.

If users download the sdist (source tarball) from PyPI, would the tables be included there, or would they be built on the user's system?

Copy link
Member

@rocky rocky Feb 19, 2024

Choose a reason for hiding this comment

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

In setup we need to build some tables.

If users download the sdist (source tarball) from PyPI, would the tables be included there, or would they be built on the user's system?

Tables are included in PyPI tarballs, eggs, and wheel. This is done via https://github.com/Mathics3/mathics-scanner/blob/master/setup.py#L102-L104 along with Makefile targets which make sure files are around in development.

I think this was also added to setup.py as a way to generate tables as well if make isn't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was also added to setup.py as a way to generate tables as well

Yes, that's the one that I had to comment out.

I've just pushed a change (52fbe3d that does this in the egg_info phase of building an sdist (or wheel)

@rocky
Copy link
Member

rocky commented Feb 19, 2024

@mkoeppe Overall I like this PR and it is needed. To get to 3.12 we'll need do this not just here but also in mathics-core.

Note that the CI failure are not a problem of this PR, but a problem in master. That should now be fixed though.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 20, 2024

Note that the CI failure are not a problem of this PR, but a problem in master. That should now be fixed though.

Thanks. I've rebased on top of it

@rocky
Copy link
Member

rocky commented Feb 20, 2024

Here are diffs to make black happy:

index c7efc5b..b2f845a 100755
--- a/mathics_scanner/generate/build_tables.py
+++ b/mathics_scanner/generate/build_tables.py
@@ -15,7 +15,7 @@ try:
     from mathics_scanner.version import __version__
 except ImportError:
     # When using build isolation
-    __version__ = 'unknown'
+    __version__ = "unknown"
 
 
 def get_srcdir():
diff --git a/setup.py b/setup.py
index cf4dfd5..d0c7634 100644
--- a/setup.py
+++ b/setup.py
@@ -86,11 +86,15 @@ class table_building_egg_info(egg_info):
 
     def finalize_options(self):
         """Run program to create JSON tables"""
-        build_tables_program = osp.join(get_srcdir(), "mathics_scanner", "generate", "build_tables.py")
+        build_tables_program = osp.join(
+            get_srcdir(), "mathics_scanner", "generate", "build_tables.py"
+        )
         print(f"Building JSON tables via {build_tables_program}")
         result = subprocess.run([sys.executable, build_tables_program])
         if result.returncode:
-            raise RuntimeError(f"Running {build_tables_program} exited with code {result.returncode}")
+            raise RuntimeError(
+                f"Running {build_tables_program} exited with code {result.returncode}"
+            )
         super().finalize_options()

@rocky
Copy link
Member

rocky commented Feb 20, 2024

Looks Great - Many thanks!

@rocky rocky merged commit 864fe5d into Mathics3:master Feb 20, 2024
12 checks passed
@mkoeppe mkoeppe deleted the pyproject_metadata branch February 21, 2024 04:49
@mkoeppe mkoeppe mentioned this pull request Feb 21, 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.

2 participants