New feature for eclib/mwrank #41042
New feature for eclib/mwrank #41042
Conversation
|
Documentation preview for this PR (built with commit 121c52e; changes) is ready! 🎉 |
| spkg='sagemath_ntl', type='standard') | ||
|
|
||
|
|
||
| class sage__libs__eclib(JoinFeature): |
There was a problem hiding this comment.
I don't think conditionalizing on imports in sagelib is a good idea: when the import fails, all tests that would otherwise have notified us about the import problem are disabled. (This actually came up recently in the context of bliss, where it was not clear if the tests just 'succeed' because they were disabled.)
So I would prefer if you could directly conditionalize on eclib, using the detection with meson. So either write a eclib_is_present toggle in sage.config or via a new sage/features/eclib.py.in (which then has a constant 'presence' state).
There was a problem hiding this comment.
I think this is the way to go, but I also think we should all think this through (in a GH discussion?) first. It would make more sense, especially for the features in meson.options, to do them all at once. There are at least three classes of feature that we need to think about:
- Executables (mwrank, but also e.g. ffmpeg)
- Internal optional cython modules (sage.libs.eclib)
- External cython modules (sagemath_giac)
For executables, I definitely want the ability to pass -Dffmpeg=disabled, and have the corresponding feature disabled. I don't want it detected at runtime and then failing because (say) animated gif support isn't there, and nobody copied that check to sage/features/ffmpeg.py. (I also simply don't want to waste time testing something I'm never going to use.) That -Dffmpeg=enabled should do the opposite is fairly straightforward. Where it gets tricky is that not everyone wants to recompile sage just to detect /usr/bin/ffmpeg. On binary distros that isn't an option, and people will probably complain if we take away the ability. We do have -Dffmpeg=auto, but that means something different, and we probably don't want to redefine standard meson terms. We could have a global option to override any "disabled" features at run-time. In any case, doing the detection at runtime (even optionally) does mean that we need to duplicate every configure-time check into the corresponding sage feature, so there are trade-offs to supporting this.
For internal cython modules, there's really no other way to obtain the module than to rebuild sage, so being able to detect them on-the-fly doesn't really help. Although I guess it's possible that some distro maintainer is building all of sage and then putting sage/libs/eclib into a separate tarball. It probably depends on how independent the module is. eclib is more tightly integrated than bliss, coxeter, and sirocco, for example.
External cython modules are the easiest case IMO. Right now sagemath_giac is the main example, but there's no reason (other than lack of time) we couldn't do the same thing with e.g. sagemath_bliss. What's nice about these is that it solves the runtime detection problem in a very simple way. If you want support for giac in sage, you install sagemath_giac. If you don't, you don't. We can detect it at runtime without side effects because, in this very special case, there's no other reason for you to have sagemath_giac installed. This is in contrast with (say) ffmpeg that I have installed for other reasons. tl;dr in this case runtime detection is fine. Being able to force-disable or force-enable the feature would be a bonus, but these aren't our main problem.
| sage: isinstance(Mwrank(), Mwrank) | ||
| True | ||
| """ | ||
| Executable.__init__(self, 'mwrank', executable='mwrank', |
There was a problem hiding this comment.
Does it make sense to move this runtime check to a compile time meson check?
There was a problem hiding this comment.
See above. Yes, but we should either (a) have a plan for how to leave it detectable at runtime, since this is easy to install after sage is installed, or (b) decide as a group that we don't want to detect it at runtime, so that I don't get yelled at for breaking something without community input.
|
Seems it is OK. Can you merge the lastest dev branch? |
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 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.
If the feature for sage.libs.foo is enabled, we shouldn't skip over files that fail to collect with an ImportError for sage.libs.foo.
When sage.libs.eclib doesn't exist, pytest will still try to collect *.py files that import it, leading to an ImportError. We add eclib to the list of feature-backed modules with workarounds for this issue.
orlitzky
force-pushed
the
eclib-feature-only
branch
from
6676a62 to
121c52e
Compare
Sure. There's an ongoing discussion in #41067 about how to handle these features in the long run, and while there doesn't seem to be much controversy, I don't think it hurts to merge this as-is (to get it out of the way) and then update it later on with the others. |
|
Yes, I think we can go ahead with this PR and then later revisit all of the dependencies. But could you still please rename the feature |
OK, let me think about this for a day. The meson flags are part of the UI and ideally would not have names like |
|
My reasoning was that the relevant dependency is |
I see what you're saying... if at all possible, we should avoid copy/pasting features that are ultimately enabled or disabled as a group. But here is where I get caught up: with gap we have an executable that is needed by the pexpect interface, and then also libgap used by sagelib itself. In sage/interfaces/gap.py we would have |
3 participants
eclib and mwrank are now optional for sagelib, but without them, the tests fail. This PR addresses the easy part of that problem, by cherry-picking the new features and pytest tweaks from #40546
I will make #40546, which is a review nightmare, depend on this one.