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

Add module sage.config as alternative to sage.conf package #37136

Closed
wants to merge 4 commits into from

Conversation

tobiasdiez
Copy link
Contributor

Add a fallback option src/sage/config.py to specify the runtime config (as an alternative to the sage.conf package). This should help with downsteam packaing and is part of #36524.

📝 Checklist

  • 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

@tobiasdiez
Copy link
Contributor Author

tobiasdiez commented Jan 22, 2024

This is ready for review.

@orlitzky @tornaria you might be interested in it.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 22, 2024

Why would reading from a package named sage.config instead of from a package named sage_conf be an improvement?

@orlitzky
Copy link
Contributor

Why would reading from a package named sage.config instead of from a package named sage_conf be an improvement?

I mentioned one reason briefly here: #36524 (comment)

Having a separate place to store meson-detected configuration allows us to migrate checks one-at-a-time from the distro ./configure script into meson, without changing the syntax of anything in sagelib. Once meson can detect everything itself (without distro ./configure and without the user manually typing in the correct values), we would then be able to remove sage_conf without breaking anything.

@tornaria
Copy link
Contributor

Why would reading from a package named sage.config instead of from a package named sage_conf be an improvement?

I don't see the point either. Runtime configuration should go into /etc/ or in ~/.config/sagemath, but mostly it should "just work" out of the box in a reasonably standard system.

For building, picking stuff from standard locations is ok; at most a lean easy way to configure alternate paths. AFAICT not even building the sagelib spkg inside sage-the-distro requires sage_conf (and possibly after #37024 sage-the-distro can run without any trace of sage_conf as well, and if not I'd consider it a bug of sagelib, which should support sage-the-distro at the same level as arch, conda, gentoo, void, etc).

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 22, 2024

b. Once meson can detect everything itself (without distro ./configure and without the user manually typing in the correct values), we would then be able to remove sage_conf

@orlitzky This certainly makes some sense, but I'd urge to take the modularization into account in the design.

The idea that there is one build step for the Sage library with integrated configuration, and one build-time generated configuration file that is read at runtime seems to hinge on having a monolithic Sage library provided by one Python distribution package.

@tornaria
Copy link
Contributor

Why would reading from a package named sage.config instead of from a package named sage_conf be an improvement?

I mentioned one reason briefly here: #36524 (comment)

Having a separate place to store meson-detected configuration allows us to migrate checks one-at-a-time from the distro ./configure script into meson, without changing the syntax of anything in sagelib. Once meson can detect everything itself (without distro ./configure and without the user manually typing in the correct values), we would then be able to remove sage_conf without breaking anything.

What do you need to manually type so that python -m build -nx works. Assuming the necessary distro devel packages are installed, of course, but I don't think meson can do that for me, can it?

@orlitzky
Copy link
Contributor

What do you need to manually type so that python -m build -nx works. Assuming the necessary distro devel packages are installed, of course, but I don't think meson can do that for me, can it?

We just saw an example recently: Fedora renames ecm to gmp-ecm. If you want to install sagelib on Fedora, you have to type that in to sage_conf yourself. Instead, meson should detect it and enter it into sage.config for you.

You might say that example is invalid or that fedora is stupid, but similar "relocations" happen all the time and pkg-config exists to make them easy.

@orlitzky
Copy link
Contributor

@orlitzky This certainly makes some sense, but I'd urge to take the modularization into account in the design.

The idea that there is one build step for the Sage library with integrated configuration, and one build-time generated configuration file that is read at runtime seems to hinge on having a monolithic Sage library provided by one Python distribution package.

Having a separate module for meson configuration would be an interim measure. Once meson can perform all of the required checks, it should just write the values into the python files directly like autoconf does with @FOO@. If each separate component performs its own meson checks, and if they all write the config values directly into the source files (rather than into sage.config), then there's no naming conflict between the various components.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 22, 2024

It's probably tangential to this discussion, but both names, ecm and gmp-ecm, could/should just be probed by the Executable feature at runtime. Of course a static configuration of the executable (perhaps at build time) is also a valid use case that may be favored in some deployment situations. We should design our configuration mechanisms so that all of them are supported with ease.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 22, 2024

If each separate component performs its own meson checks, and if they all write the config values directly into the source files (rather than into sage.config), then there's no naming conflict between the various components.

Where would these source files live, and what would be their relation to sage.config?

@tornaria
Copy link
Contributor

b. Once meson can detect everything itself (without distro ./configure and without the user manually typing in the correct values), we would then be able to remove sage_conf

@orlitzky This certainly makes some sense, but I'd urge to take the modularization into account in the design.

The idea that there is one build step for the Sage library with integrated configuration, and one build-time generated configuration file that is read at runtime seems to hinge on having a monolithic Sage library provided by one Python distribution package.

IMHO the idea of splitting sage library has proven itself too costly, both technically and socially. There is a very clear line to cut the project and reduce complexity, and that is split of the library from the rest of the world. I think there's ample consensus for this split. Let's finish it. All the other lines are kind of artificial and it all seems to end up adding complexity instead of reducing it. And it seems to be causing a lot of social and technical friction.

@orlitzky
Copy link
Contributor

Where would these source files live, and what would be their relation to sage.config?

I don't think I understand the question. They could live anywhere, either in a separate repo, or in the monorepo. So long as they have their own meson-based build systems that can make @VARIABLE@ substitutions, everything is fine. At this point sage.config is obsolete and has already been removed.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 22, 2024

@tornaria It's OK if you're not interested in the modularization.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 22, 2024

Where would these source files live, and what would be their relation to sage.config?

I don't think I understand the question. They could live anywhere, either in a separate repo, or in the monorepo. So long as they have their own meson-based build systems that can make @VARIABLE@ substitutions, everything is fine. At this point sage.config is obsolete and has already been removed.

OK, then why would we introduce the monolithic sage.config in the first place?

@orlitzky
Copy link
Contributor

OK, then why would we introduce the monolithic sage.config in the first place?

As an interim measure, so that every ./configure check doesn't have to be ported to meson at once. In the meantime, they can be added one-at-a-time to sage.config without changing how they are accessed.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 22, 2024

I think we can come up with a solution that aligns with the 0-1-infinity rule

@tornaria
Copy link
Contributor

It's probably tangential to this discussion, but both names, ecm and gmp-ecm, could/should just be probed by the Executable feature at runtime. Of course a static configuration of the executable (perhaps at build time) is also a valid use case that may be favored in some deployment situations. We should design our configuration mechanisms so that all of them are supported with ease.

Seems reasonable, we talked about this in the other PR. I looked a bit into this, but the code is not very well organized, it would take some effort. It seems to need a good discussion / brainstorming / design before we keep coding the thing with differing styles and goals. I don't like that to implement a feature (which is, after all, a small piece of meta-configuration) requires so much boilerplate (and useless-looking doctests which are there just to keep the burocracy straight) It may be worth looking if there's another project with similar needs, or an independent python package that suits us.

@tobiasdiez
Copy link
Contributor Author

Why would reading from a package named sage.config instead of from a package named sage_conf be an improvement?

Because it's easier to add a file under the sage folder (either by patching sagelib or by using a build system such as meson that can generate such a file as part of the build step).

@tobiasdiez
Copy link
Contributor Author

Can we get this in please? I think we all agree that the config should be removed in the long term, but until then this convenience fall-back doesn't hurt.

@orlitzky
Copy link
Contributor

It might be nice to add an empty sage.config to start, with documentation at the top explaining why the module exists?

If we're on the sage page: it's a sage-conf replacement to hold the configuration detected by the (new) dedicated sagelib build system. It allows us to port ./configure checks one at a time to meson, storing the detected variables in a way that can still be accessed through sage.env, i.e. without having to change anything else in sagelib. (Ultimately it too may become obsolete if the detected values can be substituted directly into the code, bypassing the sage.env lookup -- but that's a separate project.)

I'm in favor of this, but I don't think it makes sense to merge it until there's a resolution to the "disputed" on #36524

@tobiasdiez
Copy link
Contributor Author

It might be nice to add an empty sage.config to start, with documentation at the top explaining why the module exists?

In principle a good idea, but then a bit more work may be needed to handle the "empty but present case" (currently we can just handle the absence via ImportError see https://github.com/sagemath/sage/pull/37136/files#diff-45c63bcc74fb1cd2c0781892587774cffa3bd405d659ec188dee4afe0d583f59R152)

Copy link

github-actions bot commented Feb 25, 2024

Documentation preview for this PR (built with commit 30021de; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@orlitzky orlitzky mentioned this pull request Mar 15, 2024
5 tasks
@dimpase
Copy link
Member

dimpase commented Mar 18, 2024

It's probably tangential to this discussion, but both names, ecm and gmp-ecm, could/should just be probed by the Executable feature at runtime. Of course a static configuration of the executable (perhaps at build time) is also a valid use case that may be favored in some deployment situations. We should design our configuration mechanisms so that all of them are supported with ease.

Seems reasonable, we talked about this in the other PR. I looked a bit into this, but the code is not very well organized, it would take some effort. It seems to need a good discussion / brainstorming / design before we keep coding the thing with differing styles and goals. I don't like that to implement a feature (which is, after all, a small piece of meta-configuration) requires so much boilerplate (and useless-looking doctests which are there just to keep the burocracy straight) It may be worth looking if there's another project with similar needs, or an independent python package that suits us.

I don't think anything like this should be checked at runtime. ecm/gmp-ecm should be recognised at config/build time, and then assumed to be present (if one of these is present). If ecm/gmp-ecm was present, but is now gone, Sage should just be allowed to error out.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 18, 2024

don't think anything like this should be checked at runtime. ecm/gmp-ecm should be recognised at config/build time, and then assumed to be present (if one of these is present). If ecm/gmp-ecm was present, but is now gone, Sage should just be allowed to error out.

Whenever we read such declarations, it seems that lots of assumptions have been made about the deployment scenario.

@tobiasdiez
Copy link
Contributor Author

No longer needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants