#26488 closed enhancement (fixed)
Remove one usage of SAGE_ROOT in doctests
Reported by:  arojas  Owned by:  

Priority:  major  Milestone:  sage8.5 
Component:  doctest coverage  Keywords:  packaging 
Cc:  saraedum, ghtimokau, 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: 
Description (last modified by )
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 sagethedistribution.
Change History (16)
comment:1 Changed 2 years ago by
 Branch set to u/arojas/remove_one_usage_of_sage_root_in_doctests
comment:2 Changed 2 years ago by
 Cc saraedum ghtimokau 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
comment:3 Changed 2 years ago by
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
 Commit changed from 770f848e7837b182db60d1c12d046b834af74f7e to 1bda3a233aaca4cf544a2f74e8582f55931ef80e
Branch pushed to git repo; I updated commit sha1. New commits:
1bda3a2  Test for an executable in a given path

comment:5 Changed 2 years ago by
I guess the only option that would work in all platforms/distros is SAGE_LOCAL/bin
comment:6 Changed 2 years ago by
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
have_program doesn't search in PATH if a path= parameter is passed to it.
comment:8 followup: ↓ 10 Changed 2 years ago by
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
 Commit changed from 1bda3a233aaca4cf544a2f74e8582f55931ef80e to 80d8e0235248afedea93a7f16a4a3a87b3fefb05
Branch pushed to git repo; I updated commit sha1. New commits:
80d8e02  Test also for nonexistant binaries and paths

comment:10 in reply to: ↑ 8 Changed 2 years ago by
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
 Reviewers set to François Bissey
Author please. Unfortunately that won't make it into 8.4.
comment:12 Changed 2 years ago by
comment:13 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:14 Changed 2 years ago by
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
 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
 Milestone changed from sage8.4 to sage8.5
This should be retargeted for 8.5.
New commits:
Remove SAGE_ROOT usage in sage_ostools.pyx