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

Yet more more spkgs confs #36332

Merged
merged 16 commits into from
Oct 21, 2023
Merged

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Sep 25, 2023

a continuation of #36276 (more system Python packages support via --enable-system-site-packages)

Also, added a Cython 3.0.2 patch to fix the usage of --enable-system-site-packages option. Without it, and sufficiently many Python system packages, sagelib won't build. (Therefore, blocker label added)

@dimpase dimpase marked this pull request as draft September 25, 2023 14:51
@dimpase
Copy link
Member Author

dimpase commented Sep 25, 2023

need to add more distros/ stuff

@orlitzky
Copy link
Contributor

We could do cython now too. The standard spkg-configure.m4 works fine. I've only tested it with cython-3.x but we'll probably have that soon from #36110

@dimpase dimpase marked this pull request as ready for review September 26, 2023 07:51
@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 26, 2023

Can you please redo this branch with proper merge commits for its depencies?

@dimpase
Copy link
Member Author

dimpase commented Sep 26, 2023

by right, this branch should be based off develop, but it needs #36276, which is only in the "WIP-develop".
This gives me a headache. Anyhow, it's all in
https://github.com/dimpase/sage/tree/pr36332

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 28, 2023

I'd suggest to remove the additions to hatch_nodejs_version because #36129 will remove this package.

@dimpase
Copy link
Member Author

dimpase commented Sep 28, 2023

I'd suggest to remove the additions to hatch_nodejs_version because #36129 will remove this package.

let's not count chickens before they're hatched. If this gets it before #36129 I'll fix the latter branch, no problem

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 28, 2023

Or let's not lay eggs that are unwanted?

@dimpase
Copy link
Member Author

dimpase commented Sep 28, 2023

one cannot realistically scan all the open PRs and check for potential conflicts

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 28, 2023

I know, hence my helpful comment

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 30, 2023

@orlitzky In the platforms tests for --enable-system-site-packages, I see Cython compiler crashes while building sagelib, on many platforms.
https://github.com/mkoeppe/sage/actions/runs/6360039461/job/17277499571#step:12:58476

      Error compiling Cython file:
      ------------------------------------------------------------
      ...
      # distutils: libraries = NTL_LIBRARIES gmp
      ^
      ------------------------------------------------------------
  
      sage/rings/polynomial/polynomial_integer_dense_flint.pyx:1:0: Compiler crash in DebugTransform
  
  
      Compiler crash traceback from this point on:
        File "Cython/Compiler/Visitor.py", line 182, in Cython.Compiler.Visitor.TreeVisitor._visit
        File "/sage/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/Cython/Compiler/ParseTreeTransforms.py", line 4053, in visit_ModuleNode
          self.visit_FuncDefNode(nested_funcdef)
        File "/sage/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/Cython/Compiler/ParseTreeTransforms.py", line 4113, in visit_FuncDefNode
          self.tb.start(arg.name)
        File "/sage/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/Cython/Debugger/DebugWriter.py", line 42, in start
          self.tb.start(name, attrs or {})
        File "src/lxml/saxparser.pxi", line 841, in lxml.etree.TreeBuilder.start
        File "src/lxml/saxparser.pxi", line 769, in lxml.etree.TreeBuilder._handleSaxStart
        File "src/lxml/apihelpers.pxi", line 179, in lxml.etree._makeSubElement
        File "src/lxml/apihelpers.pxi", line 1753, in lxml.etree._tagValidOrRaise
      ValueError: Invalid tag name '.0'
      Traceback (most recent call last):
        File "/sage/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/Cython/Build/Dependencies.py", line 1325, in cythonize_one_helper
          return cythonize_one(*m)
        File "/sage/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/Cython/Build/Dependencies.py", line 1301, in cythonize_one
          raise CompileError(None, pyx_file)
      Cython.Compiler.Errors.CompileError: sage/rings/polynomial/polynomial_integer_dense_flint.pyx

Any thoughts?

@orlitzky
Copy link
Contributor

Any thoughts?

Looks like cython/cython#5552

@orlitzky
Copy link
Contributor

Unrelated: dev-python/stack_data was just renamed to dev-python/stack-data in Gentoo so now its distros/gentoo.txt needs an update.

@dimpase
Copy link
Member Author

dimpase commented Oct 2, 2023

calver is for setting up python package versions via date (CalendarVersion)
And, well, the version Sage installs is 2022.6.26
(same version as in Gentoo a.t.m.)

@orlitzky
Copy link
Contributor

orlitzky commented Oct 2, 2023

calver is for setting up python package versions via date (CalendarVersion) And, well, the version Sage installs is 2022.6.26 (same version as in Gentoo a.t.m.)

But why create a package-version.txt under distros/?

@dimpase
Copy link
Member Author

dimpase commented Oct 2, 2023

this happends when you put a bunch of .txt files in a wrong place, then move them to the right place, but...

@dimpase
Copy link
Member Author

dimpase commented Oct 2, 2023

Any thoughts?

Looks like cython/cython#5552

I tried patching Cython with the patch they provide, still getting this error.

PS. Sorry, it's OK - I forgot how to patch Gentoo packages, and the patch got lost...

@orlitzky
Copy link
Contributor

orlitzky commented Oct 2, 2023

PS. Sorry, it's OK - I forgot how to patch Gentoo packages, and the patch got lost...

I opened a Gentoo bug to backport the patch. Otherwise cython-3.0.3 should fix it, or --without-system-cython. I'm happy with this branch if Matthias has no more comments.

@dimpase
Copy link
Member Author

dimpase commented Oct 2, 2023

we'd also sort out these endless

Implicit noexcept declaration is deprecated. Function declaration should contain 'noexcept' keyword.

warnings, eventually

@github-actions
Copy link

Documentation preview for this PR (built with commit 89a6f9f; changes) is ready! 🎉

@orlitzky
Copy link
Contributor

I don't mind the cython patch but note that it won't help when cython also comes from the system (after this PR). The cython bug you patched is fixed in cython-3.0.3 (#36416), but cython-3.0.3 has a new problem (cython/cython#5748). Hopefully 3.0.4 comes soon.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 17, 2023
    
a continuation of sagemath#36276  (more system Python packages support via
`--enable-system-site-packages`)

Also, added a Cython 3.0.2 patch to fix the usage of `--enable-system-
site-packages` option. Without it, and sufficiently many Python system
packages, sagelib won't build. (Therefore, blocker label added)
    
URL: sagemath#36332
Reported by: Dima Pasechnik
Reviewer(s): Michael Orlitzky
@vbraun vbraun merged commit f89aaf1 into sagemath:develop Oct 21, 2023
24 of 34 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 21, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 26, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36332 (to remove the patch added there)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36416
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 28, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36332 (to remove the patch added there)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36416
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 28, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36332 (to remove the patch added there)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36416
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 29, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36332 (to remove the patch added there)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36416
Reported by: Matthias Köppe
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 29, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36332 (to remove the patch added there)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36416
Reported by: Matthias Köppe
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants