Opened 3 years ago

Closed 3 years ago

#27196 closed enhancement (fixed)

make sagelib work and doctest even if SAGE_ROOT is None

Reported by: fbissey Owned by:
Priority: major Milestone: sage-8.7
Component: distribution Keywords:
Cc: Merged in:
Authors: François Bissey Reviewers: Jeroen Demeyer, François Bissey
Report Upstream: N/A Work issues:
Branch: 56029cb (Commits, GitHub, GitLab) Commit: 56029cb708607b5af2aa8320a3ac9fe561005cb8
Dependencies: #27206 Stopgaps:

Status badges

Description (last modified by fbissey)

This is a push to be able to use and doctest sage without SAGE_ROOT being defined. This is mainly useful for distro or if you import sage from python directly without running sage-env first.

  • sage/misc/persist.pyx is dealt with in #27192
  • in sage/misc/citation.pyx SAGE_ROOT is replaced with "" but for all cases I have seen SAGE_LIB would be correct. I have only gone for SAGE_LOCAL in case something in SAGE_LOCAL/bin needs to be picked up.
  • in bin/sage-notebook and sage/misc/package.py, SAGE_ROOT is imported but not used. So I removed it while I was at it.

This ticket doesn't deal with SAGE_ROOT used in src/sage/misc/copying.py.

Change History (43)

comment:1 Changed 3 years ago by fbissey

  • Authors set to François Bissey
  • Branch set to u/fbissey/sage_root
  • Commit set to aa46c639c603bda111f892544963c4c3042fa335
  • Status changed from new to needs_review

New commits:

aa46c63make sagelib work even if SAGE_ROOT is None

comment:2 Changed 3 years ago by jdemeyer

  • Dependencies changed from #27040 #27192 to #27040

I don't think that this actually depends on #27192.

comment:3 Changed 3 years ago by jdemeyer

  1. For DOT_GIT, I would prefer to see that added to sage/env.py (maybe rename it to SAGE_ROOT_GIT). Note that you'll need to rebase the branch to #27040 then or wait for the next beta. I'm also not convinced that we can rely on the fact that os.path.isdir("") returns False, I would prefer an explicit check for None there.
  1. In src/sage/misc/misc.py, you can use sys.executable instead.

comment:4 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:5 follow-up: Changed 3 years ago by fbissey

I came to think I wasn't very pythonic as well. I should have used try:...expect: blocks instead of conditionals. Before #27040 there was a SAGE_ROOT_GIT I guess that would mean restoring it. Should we do that when it is only used in a single file as far as I can tell?

comment:6 in reply to: ↑ 5 Changed 3 years ago by jdemeyer

Replying to fbissey:

Before #27040 there was a SAGE_ROOT_GIT I guess that would mean restoring it.

Sure. I just removed it because I wasn't used. :-)

Should we do that when it is only used in a single file as far as I can tell?

Sure, why not? It's cleaner that way.

comment:7 follow-up: Changed 3 years ago by fbissey

OK, that's a plan. Do you have any comments about citation.pyx?

comment:8 in reply to: ↑ 7 Changed 3 years ago by jdemeyer

  • Dependencies #27040 deleted

Replying to fbissey:

Do you have any comments about citation.pyx?

No. I think your changes are OK.

comment:9 Changed 3 years ago by fbissey

  • Branch changed from u/fbissey/sage_root to u/fbissey/sage_root_v2
  • Commit changed from aa46c639c603bda111f892544963c4c3042fa335 to f79cb445250a72a53b1a8cf7222010733f307f6a
  • Status changed from needs_work to needs_review

OK rebased on new beta and better documented.


New commits:

1bd3c8fremove useless SAGE_ROOT
f79cb44Eliminate the use of SAGE_ROOT where possible. Properly catch when replacement is None.

comment:10 Changed 3 years ago by git

  • Commit changed from f79cb445250a72a53b1a8cf7222010733f307f6a to f78637548620b8b51145e23bd17bebba7750a181

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

f786375remove pesky tabs

comment:11 Changed 3 years ago by fbissey

  • Description modified (diff)

comment:12 follow-up: Changed 3 years ago by jdemeyer

This is an odd way to test for None:

        try:
            have_git = os.path.exists(SAGE_ROOT_GIT)
        except TypeError:
            have_git

What's wrong with

        if SAGE_ROOT_GIT is not None:
            have_git = os.path.isdir(SAGE_ROOT_GIT)
        else:
            have_git = False

or even

        have_git = (SAGE_ROOT_GIT is not None) and os.path.isdir(SAGE_ROOT_GIT)

comment:13 Changed 3 years ago by jdemeyer

Also little reason to use an extra variable here:

        sage: python_executable = sys.executable
        sage: sage_makedirs(python_executable)

Why not

        sage: sage_makedirs(sys.executable)

comment:14 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

You may also want to squash your commits for a cleaner history.

comment:15 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by fbissey

Replying to jdemeyer:

This is an odd way to test for None:

        try:
            have_git = os.path.exists(SAGE_ROOT_GIT)
        except TypeError:
            have_git

What's wrong with

        if SAGE_ROOT_GIT is not None:
            have_git = os.path.isdir(SAGE_ROOT_GIT)
        else:
            have_git = False

or even

        have_git = (SAGE_ROOT_GIT is not None) and os.path.isdir(SAGE_ROOT_GIT)

Well it seemed to me the pythonic way is try:... except: but I won't be religious on that and arguably it is not testing for None so yes, I could go back to the way I originally did it.

As for python_executable I'll admit I thought of removing it and ended up going with minimal changes.

comment:16 Changed 3 years ago by git

  • Commit changed from f78637548620b8b51145e23bd17bebba7750a181 to d3eefbbd0ea81552cab7bfaa08f10bc5c08ca32c

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

d46ba1echeck more explictly for None
d3eefbbremove useless statement

comment:17 follow-up: Changed 3 years ago by fbissey

  • Status changed from needs_work to needs_review

There, and all squashed.

comment:18 in reply to: ↑ 15 Changed 3 years ago by jdemeyer

Replying to fbissey:

Well it seemed to me the pythonic way is try:... except:

I think that mainly applies when the condition is either expensive to check or when the condition might change independently (e.g. checking for existence of a file).

comment:19 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:20 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by jdemeyer

Replying to fbissey:

There, and all squashed.

No, but it doesn't matter.

comment:21 in reply to: ↑ 20 Changed 3 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

There, and all squashed.

No, but it doesn't matter.

All right. Give me a pointer to how to squash for next time then.

comment:22 Changed 3 years ago by fbissey

  • Dependencies set to #27206
  • Status changed from positive_review to needs_work

#27206 which is in the process of being merged by Volker interferes with this in citation.pyx.

comment:23 Changed 3 years ago by jdemeyer

  • Branch changed from u/fbissey/sage_root_v2 to u/jdemeyer/sage_root_v2

comment:24 Changed 3 years ago by git

  • Commit changed from d3eefbbd0ea81552cab7bfaa08f10bc5c08ca32c to 099f3e7bf127e84b644789e881f504b8c71d5526

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

099f3e7Use os.path.isdir() to check SAGE_ROOT_GIT

comment:25 Changed 3 years ago by git

  • Commit changed from 099f3e7bf127e84b644789e881f504b8c71d5526 to 172648d22c55b53c7af036448b2f306c02b293d4

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

172648dUse os.path.isdir() to check SAGE_ROOT_GIT

comment:26 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Merged with #27206 and squashed. I added one tiny additional commit, please review.

comment:27 Changed 3 years ago by fbissey

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, François Bissey
  • Status changed from needs_review to positive_review

Cool, that means I won't have to do it tomorrow :) your comment is more comprehensive. I guess isdir is a more precise test as well. Someone could have fun creating a file SAGE_ROOT/.git and see what breaks.

comment:28 follow-ups: Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

There is a syntax error in env.py, obviousy none of you never tried it...

comment:29 in reply to: ↑ 28 Changed 3 years ago by fbissey

Replying to vbraun:

There is a syntax error in env.py, obviousy none of you never tried it...

Got it. Well, not in that form.

comment:30 in reply to: ↑ 28 Changed 3 years ago by jdemeyer

Replying to vbraun:

There is a syntax error in env.py, obviousy none of you never tried it...

Good point, this has never actually worked.

comment:31 Changed 3 years ago by fbissey

  • Branch changed from u/jdemeyer/sage_root_v2 to u/fbissey/sage_root_v2
  • Commit changed from 172648d22c55b53c7af036448b2f306c02b293d4 to fa1084b513cacad52a7f89f529dab6ffbcc88421
  • Status changed from needs_work to needs_review

This version has been properly doctested.


New commits:

5bbfa18Merge branch 'develop' into sage_root_v3
fa1084bfix syntax error in env.py

comment:32 follow-up: Changed 3 years ago by jhpalmieri

I get a doctest failure in src/sage/misc/inline_fortran.py:

Failed example:
    os.getcwd() == DOT_SAGE
Expected:
    True
Got:
    False

The issue is that there is a trailing slash in DOT_SAGE but not in os.getcwd(). Does this come from src/bin/sage-env?

    # In theory, DOT_SAGE is not required to have a trailing slash.
    # But since there are some issues (#11924, maybe #12221),
    # we add a slash for safety.
    DOT_SAGE="${DOT_SAGE}/"
    export DOT_SAGE

comment:33 in reply to: ↑ 32 Changed 3 years ago by fbissey

Replying to jhpalmieri:

I get a doctest failure in src/sage/misc/inline_fortran.py:

Failed example:
    os.getcwd() == DOT_SAGE
Expected:
    True
Got:
    False

The issue is that there is a trailing slash in DOT_SAGE but not in os.getcwd(). Does this come from src/bin/sage-env?

Must be. env.py's default doesn't have one and my own sage-on-gentoo sage-env defines DOT_SAGE without a trailing slash.

    # In theory, DOT_SAGE is not required to have a trailing slash.
    # But since there are some issues (#11924, maybe #12221),
    # we add a slash for safety.
    DOT_SAGE="${DOT_SAGE}/"
    export DOT_SAGE

I think both issues are totally irrelevant now because of other changes. Although I will study #12221 because I have a issue with pari's testsuite in Gentoo's portage that may be totally related.

In this particular case I guess we need to make sure to "normalize" the path tested so it doesn't end up with a slash. Is there a python function for that?

comment:34 Changed 3 years ago by fbissey

Would using os.getcwd() == os.path.normpath(DOT_SAGE) (provided the right imports before hand) instead work?

comment:35 Changed 3 years ago by fbissey

OK normpath works. samefile would be nice but is not available on windows apparently, so that could cause trouble on cygwin.

Extra question: what should we do about that difference between sage-env and env.py? Should we have trailing / at all?

comment:36 Changed 3 years ago by jhpalmieri

I don't understand the comment in sage-env about #11924, since that ticket was explicitly trying to set things up so a trailing backslash would not be necessary. For this ticket I would just use os.path.normpath, but we should also (probably on a different ticket) try removing the trailing slash. I don't know the relationship between src/sage/env.py and src/bin/sage-env: do we need to deal with DOT_SAGE in sage-env if we set it in env.py?

comment:37 Changed 3 years ago by fbissey

All right, a very good question.

  • sage-env contains many things and does overlaps with env.py
  • env.py has default values for most variables but those default will be overridden if the variable is already defined in the running environment - by sage-env for example.
  • sage-env is loaded by the sage start up script. Depending on the arguments it may actually start other scripts rather than sage itself. Those scripts may need environment variables to be set (look at sage-cython, sage-ipynb2rst ...).
  • env.py has default that means sage will start if you load it directly from python. With this ticket and a few others, it will just work even if no environment variables are loaded from sage-env before hand. Except for sage/misc/copying.py (which isn't doctested).

Now, a lot of the stuff in sage-env is about the building of sage. Some other stuff is about the self containment of sage. For example, sage change the default ipython folder to .sage/ipython so there is no interference with a system package. This is of course moot for sage-on-distro. Finally there is stuff that is really for running sage and that is mostly redundant with env.py.

Finally I should plug myself https://groups.google.com/forum/#!topic/sage-packaging/X-cMW8fFfso. Note that I still have DOT_SAGE in sage-env because of some scripts called from the sage starting shell script (sage-cleaner and sage-runtests being the most concerning).

Last edited 3 years ago by fbissey (previous) (diff)

comment:38 Changed 3 years ago by git

  • Commit changed from fa1084b513cacad52a7f89f529dab6ffbcc88421 to 56029cb708607b5af2aa8320a3ac9fe561005cb8

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

56029cbBetter comparison of folder names

comment:39 Changed 3 years ago by fbissey

OK fixed that doctest.

comment:40 Changed 3 years ago by jdemeyer

I agree with that last change.

comment:41 Changed 3 years ago by jdemeyer

Let's wait for the patchbot before setting to positive review :-)

comment:42 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:43 Changed 3 years ago by vbraun

  • Branch changed from u/fbissey/sage_root_v2 to 56029cb708607b5af2aa8320a3ac9fe561005cb8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.