-
-
Notifications
You must be signed in to change notification settings - Fork 768
Explicit build-time features #41550
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
base: develop
Are you sure you want to change the base?
Explicit build-time features #41550
Conversation
Following the discussion on, sagemath#41067 we add an option to defer (forthcoming) build-time feature checks to runtime.
Record the value of the "defer_feature_checks" meson option in the sage config file, for use by the sage/features subsystem.
|
Documentation preview for this PR (built with commit c90afeb; changes) is ready! 🎉 |
2b669f7 to
d045aee
Compare
d045aee to
5b776af
Compare
Implement Option 3 from the dissussion at sagemath#41067 with a new BuildFeature class. Features that can be detected at build-time should 1. Subclass this 2. Set self._enabled_in_build to a value from sage.config that says whether or not the feature was enabled at build time. 3. Implement is_present_at_runtime() if it makes sense to do so. (Without this, deferring the check to run-time won't help.)
Add foo_enabled variables for the existing features defined in meson.options that are detectable at build-time.
For all of the build-time features defined in meson.options, record whether or not the feature (and its dependencies) were found by meson. The associated config variables were added to src/sage/config.py.in in an earlier commit; here we just substitute the correct values.
5b776af to
f93d1a6
Compare
The meson build system is now capable of building sagelib without linking to libec, which means that the mwrank program may not be installed. Here we add a new feature to represent it. In particular this allows us to use "needs mwrank" in tests.
f93d1a6 to
cc97f87
Compare
The meson build system is now capable of building sagelib without sage.libs.eclib. Here we add a new feature to represent it. In particular this allows us to use "needs sage.libs.eclib" in tests.
|
The distro CI is hosed, but I think this works now and there are no major problems with the docs. Please play around and let me know your thoughts. The implementation was straightforward so far, but I haven't tried to convert any other runtime features yet, only the ones with existing meson options/checks. |
|
I would expect |
|
This doesn't look right: |
No problem, I didn't realize anyone was doing this yet. How is it implemented, are you packaging the optional python modules (like sage.libs.sirocco) separately? If so I'll put back the python module check as
Ugh, no, I'll take a look. I have a feeling that this was caused by changing |
For now I'm shipping everything together (and the features are enabled by installing the corresponding libraries), but I may move to split packages soon as our tooling is moving towards automatic detection of link libraries at packaging time. Since sage Features check for the modules loadability (as opposed to their presence), this works fine either way (currently). |
A trick is needed to dynamically add a method to an object.
Call Executable._is_present() instead of Executable.is_present() to avoid infinite recursion. Somehow this worked before I replaced the relative import of Executable with an absolute one.
The result of _is_present() is cached and we want to check for a module with a different name than that of the feature, so we create a new PythonModule on the fly for this.
The result of _is_present() is cached and we want to check for a module with a different name than that of the feature, so we create a new PythonModule on the fly for this.
The result of _is_present() is cached and we want to check for a module with a different name than that of the feature, so we create a new PythonModule on the fly for this.
The result of _is_present() is cached and we want to check for a module with a different name than that of the feature, so we create a new PythonModule on the fly for this.
The result of _is_present() is cached and we want to check for a module with a different name than that of the feature, so we create a new PythonModule on the fly for this.
The result of _is_present() is cached and we want to check for a module with a different name than that of the feature, so we create a new PythonModule on the fly for this.
The result of _is_present() is cached and we want to check for a module with a different name than that of the feature, so we create a new PythonModule on the fly for this.
The result of _is_present() is cached and we want to check for a module with a different name than that of the feature, so we create a new PythonModule on the fly for this.
The module name that we check for in this case is the same as the feature name, so we can subclass the feature from PythonModule and use PythonModule's _is_present() in is_present_at_runtime().
|
Ok, runtime detection probably works. Tested with the ones I happen to have installed (bliss, brial, eclib, rankwidth). |
|
(And the Mrwank bug is fixed.) |
|
Thanks. Besides the coxeter issue, everything works fine here with |
To detect coxeter3 at runtime, we should check for a module that won't be present when coxeter3 support is disabled. This feature now checks for a conditionally-compiled cython module beneath sage.libs.coxeter3, rather than for sage.libs.coxeter3 itself.
|
On 2026-01-31 11:31:11, Antonio Rojas wrote:
antonio-rojas left a comment (sagemath/sage#41550)
Thanks. Besides the coxeter issue, everything works fine here with `defer_feature_checks=true` (as in, same as before)
Coxeter should be fixed now too, thanks a lot for testing.
|
| # The build system installs the sage/libs/coxeter3 source code | ||
| # even when coxeter3 support is disabled, so a naive import of | ||
| # that module will actually succeed. We check for a | ||
| # conditionally-compiled cython module instead. |
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.
@antonio-rojas @orlitzky I probably misunderstood something in your discussion, but if you check here for a cython module that may be disabled by the coexeter dependency (in meson) - then why not simply embed via meson the value of coxeter.is_found here?
I was hoping we could replace the needs sage.libs.coexeter3 annotations by simply coexeter3 (as the cython module is only a proxy anyway, and it shouldn't really matter for the feature if it's a runtime or compile time dependency that is missing).
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.
That's the is_present_at_runtime() method and is only used if you build sage with -Ddefer_feature_checks=true. Otherwise, the "found" value from meson is used.
@antonio-rojas will build sage with all of the features enabled at build time but with feature checks deferred. (This essentially ignores the values recorded in the config file.) The resulting distro package won't have a hard dependency on coxeter3... if the user installs it, the deferred import sage.libs.coxeter3.coxeter3 will work; otherwise, it will just crash and the feature will be turned off at runtime. So even though it's a build requirement from the maintainer's perspective, the fact that the damage is limited to one compiled module lets us test for it at runtime with an import. But again, this is only for people who request it (and for the sage distro, for backwards compatibility).
This is controlled with needs coxeter3 by the way. There are others where the feature name includes the module name like eclib and brial, but there's no technical reason for that. Going back over my comments on #41042, it looks like I chose needs sage.libs.eclib to reflect what the feature is actually looking for. Though now that we are literally using the value of eclib.found() from the build system, that's less of a selling point.
| from sage.features.build_feature import BuildFeature | ||
|
|
||
|
|
||
| class Mwrank(BuildFeature, Executable): |
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.
Meson will now check for the presence of the mwrank executable in a certain list of paths and then enables/disables the feature here. But the Exectuable feature uses a different search algorithm and thus may not actually found the exe that meson was able to find (say you change PATH temporarily for building sage, but not for running it).
In the case of exes it would probably make more sense to embed the full path via meson, instead of using only a simple boolean toggle. What do you think?
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 thought of this at the time but meson doesn't actually check for mwrank yet. It should be easy to add later on with a subclass that returns getattr(sage.config, self.name + '_path') for the path.
| ``foo_enabled`` is written to ``sage.config``. In your subclass, | ||
| you should set the member variable ``_enabled_in_build`` to the | ||
| value of that config variable. |
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.
Just as an idea in the spirit of convention-over-configuration: you could query in _is_present below always the variable <name of feature>_enabled from sage.config. Then you don't have to set _enabled_in_build for each feature.
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 did this at first and talked myself out of it! With the current implementation, the downside to doing it by convention is that when the convention fails (e.g. mwrank, brial), you have to override the whole _is_present() method leading to duplicated code.
Given that you have to create a whole new subclass for every new feature and give it a name anyway, just spelling out the variable name in one line felt simpler.
Implement Option 3 from #41067.
defer_feature_checksthat defaults to false in meson, and true in the sage distro (for backwards compatibility). Record its value insage/config.py.meson.options, define variables insage/config.pystating whether or not they were enabled in the build.sage.features.build_feature.BuildFeatureclass to handle the processing.BuildFeaturein the sage features for things inmeson.options.sage -t.All permutations of the options should work, but for an example using the meson build,
meson setup -Dauto_features=disabled ...will disable everything at build-time, and not check for it again at run-time. You should be able to see the valuesfoo_enabled = 0insage/config.py. The feature objects (BuildFeaturesubclasses) will consult these variables to determine if the feature is enabled.An example of a feature that can be detected at runtime is the mwrank executable. In the current scenario, its
BuildFeaturecheckssage.config.eclib_enabled, so even if eclib is installed, the mwank executable will not be used. However, if you rebuild withmeson setup -Ddefer_feature_checks=true ..., then mwrank will be detected at run-time.The other features can also be detected at run-time, but they are linked to various python modules at build time. To test the runtime detection, one option is to uninstall the library that the feature uses (say, sirocco). This will cause the import test of sage.libs.sirocco to fail at runtime, leading to the feature being disabled. Reinstall sirocco and the feature should re-enable itself.
Relevant docs: