Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve symbolic links in $HOME/.sage #11704

Closed
jdemeyer opened this issue Aug 18, 2011 · 20 comments
Closed

Resolve symbolic links in $HOME/.sage #11704

jdemeyer opened this issue Aug 18, 2011 · 20 comments

Comments

@jdemeyer
Copy link
Contributor

Similarly to SAGE_ROOT, it might be good to resolve symbolic links in $HOME/.sage when assigning DOT_SAGE. This can be useful for example when $HOME/.sage is a symbolic link to a non-existent directory (the directory would be created later in sage-sage).

Probably we should use whatever solution comes from #5852.

Depends on #11924

CC: @nexttime

Component: scripts

Author: Jeroen Demeyer

Reviewer: John Palmieri

Merged: sage-4.8.alpha6

Issue created by migration from https://trac.sagemath.org/ticket/11704

@jdemeyer jdemeyer added this to the sage-4.8 milestone Aug 18, 2011
@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Contributor Author

Dependencies: #5852

@jdemeyer
Copy link
Contributor Author

Changed dependencies from #5852 to none

@jhpalmieri
Copy link
Member

comment:9

Note also #11924: currently if DOTSAGE doesn't end with a slash, then there are unintended consequences. (#11924 should be an easy review.)

@jdemeyer
Copy link
Contributor Author

Dependencies: #11924

@jhpalmieri
Copy link
Member

comment:11

I wonder if we should put the resolvelinks script in a separate file in the root repo, and then call it from the top-level sage script (as in #5852) or from sage-env (for this ticket). As such, this seems related to #11073.

@jdemeyer
Copy link
Contributor Author

comment:12

Replying to @jhpalmieri:

I wonder if we should put the resolvelinks script in a separate file in the root repo, and then call it from the top-level sage script (as in #5852) or from sage-env (for this ticket).

We cannot put it in a seperate file because $SAGE_ROOT/sage would need to know SAGE_ROOT in order to find this new separate file. So, in any case, $SAGE_ROOT/sage needs the resolvelinks script. So, we would have to source $SAGE_ROOT/sage from sage-env which doesn't sound like a very good idea (it could be done though).

@jhpalmieri
Copy link
Member

comment:13

Well, what's the purpose of canonicalizing SAGE_ROOT in the sage script? It seems useful to have canonical names for paths, and it's important for some doctests (etc.), but do we actually need to do this for the script to function properly right at the start? If not, we could detect a preliminary value for SAGE_ROOT first, find the resolvelinks script, run it if it exists (e.g., check after seeing if sage-sage exists and before exporting SAGE_ROOT), and then find and export the permanent value for SAGE_ROOT.

@jdemeyer
Copy link
Contributor Author

comment:14

Replying to @jhpalmieri:

Well, what's the purpose of canonicalizing SAGE_ROOT in the sage script?

It's precisely for the reason I said: we need to be able to determine a usable value of $SAGE_ROOT before we can do anything with Sage.

It's true that it doesn't matter that it's canonical, but we still need to dereference symbolic links in $0 (the script filename) to find $SAGE_ROOT. Since #5852, we support an installation where somebody makes a symbolic link from /usr/local/bin/sage to /usr/local/sage/sage-4.8/sage or whatever.

I agree the code duplication is not so nice, but I still think the patch can be reviewed as-is.

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@jdemeyer
Copy link
Contributor Author

comment:16

Amazing, this causes

**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha4-remove-base/devel/sage-main/sage/schemes/elliptic_curves/ell_rational_field.py", line 526:
    sage: E.conductor(algorithm="gp")
Exception raised:
    Traceback (most recent call last):
      File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha4-remove-base/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha4-remove-base/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha4-remove-base/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_12[5]>", line 1, in <module>
        E.conductor(algorithm="gp")###line 526:
    sage: E.conductor(algorithm="gp")
      File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha4-remove-base/local/lib/python/site-packages/sage/schemes/elliptic_curves/ell_rational_field.py", line 552, in conductor
        self.__conductor_gp = Integer(gp.eval('ellglobalred(ellinit(%s,0))[1]'%list(self.a_invariants())))
      File "integer.pyx", line 681, in sage.rings.integer.Integer.__init__ (sage/rings/integer.c:6786)
    TypeError: unable to convert x (=read("/mnt/usb1/scratch/jdemeyer/merger/sage-4.8.alpha4-remove-base/home/.sage/temp/sage.math.washington.edu/9425//interface//tmp11939") ) to an integer
**********************************************************************

@jdemeyer
Copy link
Contributor Author

comment:17

I keep positive review here, the latter problem is now #12221.

@jdemeyer
Copy link
Contributor Author

Changed dependencies from #11924 to #11924, #12221

@jdemeyer
Copy link
Contributor Author

Attachment: 11704.patch.gz

Patch to the SCRIPTS repository (local/bin)

@jdemeyer
Copy link
Contributor Author

Changed dependencies from #11924, #12221 to #11924

@jdemeyer
Copy link
Contributor Author

comment:18

I changed the patch slightly to add the slash after $DOT_SAGE. I don't see how this could cause #12221, but it seems safer to keep the slash, just like it was before. Needs review please.

@jdemeyer
Copy link
Contributor Author

comment:20

John, can you please review this new revised patch?

@jhpalmieri
Copy link
Member

comment:21

The only real change is to append the slash, right? It works for me in my testing.

@jdemeyer
Copy link
Contributor Author

Merged: sage-4.8.alpha6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants