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

Fix broken symlink to documentation in the Sage jupyter kernelspec #30903

Closed
mkoeppe opened this issue Nov 12, 2020 · 22 comments
Closed

Fix broken symlink to documentation in the Sage jupyter kernelspec #30903

mkoeppe opened this issue Nov 12, 2020 · 22 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 12, 2020

Follow-up from #30299 (Minimal fix for broken jupyter notebook):

Sage creates a broken symlink

local/share/jupyter/kernels/sagemath/doc -> /doesnotexist/html/en

which blocks jupyter kernelspec install into a system Jupyter using the instructions from #30476:

  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/shutil.py", line 368, in copytree
    raise Error(errors)
shutil.Error: [('/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/share/jupyter/kernels/sagemath/doc', '/usr/local/share/jupyter/kernels/sagemath/doc', "[Errno 2] No such file or directory: '/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/share/jupyter/kernels/sagemath/doc'")]

Reports:

CC: @EmmanuelCharpentier @malb @jcamp0x2a @dimpase @jhpalmieri @egourgoulhon @kiwifb @slel

Component: notebook

Keywords: jupyter kernel, symlink

Author: Matthias Koeppe

Branch: 42eaffa

Reviewer: Dima Pasechnik, Erik Bray

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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Nov 12, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 15, 2020

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 15, 2020

Commit: 42eaffa

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 15, 2020

New commits:

42eaffabuild/pkgs/sagelib/spkg-install: Fix installation of doc symlink in sage kernel spec

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 15, 2020

Author: Matthias Koeppe

@jhpalmieri
Copy link
Member

comment:5

It is not completely clear to me why we "poison" all of these directory names. The comment "# We also poison all directories below SAGE_LOCAL" seems like an aside, as if this may not be necessary. Any comments?

Also, does this affect bdist production?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 1, 2020

comment:6

We poison them so that it becomes harder to reintroduce bad code that breaks the separation of the sage distribution from the sage library. I introduced this in #21480.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 1, 2020

comment:7

Replying to @jhpalmieri:

Also, does this affect bdist production?

I don't know if the bdist does anything special about these symlinks. If it does not, then it is broken currently and will be fixed by this ticket as well.

@embray
Copy link
Contributor

embray commented Dec 1, 2020

comment:8

I think this is a bad-enough problem that we should release a 9.2.1 patch release, since this broke using help in the Jupyter kernel for users. Not a good look.

@embray
Copy link
Contributor

embray commented Dec 1, 2020

comment:9

Is this really the right approach? It also poisons SAGE_SHARE which is used in sage.env to set SAGE_DOC. I think if we really want this environment variable "poisoning" to be used to catch problems like this, there should be something in sage_setup which imports sage.env and makes sure none of the variables start with /doesnotexist what I wrote before doesn't make sense. But maybe it should somehow actually crash when using any of those variables during setup.py.

@embray
Copy link
Contributor

embray commented Dec 1, 2020

comment:10

I see now that it's ok since when running this script in the sage shell SAGE_DOC is already set explicitly, so it doesn't need to be derived from SAGE_SHARE. But I think this is still a bit of a mess. Either the variables in sage.env should work and have sane values when running setup.py, or not. And simply setting them to bogus values does not guarantee that they are not being used somewhere during build/installation. Since SAGE_SRC and SAGE_LOCAL are also used in a number of places, what exactly is the goal here?

@embray
Copy link
Contributor

embray commented Dec 1, 2020

comment:11

Instead of "poisoning" these variables, why don't we unset them instead, and then let sage.env derive the correct defaults it would use when installing sagelib independently of the sage distribution?

The one exception would be SAGE_ROOT, but I think instead of setting it to a bogus value it, and another other paths derived from it, should be set to None. Then we'll most likely get a crash if any of those variables are used during build/installation.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 1, 2020

comment:12

Replying to @embray:

Instead of "poisoning" these variables, why don't we unset them instead, and then let sage.env derive the correct defaults it would use when installing sagelib independently of the sage distribution?

The point of the poisoning is that defaults are NOT used.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 1, 2020

comment:13

Replying to @embray:

I see now that it's ok since when running this script in the sage shell SAGE_DOC is already set explicitly, so it doesn't need to be derived from SAGE_SHARE. But I think this is still a bit of a mess.

That's right. The core of the mess is that the build system (setup.py + sage_setup) (still) uses sage.env at all and therefore has access to various variables (and their defaulting behavior) including those that are intended for runtime use only. This is what the poisoning addresses.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 1, 2020

comment:14

In any case, this ticket here is a simple fix. The real action is happening in Meta-ticket #30306.

@dimpase
Copy link
Member

dimpase commented Dec 3, 2020

Reviewer: Dima Pasechnik, Erik Bray

@dimpase
Copy link
Member

dimpase commented Dec 3, 2020

comment:15

lgtm

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 3, 2020

comment:16

Thanks.

@vbraun
Copy link
Member

vbraun commented Dec 6, 2020

@slel
Copy link
Member

slel commented Dec 7, 2020

Changed commit from 42eaffa to none

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Dec 7, 2020

Changed keywords from none to jupyter kernel, symlink

@slel

This comment has been minimized.

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

6 participants