Skip to content

Add pyproject.toml, update installation instructions for modern Python packaging, fix docbuild, update CI scripts, add Cython 3 support #139

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 23 commits into from
Oct 6, 2023
Merged

Add pyproject.toml, update installation instructions for modern Python packaging, fix docbuild, update CI scripts, add Cython 3 support #139

merged 23 commits into from
Oct 6, 2023

Conversation

mkoeppe
Copy link

@mkoeppe mkoeppe commented Oct 4, 2023 •
edited
Loading

This PR started out with minimal changes, replacing the outdated use of setup_requires.

Fixes #93
(with a cleaner solution for augmenting the build-time path than the stalled PR #104).

videlec
videlec reviewed Oct 5, 2023
View reviewed changes
Copy link
Collaborator

@videlec videlec left a comment

Choose a reason for hiding this comment

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

Thanks.

_custom_build_meta.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you make the file name starts with an underscore here?

Copy link
Author

Choose a reason for hiding this comment

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

No reason, feel free to rename

Copy link
Author

Choose a reason for hiding this comment

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

Do you want me to just remove the underscore, or do you have other naming preferences?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing beyond following standard practice.

Copy link
Author

Choose a reason for hiding this comment

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

You're making it sound as if you think my PR was not following standard practice.

@dimpase
Copy link
Member

dimpase commented Oct 5, 2023

@videlec - what is the status of cypari2 vs Cython 3? You probably noticed that Sage has moved to Cython 3 (and it works)

dimpase
dimpase reviewed Oct 5, 2023
View reviewed changes
@dimpase
Copy link
Member

dimpase commented Oct 5, 2023 •
edited
Loading

@mkoeppe - can you add https://github.com/sagemath/sage/blob/develop/build/pkgs/cypari/patches/cython3-legacy.patch and drop the <3 constraint on Cython version added in c473052 ?

PS. OK, I've done it myself

@videlec
Copy link
Collaborator

videlec commented Oct 6, 2023

I think the workflow should include both Cython>=3 and Cython<3.

@dimpase
Copy link
Member

dimpase commented Oct 6, 2023

I think the workflow should include both Cython>=3 and Cython<3.

This would be a bigger change than this PR. A new issue?

@dimpase
Copy link
Member

dimpase commented Oct 6, 2023

@mkoeppe - what this ERROR: Could not find a version that satisfies the requirement setuptools (from versions: none)
stuff mean? I have no clue.

@dimpase
Copy link
Member

dimpase commented Oct 6, 2023

Does make build run an isolated Python environment, so that the previously run pip install has no effect?
I don't understand.

@dimpase
Copy link
Member

dimpase commented Oct 6, 2023

@mkoeppe - it doesn't fly. make build does not want to accept system-wide installed setuptools. full stop.

@dimpase
Copy link
Member

dimpase commented Oct 6, 2023

OK, we finally get a working build and testing, doc building is broken:

Running Sphinx v7.2.6
WARNING: Invalid configuration value found: 'language = None'. Update your configuration to a valid language code. Falling back to 'en' (English).
making output directory... done
WARNING: html_static_path entry '_static' does not exist
building [mo]: targets for 0 po files that are out of date
writing output... 
building [html]: targets for 7 source files that are out of date
updating environment: [new config] 7 added, 0 changed, 0 removed

Exception occurred:
  File "/home/runner/work/cypari2/cypari2/tvenv/lib/python3.11/site-packages/sphinx/ext/extlinks.py", line 108, in role
    title = caption % part
            ~~~~~~~~^~~~~~
TypeError: not all arguments converted during string formatting
The full traceback has been saved in /tmp/sphinx-err-dpid6l08.log, if you want to report the issue to the developers.

@dimpase
Copy link
Member

dimpase commented Oct 6, 2023

this is due to broken format strings in extlinks in docs/source/conf.py. fixing...

@dimpase
Copy link
Member

dimpase commented Oct 6, 2023

OK, builds/ytests/docs with Pari 2.15.4 pass, ones with Pari 2.11 and 2.13 take much more time, no idea why.

@mkoeppe
Copy link
Author

mkoeppe commented Oct 6, 2023

Let's get rid of make build. This makes no sense any more.

@mkoeppe
Copy link
Author

mkoeppe commented Oct 6, 2023

All done, please merge

@dimpase dimpase merged commit 7386a9f into sagemath:master Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimpase dimpase dimpase left review comments

@videlec videlec videlec left review comments

Assignees

@dimpase dimpase

@videlec videlec

@mkoeppe mkoeppe

Labels
None yet
Projects
None yet
Milestone
2.1.4
Development

Successfully merging this pull request may close these issues.

Needs pyproject.toml to declare build-time dependency on cysignals
3 participants
@mkoeppe @dimpase @videlec