Opened 3 years ago
Closed 3 years ago
#21527 closed defect (invalid)
Fix symbolic link to thebe.js
Reported by: | mkoeppe | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | build | Keywords: | |
Cc: | jdemeyer, fbissey, vdelecroix, vbraun, rbeezer, slelievre, tmonteil, nthiery, embray | Merged in: | |
Authors: | Reviewers: | François Bissey | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Followup on #20690:
The symbolic link src/doc/common/themes/sage/static/thebe.js
points to ../../../../../../local/share/thebe/thebe.js
which is in the Sage installation directory $SAGE_LOCAL
. Having such a symlink from the source to the installation directory really makes no sense and goes against the goal of making Sage more modular.
See also fbissey (#21501, comment 18):
I am expecting the link
src/doc/common/themes/sage/static/thebe.js
which is pointing to../../../../../../local/share/thebe/thebe.js
to be dangling for you. I have no solution for you, I usually deal with those at the package manager level. Although in that particular case I just addedthebe.js
to the source in place.
See #21534 for a different issue with allowing a custom $SAGE_LOCAL
.
Change History (20)
comment:1 Changed 3 years ago by
- Cc fbissey added
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
- Cc vdelecroix vbraun rbeezer slelievre tmonteil nthiery added
Cc'ing people who worked on the Thebe ticket #20690. This needs fixing. Having a symbolic link like that in the source tree is just not sustainable.
comment:4 Changed 3 years ago by
- Description modified (diff)
- Summary changed from Allow SAGE_LOCAL to be customized - follow-up to Fix symbolic link to thebe.js
Splitting this ticket in two: see also #21534.
comment:5 Changed 3 years ago by
- Description modified (diff)
comment:6 Changed 3 years ago by
- Description modified (diff)
comment:7 follow-ups: ↓ 8 ↓ 9 Changed 3 years ago by
I don't have a good suggestion for what to do with thebe.js. It's a general problem: we are lacking a clean solution for managing the javascript packages Sage depends on. Reverting to the first implementation of #20690 (including thebe.js directly in Sage's lib) is indeed on the ugly side. But this is a simple solution that seems to actually ease the life of our packagers for now, and we are already doing that for mathjax.
comment:8 in reply to: ↑ 7 Changed 3 years ago by
Replying to nthiery:
and we are already doing that for mathjax.
What are we doing for mathjax? I cannot really find how we make mathjax work, but it's certainly not the same way as we do with thebe.js
.
comment:9 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 3 years ago by
Replying to nthiery:
I don't have a good suggestion for what to do with thebe.js. It's a general problem: we are lacking a clean solution for managing the javascript packages Sage depends on. Reverting to the first implementation of #20690 (including thebe.js directly in Sage's lib) is indeed on the ugly side. But this is a simple solution that seems to actually ease the life of our packagers for now, and we are already doing that for mathjax.
This is not true, mathjax is installed separately for a while now (it used to be bundled with sagenb, and was made a spkg to avoid duplication since jupyter also depends on mathjax).
For sagenb, it also uses symlink, the only difference is that the symlink is installed during sagenb's spkg-install
(in particular, both sides of the symlink belong to the SAGE_LOCAL
directory, which i guess is why no downstream packager complained). As i told in the previous ticket, there is no docbuild pkg where to install the symlink, but we can try to set is somewhere in the docbuild script, so that both sides of the symlink are done within the SAGE_LOCAL
directory.
The use of symlink at install time used to be the case for jupyter as well, but it seems not to be the case anymore, so perhaps we could try to mimic the jupyter way (i have no idea how mathjax and jupyter are currently linked).
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 14 Changed 3 years ago by
Replying to tmonteil:
there is no docbuild pkg where to install the symlink, but we can try to set is somewhere in the docbuild script, so that both sides of the symlink are done within the
SAGE_LOCAL
directory.
Sounds good - could you prepare a patch for that?
If no better place can be found, I suppose it would even be OK to make the symlink in one of the doc-related targets of build/make/deps
.
comment:11 Changed 3 years ago by
- Milestone changed from sage-7.5 to sage-7.4
- Priority changed from major to blocker
comment:12 Changed 3 years ago by
I don't really think this is a blocker... it still works with the default SAGE_LOCAL
. Given that we don't really officially support changing SAGE_LOCAL
, I don't think this is so urgent.
comment:13 Changed 3 years ago by
- Priority changed from blocker to major
comment:14 in reply to: ↑ 10 Changed 3 years ago by
Replying to mkoeppe:
Replying to tmonteil:
there is no docbuild pkg where to install the symlink, but we can try to set is somewhere in the docbuild script, so that both sides of the symlink are done within the
SAGE_LOCAL
directory.Sounds good - could you prepare a patch for that? If no better place can be found, I suppose it would even be OK to make the symlink in one of the doc-related targets of
build/make/deps
.
I will do that, but i have to say that i will be very buzy during ther next 3 weeks, so it might take some time.
comment:15 Changed 3 years ago by
- Milestone changed from sage-7.4 to sage-7.5
OK, I've changed the milestone.
comment:16 Changed 3 years ago by
*ping*
comment:17 Changed 3 years ago by
- Cc embray added
- Status changed from new to needs_review
Apparently fixed in #22061.
comment:18 Changed 3 years ago by
Ah, didn't see this ticket before. Yes, should be addressed by #22061, hopefully.
comment:19 Changed 3 years ago by
- Milestone changed from sage-7.5 to sage-duplicate/invalid/wontfix
- Reviewers set to François Bissey
- Status changed from needs_review to positive_review
comment:20 Changed 3 years ago by
- Resolution set to invalid
- Status changed from positive_review to closed
The link didn't cause any trouble so far because it is not tested in any way. But I complained that the link was going to cause trouble in https://trac.sagemath.org/ticket/20690#comment:47 I didn't expect it to happen so soon. Its use is not currently tested in any way so doctest didn't spot that it was dangling.