Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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:

Status badges

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 arojas

  • Cc arojas added

comment:2 Changed 2 years ago by embray

  • Authors set to Erik Bray
  • 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

Added # optional - build to several tests in sage.misc.package, but perhaps some of the downstream packagers could mention some other problem test if there are any.


New commits:

99699e2Trac #27635: Add the --optional=build tag by default when run out of the Sage source repository.
c424acffixup
d4ba444Trac #27635: Add 'optional - build' to several sage-the-distribution related tests.

comment:3 Changed 2 years ago by git

  • Commit changed from d4ba44467b3ea398e297f6a7e1f83108fe600c3b to 76fae85020e052d6c732aedbea76672ed4b33183

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d96a081Trac #27635: Add the --optional=build tag by default when run out of the Sage source repository.
76fae85Trac #27635: Add 'optional - build' to several sage-the-distribution related tests.

comment:4 follow-up: Changed 2 years ago by 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 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: Changed 2 years ago by 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 and sage --info in tests/cmdline.py:198-234

comment:6 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by 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 use is_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 embray

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 and sage --info in tests/cmdline.py:198-234

Thank you for pointing them out. I'll add them.

comment:8 follow-up: Changed 2 years ago by 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 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 fbissey

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 use is_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 git

  • Commit changed from 76fae85020e052d6c732aedbea76672ed4b33183 to a01588849d4bde2d3036a82a61b9f6f5f79fddd7

Branch pushed to git repo; I updated commit sha1. New commits:

a015888Trac #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 embray

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 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.

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 in sage-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 git

  • Commit changed from a01588849d4bde2d3036a82a61b9f6f5f79fddd7 to 2533f32e2752773f66d015ca991ead9695ccb712

Branch pushed to git repo; I updated commit sha1. New commits:

a282e09Trac #27635: Add 'build' to the default TESTALL_FLAGS in the main Makefile
2533f32Trac #27635: Document the 'build' option flag in the documentation for sage-runtests

comment:13 Changed 2 years ago by jdemeyer

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 gh-timokau

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 jhpalmieri

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 fbissey

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 jhpalmieri

Any other ideas besides "build"? "packaging"? "infrastructure"? "buildsystem"?

comment:18 Changed 2 years ago by fbissey

"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: Changed 2 years ago by fbissey

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 fbissey

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 embray

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 fbissey

  • 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 embray

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 vbraun

  • 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 slabbe

  • Commit 2533f32e2752773f66d015ca991ead9695ccb712 deleted

Few "build" tags were missing. Follow-up at #27781.

Note: See TracTickets for help on using tickets.