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 symbolic link to thebe.js #21527

Closed
mkoeppe opened this issue Sep 18, 2016 · 17 comments
Closed

Fix symbolic link to thebe.js #21527

mkoeppe opened this issue Sep 18, 2016 · 17 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 18, 2016

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.

CC: @jdemeyer @kiwifb @videlec @vbraun @rbeezer @slel @sagetrac-tmonteil @nthiery @embray

Component: build

Reviewer: François Bissey

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

@mkoeppe mkoeppe added this to the sage-7.5 milestone Sep 18, 2016
@kiwifb
Copy link
Member

kiwifb commented Sep 18, 2016

comment:2

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 18, 2016

comment:3

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.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

comment:4

Splitting this ticket in two: see also #21534.

@jdemeyer jdemeyer changed the title Allow SAGE_LOCAL to be customized - follow-up Fix symbolic link to thebe.js Sep 18, 2016
@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@nthiery
Copy link
Contributor

nthiery commented Sep 19, 2016

comment:7

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.

@jdemeyer
Copy link

comment:8

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.

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Sep 19, 2016

comment:9

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 3, 2016

comment:10

Replying to @sagetrac-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.

@mkoeppe mkoeppe modified the milestones: sage-7.5, sage-7.4 Oct 9, 2016
@jdemeyer
Copy link

jdemeyer commented Oct 9, 2016

comment:12

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.

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Oct 16, 2016

comment:14

Replying to @mkoeppe:

Replying to @sagetrac-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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 16, 2016

comment:15

OK, I've changed the milestone.

@mkoeppe mkoeppe modified the milestones: sage-7.4, sage-7.5 Oct 16, 2016
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 4, 2016

comment:16

ping

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 29, 2016

comment:17

Apparently fixed in #22061.

@embray
Copy link
Contributor

embray commented Dec 29, 2016

comment:18

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

@kiwifb
Copy link
Member

kiwifb commented Dec 29, 2016

Reviewer: François Bissey

@kiwifb kiwifb removed this from the sage-7.5 milestone Dec 29, 2016
@vbraun vbraun closed this as completed Jan 21, 2017
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