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 jdemeyer)

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 added thebe.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 mkoeppe

  • Cc fbissey added

comment:2 Changed 3 years ago by fbissey

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.

comment:3 Changed 3 years ago by mkoeppe

  • 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.

Last edited 3 years ago by mkoeppe (previous) (diff)

comment:4 Changed 3 years ago by jdemeyer

  • 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 jdemeyer

  • Description modified (diff)

comment:6 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:7 follow-ups: Changed 3 years ago by 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.

comment:8 in reply to: ↑ 7 Changed 3 years ago by jdemeyer

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: Changed 3 years ago by tmonteil

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: Changed 3 years ago by 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.

comment:11 Changed 3 years ago by mkoeppe

  • Milestone changed from sage-7.5 to sage-7.4
  • Priority changed from major to blocker

comment:12 Changed 3 years ago by jdemeyer

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 vbraun

  • Priority changed from blocker to major

comment:14 in reply to: ↑ 10 Changed 3 years ago by tmonteil

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 mkoeppe

  • Milestone changed from sage-7.4 to sage-7.5

OK, I've changed the milestone.

comment:16 Changed 3 years ago by mkoeppe

*ping*

comment:17 Changed 3 years ago by mkoeppe

  • Cc embray added
  • Status changed from new to needs_review

Apparently fixed in #22061.

comment:18 Changed 3 years ago by embray

Ah, didn't see this ticket before. Yes, should be addressed by #22061, hopefully.

comment:19 Changed 3 years ago by fbissey

  • 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 vbraun

  • Resolution set to invalid
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.