-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
sagemath-standard: include sage_setup in sdist #37287
Conversation
-1. sage-setup is declared as PEP 518 build requirement. Including a separate copy of a part of it in the sdist can only create confusion. |
No thanks, strong -1. |
You are right. I forgot to remove it from there. Done now. I've got used to build without |
I know you are having difficulties with this but I cannot really agree to that while there is a separate sage_setup package. It is really a kind of vendoring. And I spent some time organising stuff in Gentoo so those parts move smoothly together as separate packages. I really would need to remove the whole sage_setup as a separate package to rebalance thing. |
The issue is that sage_setup is not really a separate package. Building sagemath-standard is very, very intertwined with the exact version of sage-setup. Lots of changes happen in sync, refactoring from one to the other, etc. This means any change in one affects the other, etc. The situation would be different if sage-setup was a separate project with different release schedule (separate repo, etc) and with a clear-cut API between one and the other, but it doesn't really work like that. For a tiny example: can someone tell me how I can build sagemath-standard from #37270 using python 3.12? Given that #37270 changes metadata for both sagemath-standard and sage-setup to allow 3.12, it should be possible but it isn't. BTW, don't get me started with circular dependencies: sage-setup depends on sagemath-standard even if it's not declared afaict. |
The correct solution for this is
The modularized distributions sagemath-objects etc. already do the latter: https://github.com/sagemath/sage/blob/develop/pkgs/sagemath-objects/pyproject.toml.m4#L8 If you'd like to know why this correct solution is not yet implemented, you don't need to look much farther than #36951 |
Yes, sage_setup has some run time dependencies on sage to build sage stuff. Maybe some of the stuff from sage.misc.packagedir should be moved across. But sage is not a build time dependency of sage_setup at all. It will install quite fine without sage being there. |
In any case, this solves a real issue that I've raised, and nobody has shown any real downsides (as in: breaks this or that). |
As you know, that's not the standard that we use in this project. |
Please be respectful. |
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 think this is a great idea. sage_setup
is just a bunch of scripts with very little potential to be helpful to anyone outside of the sage-xyz
packages (eg who else would like to auto-generate interpreters?). Inlining it streamlines the installation and is eg a step towards pip installation with build isolation.
@tornaria Could you please also remove the sage-setup
mentions in the conda install docs and the conda ci. Otherwise this is good to go from my side.
The modularized distributions sagemath-* do. |
this is nonsense. |
Let me know if what I did works for you. I don't use conda myself. |
Documentation preview for this PR (built with commit 0c8d742; changes) is ready! 🎉 |
Could we please disable the spam bot? |
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.
Thanks, this works well indeed. CI is also passing, so good to go from my side!
Then please explain how |
I don't think you need to remove sage_setup as a separate package: it should not interfere with building sagelib / sagemath-standard. It will still be needed to build the split distributions. |
So we have a positive review (@tobiasdiez) and a -1 (@mkoeppe). Not sure how to count @kiwifb (neutral?) I believe negative reviews are not a thing, but we should still give Matthias the chance to:
Let's try to move forward in a way that satisfies everybody. |
@tornaria If your workflow has a problem with sage-setup being a "build-system requires" of sagemath-standard, then you are going to have the same problem with the modularization of the Sage library. |
@tornaria I previously asked on another ticket whether you can point us to policy documents in voidlinux that would help us understand where these rigid rules ("package A cannot build-depend on a specific version of package B") that you seem to be citing here and in similar tickets are coming from. This would be really helpful. |
@tornaria To elaborate: The standard in our project is to go towards best practices, not away from them. One such best practice in packaging is to ship the same file in exactly one distribution, not several. sagemath-standard of course already violates that, as it ships files that are also shipped by the modularized distributions sagemath-environment, sagemath-objects, etc. Making sage-setup a separate distribution and NOT ship it with sagemath-standard -- that was a move towards best practices, as it is needed by multiple distributions. The path forward is to remove the existing duplications, making sagemath-standard depend on the modularized distributions. In the branch of #35095 you can see it in its final form: sagemath-standard does not ship anything; it only depends on the modularized distributions. |
You seem to use the word "modularization" to mean the efforts to break the "batteries-included"ness of sage. Yes, by the efforts of 15 or so years, sage became to use system packages and system pip packages instead of carrying them. That is nice.
In this PR, the "modularization" should mean making distribution packages (=pip-installable packages) splitting the sage library. I meant conflicts with the design of this modularization. Obviously, this PR goes against with the design. There's no way to improve this PR to avoid the conflict. My question is whether the cause of your PR is worth to change the design. |
This comment was marked as abuse.
This comment was marked as abuse.
If you want to help the author, you better use this argument to recruit positive voters from sage-devel, instead of attacking negative voters. |
If there are two separate packages, the distro needs to build and test each one in sequence. The 1st package could be installed when the 2nd package is built, but we cannot require the 2nd package to be installed when we build the 1st one, as that would lead to a bootstrapping problem. In this case, we would do:
The 1st package could "checkdepend" on the 2nd package. But this would not work when 1st and 2nd package have to be updated in sync as is the case for sagemath.
Yes those two are problems of the current design. The issue also affects all versions 10.4.beta*. This is not unimportant. I do track prereleases and make draft distro packages for them (e.g., void-linux/void-packages#49571). This is quite important as it means I'm preemptively discovering issues and this is a great feedback loop for sagemath. I also build on git branches to test and for the PRs that affect build system is important that the way I build on a git checkout is identical to the way I build on the distro packages. As an example, let's look at jupyter, which is a valid comparison being very highly modularized:
No, that's not what I mean. Or rather, not only that. There's also lots of functionality that used to be part of sage that has been modularized into separate packages. Some core examples: cysignals, cypari2, etc. These are good examples: they are packages that were "modularized" out of sagemath, they have independent releases, they can be used and tested independently of sagemath, etc.
"Obviously" is a subjective word. Can you spell out in which way this PR conflicts or goes against "modularization"? Indeed, some of my ideas and opinions might conflict with "modularization". But I do respect the ideals of modularization, I made this PR taking that into account, and I really don't think there's any conflict here.
I can't argue with this since I still don't know what is the conflict. I don't think there's any conflict.
Again: I do think that the design should be open for a (technical) discussion, yes. But this PR is not that. |
We have similar problems in Gentoo but they affect the end users. Installing a package on Gentoo builds it from source and (optionally) tests it. If you can't test one package without the other, you have to install both without testing, and then reinstall them both with testing. That not only doubles the amount of time involved, but can possibly leave you with a broken package -- once you know that the tests fail, it's too late -- the new version is already installed. |
Right. I forgot to include that. |
I agree that these (testing, and testing betas) are important.
So the problem affects sagemath distro package on all distros if sagemath-standard sdist is used for the packaging. @mkoeppe I think that the above difficulty of packagers could be a cause enough to change the design of the modularization. I hope that having sage-setup as a separate distribution package is not essential in the design. Perhaps sage-setup can be included to every distribution packages. Would this work? |
I should clarify that I don't oppose the existence of a sage-setup (and I have not voted here, because Francois is the only one who has to deal with this at the moment). Lots of packages have build dependencies, and sage-setup is, in theory, just another build dependency. I mentioned how it works on Gentoo because I don't want you to get the idea that Gonzalo is doing something weird/wrong. Any time you have packages with circular dependencies it is a headache for packagers and users, and indicates that maybe those packages were not split optimally. |
@tornaria That's false. It's testable using just sagemath-repl, which provides the doctester. The whole talk about "circular dependencies" -- I already replied to it 2 months ago in #37287 (comment). In short: sage-setup does not needs sagemath-standard. It only needs sagemath-environment at runtime. (That that is not declared as a runtime dependency is simply because sage-setup still caters to the monolithic Sage library, in which sagemath-environment is not a thing.) |
He seems to mean the testing of the distro package |
Indeed.
In fact, I will echo what @orlitzky said: having sage-setup split is not a problem per se. It would be just fine to have sage-setup as a separate package, but a separate package has to be a separate package (a bit tautological, isn't it?). The sagemath source code is very large, and split in a large hierarchy of very many packages/modules. But for legacy reasons, this separation is far from perfect, in the sense that there is no clear separation of concerns, clear ownership, etc. and there are lots of interdependencies (explicit and implicit). This also affects sage-setup, which is the main reason why we can't use a fixed version of sage-setup to build several versions of sagemath-standard (we could, but then it would be difficult to work on the setup code without breaking something). The more I think on the issue, the more I lean towards the idea that the split of sage-setup was premature. A good split would be identifying the features and requirements of building sagemath that are general and make sense to split in a separate package, design and implement such a separate package (using the existing code, of course), and then switch sagemath building to that separate package. On the other hand, it seems setuptools is showing its limitations for building sagemath, and even with all the disagreements it seems that there is some kind of consensus that a move to meson migth be the best way. Then maybe investing much effort in sage-setup is really not worth it, and we can use this work to design a better meson build system that is suitable for building separate modules while being nice for distro packaging. Thus: can we please have this -- think of it as a temporary workaround? There might be some philosophical concern in the fact that we would have some code that is repeated in more than one sdist on pypi. However (a) the repeated code comes from a single source tree (no code duplication in that sense) and (b) the repeated code will not be shipped by any binary distribution other than sage-setup (avoiding any overlap or conflicts). Note: the sdist for sagemath-standard is I think about 19-20M, and adding sage-setup makes it maybe 100-200K larger. But the git repo tarball that I have to use to build is about 30M, so if source size is a concern this is actually better.
This sounds a little bit agressive. We are both making an effort to communicate in better terms, so let's try to keep it cool and learn to agree and disagree. You know what I mean. I'm not currently packaging sagemath-repl since that way of packaging is not ready and not working and, AFAIK, sagemath-repl and sagemath-standard do overlap in the binary distributions. Am I mistaken? |
You seem to be missing that doing this work -- figuring out where and how to split it, and implementing it -- is exactly what I have been doing over the past 4 years. |
As I now understand the situation, having this PR clears up some difficulty in preparing sagemath distro package from sagemath-standard sdist package, but breaks the design of the modularization. The broken part of the design seems "philosophical" or "getting away from the standard". We have to choose between this PR and the current design. Fortunately, moving to meson would clear up the packager's difficulty without getting away from the standard. I don't know the technical details, but all except me seem to understand this. Based on this, I now withdraw my vote. Consider my vote as 0. |
There's nothing wrong with providing a downstream package for sagemath-repl.
They do overlap currently, so in typical downstream packaging, you would mark them as mutual conflicts. |
e280cba
to
805206f
Compare
805206f
to
0c8d742
Compare
Setting back to positive review given the vote is 5-1: Withdrawn votes:
To avoid merge conflicts, I've rebased this PR on top of #37138 and #37796 (both with positive review). In both cases the merge was trivial but not automatic. The CI failures are unrelated. |
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> - Broken in sagemath#37287 - Reported in sagemath#37434 (comment) - Reported in sagemath#36524 (comment) ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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 and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38515 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> - Broken in sagemath#37287 - Reported in sagemath#37434 (comment) - Reported in sagemath#36524 (comment) ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [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 and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38515 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee
This makes it so that
sage_setup
is included insagemath_standard
for convenience and ease of installation.
Having
sage_setup
as a separate distribution leads to chicken-and-egg problem. For example, this is what happens when I try to buildsagemath-standard
from #37270:This is because the version of sage-setup which is available on pypi requires python <= 3.11. Even if the branch I'm trying to build allows python 3.12, I cannot use it.
The main idea of this change is that
sagemath-standard
sdists are self-contained and include the exact matching version of sage-setup. The self-contained part is good for the pypi sdist, but the matching version allows for greater flexibility when testing, given that the implementations ofsagemath-standard/setup.py
andsage_setup
are so intrincately connected and really need to match.📝 Checklist