Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#30903 closed defect (fixed)

Fix broken symlink to documentation in the Sage jupyter kernelspec

Reported by: mkoeppe Owned by:
Priority: critical Milestone: sage-9.3
Component: notebook Keywords: jupyter kernel, documentation, symlink
Cc: charpent, malb, gh-jcamp0x2a, dimpase, jhpalmieri, egourgoulhon, fbissey, slelievre Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik, Erik Bray
Report Upstream: N/A Work issues:
Branch: 42eaffa (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

GitHub link to the corresponding issue

Description (last modified by slelievre)

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:

Change History (19)

comment:1 Changed 2 years ago by mkoeppe

Branch: u/mkoeppe/fix_broken_symlink_to_documentation_in_the_sage_jupyter_kernelspec

comment:2 Changed 2 years ago by mkoeppe

Authors: Matthias Koeppe
Commit: 42eaffa810d7d441fdfbfe4b53b8189b7492db69
Status: newneeds_review

New commits:

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

comment:3 Changed 2 years ago by mkoeppe

Cc: jhpalmieri added

comment:4 Changed 2 years ago by mkoeppe

Cc: egourgoulhon fbissey added

comment:5 Changed 2 years ago by jhpalmieri

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?

Last edited 2 years ago by jhpalmieri (previous) (diff)

comment:6 Changed 2 years ago by mkoeppe

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.

comment:7 in reply to:  5 Changed 2 years ago by mkoeppe

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.

comment:8 Changed 2 years ago by embray

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.

comment:9 Changed 2 years ago by embray

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.

Last edited 2 years ago by embray (previous) (diff)

comment:10 Changed 2 years ago by 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. 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?

comment:11 Changed 2 years ago by 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 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.

comment:12 in reply to:  11 Changed 2 years ago by mkoeppe

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.

comment:13 in reply to:  10 Changed 2 years ago by mkoeppe

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.

comment:14 Changed 2 years ago by mkoeppe

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

comment:15 Changed 2 years ago by dimpase

Reviewers: Dima Pasechnik, Erik Bray
Status: needs_reviewpositive_review

lgtm

comment:16 Changed 2 years ago by mkoeppe

Thanks.

comment:17 Changed 2 years ago by vbraun

Branch: u/mkoeppe/fix_broken_symlink_to_documentation_in_the_sage_jupyter_kernelspec42eaffa810d7d441fdfbfe4b53b8189b7492db69
Resolution: fixed
Status: positive_reviewclosed

comment:18 Changed 2 years ago by slelievre

Cc: slelievre added
Commit: 42eaffa810d7d441fdfbfe4b53b8189b7492db69
Description: modified (diff)
Keywords: jupyter kernel documentation symlink added

comment:19 Changed 2 years ago by slelievre

Description: modified (diff)
Note: See TracTickets for help on using tickets.