-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
build/pkgs/setuptools_scm
: Update to 8.0.4, add fixes for version 8
#36400
Conversation
0ff738d
to
8361771
Compare
build/pkgs/setuptools_scm
: Update to 8.0.4build/pkgs/setuptools_scm
: Update to 8.0.4, add fixes for version 8
I would consider those warnings an upstream bug. The way you word it ("unsuspecting"), I almost feel like having a conversation on consent in software packaging. |
Offering these setuptools entry points that modify other packages' build behavior has always seemed like a severe & puzzling misdesign to me, but I may be missing something. In what is now the default in Python dev, using build isolation, it's not a problem any more -- each package just declares its PEP517/518 build environment, and if it does not like to use setuptools_scm, it just doesn't use it. But in flat installations as in Linux distributions, the problem persists. |
I was a bit sarcastic. And I have not looked for that issue in my logs so I cannot tell if it is a big problem. But if if the configuration bits is missing, may be it should be a signal that your tool is not required and that's the end of that. There is plenty of software available on a computer and they do not all jump and down saying "use me! use me!". What's wrong with some people. |
Documentation preview for this PR (built with commit 114a610; changes) is ready! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand the motivation / problem in the context of conda. We don' use setuptools_scm
so why would we deliberately install it in the conda environment, especially if it causes problems? I'm also not aware that I've seen this error before, so it feels more like an issue with the implementation in #35593.
What's particular about conda (in contrast to normal modern python dev) is that one commonly uses
We wouldn't ideally, but users may (for example and in particular when people switch between different ways of using the same conda environment). It is important that just installing this package does not break stuff. |
Can't we get rid of the special treatment of Conda (in particular |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving this for the time being, but in general this looks like a less than ideal design.
But maybe it is sufficient to voice an warning (during configure) that some doctest may break when setuptools_scm is present in the conda environment. If one has more packages installed in the conda env than what we specify, it is logical that this may cause issues. And the setup_scm failure doesn't seem to be that bad. |
How is this not applicable to the |
I guess it is. And if you install sagelib with make it also uses the switch: sage/build/bin/sage-dist-helpers Lines 291 to 297 in b7c1c8f
As far as I know pip doesn't have a switch to "use build isolation, but take build system dependencies from the global environment if they meet the requirements". |
That's right. It's all or nothing. The isolated build environment is created from scratch; it never takes anything from the site packages (user or system). The only way to control it is via pip's general options regarding index servers. This is what we do when building packages in the Sage distribution; we disallow the use of the PyPI index server ( |
Needs rebasing. The CI failures seem unrelated, I'll join Dima in saying we can move forwards with this. |
it merges cleanly; not a good idea to merge/rebase when it's already in positive review |
Cool, I was interpreting github's message a little bit more severely. |
…ixes for version 8 <!-- ^^^^^ 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? --> Version 8 needs some new workarounds when packages that do not use `setuptools_scm` are installed with `--no-build-isolation` in an environment with `setuptools_scm`. This is relevant in particular for conda, see sagemath#35593. `setuptools_scm` inserts itself into the build process of unsuspecting packages via entry points declared by setuptools. One gets a warning: - `[sage_conf-10.2.beta5] WARNING setuptools_scm._integration.setuptools pyproject.toml does not contain a tool.setuptools_scm section` (https://github.com/sagemath/sage/actions/r uns/6409373032/job/17400573034?pr=36400#step:8:3978) These warnings show up at build time, but also in runtime uses of Cython, where it causes doctest failures. Here we do the update (for no specific reason other than to be up to date) and implement the workarounds. <!-- 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36400 Reported by: Matthias Köppe Reviewer(s): Dima Pasechnik, Tobias Diez
<!-- ^^^^^ 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 --> Since 10.2.beta8, likely as a side effect of the setuptools_scm update (sagemath#36400), some of our modularized sdists contain way too many files. https://pypi.org/project/sagemath-bliss/10.2b8/#files We fix this by updating the MANIFEST.in files. - Cherry-picked from sagemath#35095. <!-- 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. - [x] The description explains in detail what this PR is about. - [x] 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36520 Reported by: Matthias Köppe Reviewer(s): François Bissey
<!-- ^^^^^ 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 --> Since 10.2.beta8, likely as a side effect of the setuptools_scm update (sagemath#36400), some of our modularized sdists contain way too many files. https://pypi.org/project/sagemath-bliss/10.2b8/#files We fix this by updating the MANIFEST.in files. - Cherry-picked from sagemath#35095. <!-- 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. - [x] The description explains in detail what this PR is about. - [x] 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36520 Reported by: Matthias Köppe Reviewer(s): François Bissey
Instead of suppressing the warning, it now shows up in the doctests leading to build errors (also can be seen here on this PR: https://github.com/sagemath/sage/actions/runs/6412079178/job/17409624223). Could you please fix it in a follow-up? |
I honestly think this is an upstream bug. "WARNING" is not the right log level for telling you you are not using an optional feature. "INFO" would be more appropriate. |
I agree. Nevertheless we should work around it in our runtime use of Cython. |
Indeed this happens when the doctester is invoked from the |
<!-- ^^^^^ 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 --> Since 10.2.beta8, likely as a side effect of the setuptools_scm update (sagemath#36400), some of our modularized sdists contain way too many files. https://pypi.org/project/sagemath-bliss/10.2b8/#files We fix this by updating the MANIFEST.in files. - Cherry-picked from sagemath#35095. <!-- 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. - [x] The description explains in detail what this PR is about. - [x] 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36520 Reported by: Matthias Köppe Reviewer(s): François Bissey
…etuptools_scm <!-- ^^^^^ 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? --> The part of sagemath#36400 that was intended to fix the runtime use of cython did not actually fix it because `finalize_distribution` is called already by `Distribution.__init__`. This shows up when invoked from a directory with `pyproject.toml` or a similar config file. As the Build & Test workflow invokes the doctester from `SAGE_ROOT/src`, these errors show up in all tests. Marking it as a blocker so it is [applied automatically in CI workflows](https://github.com/sagemath/sage/wiki/Sage-10.2-Release- Tour#open-blocker-prs-are-applied-automatically-in-ci-workflows) <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> Fixes sagemath#36400 (comment) <!-- 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. - [x] 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36552 Reported by: Matthias Köppe Reviewer(s): Tobias Diez
…etuptools_scm <!-- ^^^^^ 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? --> The part of sagemath#36400 that was intended to fix the runtime use of cython did not actually fix it because `finalize_distribution` is called already by `Distribution.__init__`. This shows up when invoked from a directory with `pyproject.toml` or a similar config file. As the Build & Test workflow invokes the doctester from `SAGE_ROOT/src`, these errors show up in all tests. Marking it as a blocker so it is [applied automatically in CI workflows](https://github.com/sagemath/sage/wiki/Sage-10.2-Release- Tour#open-blocker-prs-are-applied-automatically-in-ci-workflows) <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> Fixes sagemath#36400 (comment) <!-- 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. - [x] 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36552 Reported by: Matthias Köppe Reviewer(s): Tobias Diez
<!-- ^^^^^ 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 --> Since 10.2.beta8, likely as a side effect of the setuptools_scm update (sagemath#36400), some of our modularized sdists contain way too many files. https://pypi.org/project/sagemath-bliss/10.2b8/#files We fix this by updating the MANIFEST.in files. - Cherry-picked from sagemath#35095. <!-- 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. - [x] The description explains in detail what this PR is about. - [x] 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36520 Reported by: Matthias Köppe Reviewer(s): François Bissey
…etuptools_scm <!-- ^^^^^ 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? --> The part of sagemath#36400 that was intended to fix the runtime use of cython did not actually fix it because `finalize_distribution` is called already by `Distribution.__init__`. This shows up when invoked from a directory with `pyproject.toml` or a similar config file. As the Build & Test workflow invokes the doctester from `SAGE_ROOT/src`, these errors show up in all tests. Marking it as a blocker so it is [applied automatically in CI workflows](https://github.com/sagemath/sage/wiki/Sage-10.2-Release- Tour#open-blocker-prs-are-applied-automatically-in-ci-workflows) <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> Fixes sagemath#36400 (comment) <!-- 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. - [x] 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36552 Reported by: Matthias Köppe Reviewer(s): Tobias Diez
<!-- ^^^^^ 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 --> Since 10.2.beta8, likely as a side effect of the setuptools_scm update (sagemath#36400), some of our modularized sdists contain way too many files. https://pypi.org/project/sagemath-bliss/10.2b8/#files We fix this by updating the MANIFEST.in files. - Cherry-picked from sagemath#35095. <!-- 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. - [x] The description explains in detail what this PR is about. - [x] 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36520 Reported by: Matthias Köppe Reviewer(s): François Bissey
…etuptools_scm <!-- ^^^^^ 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? --> The part of sagemath#36400 that was intended to fix the runtime use of cython did not actually fix it because `finalize_distribution` is called already by `Distribution.__init__`. This shows up when invoked from a directory with `pyproject.toml` or a similar config file. As the Build & Test workflow invokes the doctester from `SAGE_ROOT/src`, these errors show up in all tests. Marking it as a blocker so it is [applied automatically in CI workflows](https://github.com/sagemath/sage/wiki/Sage-10.2-Release- Tour#open-blocker-prs-are-applied-automatically-in-ci-workflows) <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> Fixes sagemath#36400 (comment) <!-- 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. - [x] 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36552 Reported by: Matthias Köppe Reviewer(s): Tobias Diez
…agemath#36435 <!-- ^^^^^ 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 --> This repairs the installation method described in https://github.com/sagemath/sage#alternative-installation-using-pypi broken by changes in sagemath#36400, sagemath#36435. We also expand the tests for this distribution in its tox.ini. Run `(cd pkgs/sage-conf_pypi && tox -v -v -e py311)` <!-- 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: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36533 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee
Version 8 needs some new workarounds when packages that do not use
setuptools_scm
are installed with--no-build-isolation
in an environment withsetuptools_scm
. This is relevant in particular for conda, see #35593.setuptools_scm
inserts itself into the build process of unsuspecting packages via entry points declared by setuptools. One gets a warning:[sage_conf-10.2.beta5] WARNING setuptools_scm._integration.setuptools pyproject.toml does not contain a tool.setuptools_scm section
(https://github.com/sagemath/sage/actions/runs/6409373032/job/17400573034?pr=36400#step:8:3978)These warnings show up at build time, but also in runtime uses of Cython, where it causes doctest failures.
Here we do the update (for no specific reason other than to be up to date) and implement the workarounds.
📝 Checklist
⌛ Dependencies