Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#26488 closed enhancement (fixed)

Remove one usage of SAGE_ROOT in doctests

Reported by: arojas Owned by:
Priority: major Milestone: sage-8.5
Component: doctest coverage Keywords: packaging
Cc: saraedum, gh-timokau, fbissey Merged in:
Authors: Antonio Rojas Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 80d8e02 (Commits, GitHub, GitLab) Commit: 80d8e0235248afedea93a7f16a4a3a87b3fefb05
Dependencies: Stopgaps:

Status badges

Description (last modified by arojas)

SAGE_ROOT doesn't really make sense in distributions. Remove one usage of it in sage_ostools.pyx, which causes a doctest failure in distributions and it doesn't look that useful in sage-the-distribution.

Change History (16)

comment:1 Changed 2 years ago by arojas

  • Branch set to u/arojas/remove_one_usage_of_sage_root_in_doctests

comment:2 Changed 2 years ago by arojas

  • Cc saraedum gh-timokau fbissey added
  • Commit set to 770f848e7837b182db60d1c12d046b834af74f7e
  • Component changed from PLEASE CHANGE to doctest coverage
  • Description modified (diff)
  • Keywords packaging added
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

New commits:

770f848Remove SAGE_ROOT usage in sage_ostools.pyx

comment:3 Changed 2 years ago by fbissey

Yes I have that one in my bag. Technically here the issue is that the doctest won't find the same "sage" executable (SAGE_LOCAL/bin/sage instead of SAGE_ROOT/sage). The ability to find an executable in a particular path (not necessarily part of PATH) is not tested. The modified test becomes redundant with

        sage: from sage.misc.sage_ostools import have_program
        sage: have_program('ls')
        True

in the same series of doctests.

Do we have something that wouldn't be in a standard path that could be tested instead?

comment:4 Changed 2 years ago by git

  • Commit changed from 770f848e7837b182db60d1c12d046b834af74f7e to 1bda3a233aaca4cf544a2f74e8582f55931ef80e

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

1bda3a2Test for an executable in a given path

comment:5 Changed 2 years ago by arojas

I guess the only option that would work in all platforms/distros is SAGE_LOCAL/bin

comment:6 Changed 2 years ago by fbissey

Yes but that's just part of PATH. I am thinking if we want to do things right, we have to do something more complicated than that. If there wasn't that SAGE_ROOT/sage script in sage the distribution they would have had to write a totally different doctest for that functionality. I think that's what we have to do now.

So I think it needs to go that way.

  • create a temporary directory
  • write a small executable script in that directory
  • test the function
  • remove the script
  • do another test to try to find the executable in that directory again (to replace the other test you just removed and that should return false)
  • clean up the temporary directory

comment:7 Changed 2 years ago by arojas

have_program doesn't search in PATH if a path= parameter is passed to it.

comment:8 follow-up: Changed 2 years ago by fbissey

Hum you have a strong point here to simplify things. Do a replacement for ls in a wrong location and I'll be happy.

comment:9 Changed 2 years ago by git

  • Commit changed from 1bda3a233aaca4cf544a2f74e8582f55931ef80e to 80d8e0235248afedea93a7f16a4a3a87b3fefb05

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

80d8e02Test also for non-existant binaries and paths

comment:10 in reply to: ↑ 8 Changed 2 years ago by arojas

Replying to fbissey:

Hum you have a strong point here to simplify things. Do a replacement for ls in a wrong location and I'll be happy.

Done

comment:11 Changed 2 years ago by fbissey

  • Reviewers set to François Bissey

Author please. Unfortunately that won't make it into 8.4.

comment:12 Changed 2 years ago by arojas

  • Authors set to Antonio Rojas

comment:13 Changed 2 years ago by fbissey

  • Status changed from needs_review to positive_review

comment:14 Changed 2 years ago by gh-timokau

I kind of cheated my way around this by setting SAGE_ROOT to the location of the sage source, so I wasn't aware of the problem. This is very much better, thanks!

comment:15 Changed 2 years ago by vbraun

  • Branch changed from u/arojas/remove_one_usage_of_sage_root_in_doctests to 80d8e0235248afedea93a7f16a4a3a87b3fefb05
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:16 Changed 2 years ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

This should be re-targeted for 8.5.

Note: See TracTickets for help on using tickets.