#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: |
Description (last modified by )
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
Branch: | → u/mkoeppe/fix_broken_symlink_to_documentation_in_the_sage_jupyter_kernelspec |
---|
comment:2 Changed 2 years ago by
Authors: | → Matthias Koeppe |
---|---|
Commit: | → 42eaffa810d7d441fdfbfe4b53b8189b7492db69 |
Status: | new → needs_review |
comment:3 Changed 2 years ago by
Cc: | jhpalmieri added |
---|
comment:4 Changed 2 years ago by
Cc: | egourgoulhon fbissey added |
---|
comment:5 follow-up: 7 Changed 2 years ago by
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?
comment:6 Changed 2 years ago by
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 Changed 2 years ago by
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
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
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 what I wrote before doesn't make sense. But maybe it should somehow actually crash when using any of those variables during /doesnotexist
setup.py
.
comment:10 follow-up: 13 Changed 2 years ago by
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 follow-up: 12 Changed 2 years ago by
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 Changed 2 years ago by
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 Changed 2 years ago by
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 fromSAGE_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
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
Reviewers: | → Dima Pasechnik, Erik Bray |
---|---|
Status: | needs_review → positive_review |
lgtm
comment:17 Changed 2 years ago by
Branch: | u/mkoeppe/fix_broken_symlink_to_documentation_in_the_sage_jupyter_kernelspec → 42eaffa810d7d441fdfbfe4b53b8189b7492db69 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:18 Changed 2 years ago by
Cc: | slelievre added |
---|---|
Commit: | 42eaffa810d7d441fdfbfe4b53b8189b7492db69 |
Description: | modified (diff) |
Keywords: | jupyter kernel documentation symlink added |
comment:19 Changed 2 years ago by
Description: | modified (diff) |
---|
New commits:
build/pkgs/sagelib/spkg-install: Fix installation of doc symlink in sage kernel spec