#27635 closed enhancement (fixed)
Allow tests that are specific to Sage's build system to be skipped
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.8 |
Component: | doctest framework | Keywords: | |
Cc: | arojas, fbissey, gh-timokau, jhpalmieri | Merged in: | |
Authors: | Erik Bray | Reviewers: | François Bissey |
Report Upstream: | N/A | Work issues: | |
Branch: | 2533f32 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
Adds an 'optional' tag called 'build' (name to be bikeshedded if necessary) specifically for tests related to sage-the-distribution and Sage's build system.
This allows downstream distributions that package Sage differently to skip these tests, but they are enabled by default when the tests are run from a build in Sage's source repository.
This provides a possible solution to #27621.
Change History (25)
comment:1 Changed 2 years ago by
- Cc arojas added
comment:2 Changed 2 years ago by
- Branch set to u/embray/doctest/optional-build
- Cc fbissey gh-timokau jhpalmieri added
- Commit set to d4ba44467b3ea398e297f6a7e1f83108fe600c3b
- Status changed from new to needs_review
comment:3 Changed 2 years ago by
- Commit changed from d4ba44467b3ea398e297f6a7e1f83108fe600c3b to 76fae85020e052d6c732aedbea76672ed4b33183
comment:4 follow-up: ↓ 6 Changed 2 years ago by
Thank you! The only possible issue I can see with this (and the reason why I still create empty dummy files in installed
) is that people may use is_package_installed
in scripts to gate certain features.
They really *should* be using the features
feature ( :) ) for that though, so maybe that's okay. Definitively net positive overall.
comment:5 follow-up: ↓ 7 Changed 2 years ago by
Other tests that should have this flag:
- The
os.path.isfile(started_file)
test in sage/all.py:288 - Tests that involve
sage --root
andsage --info
in tests/cmdline.py:198-234
comment:6 in reply to: ↑ 4 ; follow-up: ↓ 9 Changed 2 years ago by
Replying to gh-timokau:
Thank you! The only possible issue I can see with this (and the reason why I still create empty dummy files in
installed
) is that people may useis_package_installed
in scripts to gate certain features.
That's interesting... One thing that is definitely on my todo list, though I have no idea when I'll get to it, is to have an easy way for distros to plug into sage's configure
script, so that it can use the distro's packaging system to check for the required dependencies, and possibly also output these files (although they are of less interest in that case).
They really *should* be using the
features
feature ( :) ) for that though, so maybe that's okay. Definitively net positive overall.
Yep!
comment:7 in reply to: ↑ 5 Changed 2 years ago by
Replying to arojas:
Other tests that should have this flag:
- The
os.path.isfile(started_file)
test in sage/all.py:288- Tests that involve
sage --root
andsage --info
in tests/cmdline.py:198-234
Thank you for pointing them out. I'll add them.
comment:8 follow-up: ↓ 11 Changed 2 years ago by
Right now the doctesting flags are set both in the main Makefile and in sage-runtests
. Should the same happen here? At #27124, @gh-timokau suggested not even trying to guess whether to use the build
flag but just make it the default, for example via the existing variable TESTALL_FLAGS
. Then people can modify that variable as they like, perhaps as they already do to omit dochtml
.
The build
flag should probably also be documented in the parser help in sage-runtests
: if "build" is listed, will also run the tests relying on Sage's packaging system
or something like that.
comment:9 in reply to: ↑ 6 Changed 2 years ago by
Replying to embray:
Replying to gh-timokau:
Thank you! The only possible issue I can see with this (and the reason why I still create empty dummy files in
installed
) is that people may useis_package_installed
in scripts to gate certain features.That's interesting... One thing that is definitely on my todo list, though I have no idea when I'll get to it, is to have an easy way for distros to plug into sage's
configure
script, so that it can use the distro's packaging system to check for the required dependencies, and possibly also output these files (although they are of less interest in that case).
Similarly I have is_package_installed
returning False
in sage-on-gentoo so as not to remove it everywhere. That used to be a big patch. I only remove it from code where Gentoo can offer a replacement.
They really *should* be using the
features
feature ( :) ) for that though, so maybe that's okay. Definitively net positive overall.Yep!
Yes at runtime definitely.
At configure time it is still used for optional bindings that have code. At the moment I replace it by querying environment variables in sage_setup/optional_extension.py
. If it could be replaced by a setup.cfg
that could be generated by configure
- or the package building system - that would be great. On the other hand that would mean people couldn't just do a sage -i ....
followed by sage -b
to rebuild for the package they have just added.
Lastly the doctesting system relies on is_package_installed
to figure some of the options you have enabled when passed --optional=all
. I just cannot think of how to replace that.
comment:10 Changed 2 years ago by
- Commit changed from 76fae85020e052d6c732aedbea76672ed4b33183 to a01588849d4bde2d3036a82a61b9f6f5f79fddd7
Branch pushed to git repo; I updated commit sha1. New commits:
a015888 | Trac #27635: Skip more tests that may not make sense outside a Sage build from
|
comment:11 in reply to: ↑ 8 Changed 2 years ago by
Replying to jhpalmieri:
Right now the doctesting flags are set both in the main Makefile and in
sage-runtests
. Should the same happen here? At #27124, @gh-timokau suggested not even trying to guess whether to use thebuild
flag but just make it the default, for example via the existing variableTESTALL_FLAGS
. Then people can modify that variable as they like, perhaps as they already do to omitdochtml
.
I think I see what you're saying. I'll add it to TESTALL_FLAGS
: It would have to be since otherwise it is only appended if running sage -t
directly without explicitly providing an --optional
flag. I think the default guessing behavior is pretty good as-is though unless someone can find a strong counter-example.
The
build
flag should probably also be documented in the parser help insage-runtests
:if "build" is listed, will also run the tests relying on Sage's packaging system
or something like that.
Done.
comment:12 Changed 2 years ago by
- Commit changed from a01588849d4bde2d3036a82a61b9f6f5f79fddd7 to 2533f32e2752773f66d015ca991ead9695ccb712
comment:13 Changed 2 years ago by
What about replacing
if (SAGE_ROOT and os.path.isdir(SAGE_ROOT) and os.path.exists(os.path.join(SAGE_ROOT, '.git')) and os.path.isfile(os.path.join(SAGE_ROOT, 'sage'))):
by
if (SAGE_ROOT and os.path.isdir(os.path.join(SAGE_ROOT, 'build'))):
This way, we don't check for .git
which we don't actually need. Second, you do need the build
directory for the build
tests to succeed (and this happens to align with the name --optional=build
). Third, the check for os.path.isdir(SAGE_ROOT)
is superfluous: if a file inside that directory exists, then surely the directory itself exists.
comment:14 Changed 2 years ago by
I agree, there is no reason a distro build can't happen from a git checkout.
How about marking the python safe directory test (src/sage/doctest/control.py) as build
? Its not strictly about the sage build system, but it is specific to the way sage builds its python. I still don't think that should be tested at all, but that way at least packagers don't have to patch it out anymore.
comment:15 Changed 2 years ago by
There is nothing to stop anyone from opening up a new ticket and marking more tests as build
. Let's test this ticket thoroughly and make sure it works; that's more important than marking every one of them here.
comment:16 Changed 2 years ago by
I abstained from asking for more stuff to be added myself because I thought build
would become a misnomer. If we want to extend we can re-name and add more stuff in a follow up - once we are sure the concept is sound. I certainly have my own laundry list.
comment:17 Changed 2 years ago by
Any other ideas besides "build"? "packaging"? "infrastructure"? "buildsystem"?
comment:18 Changed 2 years ago by
"selfhosted" but I recon it is not as intuitive as "build". On the other hand
+ sage: (out, err, ret) = test_executable(["sage", "--root"]) # optional - build + sage: len(out) >= 2 # at least one character + newline; optional - build
is not exactly related to the build system. It happens to not necessarily be defined on sage-on-distro.
comment:19 follow-up: ↓ 20 Changed 2 years ago by
Speaking of stuff that is not self evident, we may want to add a bit of documentation somewhere to explain what it is.
comment:20 in reply to: ↑ 19 Changed 2 years ago by
Replying to fbissey:
Speaking of stuff that is not self evident, we may want to add a bit of documentation somewhere to explain what it is.
Forget that. I re-read the branch and it is documented well enough.
comment:21 Changed 2 years ago by
I'm a little lost in the bikeshedding. Does this need work or not? And if so please transition the ticket's state.
comment:22 Changed 2 years ago by
- Reviewers set to François Bissey
- Status changed from needs_review to positive_review
Let's move the stuff that's already done here merged. We can do more bikeshedding some other place.
comment:23 Changed 2 years ago by
Sounds good to me. If anyone has follow-up concerns please open tickets for them and CC me.
comment:24 Changed 2 years ago by
- Branch changed from u/embray/doctest/optional-build to 2533f32e2752773f66d015ca991ead9695ccb712
- Resolution set to fixed
- Status changed from positive_review to closed
comment:25 Changed 2 years ago by
- Commit 2533f32e2752773f66d015ca991ead9695ccb712 deleted
Few "build" tags were missing. Follow-up at #27781.
Added
# optional - build
to several tests insage.misc.package
, but perhaps some of the downstream packagers could mention some other problem test if there are any.New commits:
Trac #27635: Add the --optional=build tag by default when run out of the Sage source repository.
fixup
Trac #27635: Add 'optional - build' to several sage-the-distribution related tests.