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

Package Thebe #21309

Closed
sagetrac-tmonteil mannequin opened this issue Aug 22, 2016 · 43 comments
Closed

Package Thebe #21309

sagetrac-tmonteil mannequin opened this issue Aug 22, 2016 · 43 comments

Comments

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Aug 22, 2016

Thebe is a Jupyter javascript plugin for static sites that allows to render
selected divs of an HTML page as live cells connected to a Jupyter server.

Ticket #20690 aims to use this technology to implement live documentation
in the Jupyter notebook. Indeed, live documentation is one of the features
we had in the legacy Sage notebook and that was not yet available in Sage
when using the Jupyter notebook.

A first implementation of #20690 was entangling thebe.js within Sage source
code. Since upstream is well-defined, the aim of this ticket is to instead
package it the usual way, and #20690 is now based on this packaged version.

The zipball can be found at
https://github.com/oreillymedia/thebe/archive/9624e0a07a00026103dce1a3e32bbfbf90a6d0f9.zip
and should be renamed thebe-9624e0a0.zip
(upstream does not propose explicit releases).

It can also temporarily be downloaded at
https://lipn.univ-paris13.fr/~monteil/hebergement/tmp/thebe-9624e0a0.zip

CC: @sagetrac-fcayre @videlec @nthiery @slel

Component: packages: standard

Keywords: sd75, sdl

Author: Thierry Monteil

Branch/Commit: 520cdd0

Reviewer: Nicolas M. Thiéry, Jeroen Demeyer

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

@sagetrac-tmonteil sagetrac-tmonteil mannequin added this to the sage-7.4 milestone Aug 22, 2016
@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 22, 2016

Branch: u/tmonteil/package_thebe_js

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 22, 2016

New commits:

7d4fe86#21309 : package thebejs.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 22, 2016

Commit: 7d4fe86

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 24, 2016

Changed keywords from none to sd75

@nthiery
Copy link
Contributor

nthiery commented Aug 25, 2016

Changed branch from u/tmonteil/package_thebe_js to u/nthiery/package_thebe_js

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

Changed commit from 7d4fe86 to e1b914a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

e1b914a21309: made the instructions to fetch the latest thebe version into an executable shell command

@nthiery
Copy link
Contributor

nthiery commented Aug 25, 2016

comment:6

We discussed this face to face with Thierry, and I made minor improvements to the text in the SPKG that he double checked over my shoulder. The files look good. I tested it, and it works.

Positive review. Thanks Thierry!

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor

nthiery commented Aug 25, 2016

Reviewer: Nicolas M. Thiéry

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2016

comment:7

I personally would have preferred a spkg-src that pulls a given git commit. Downloading master.zip is quite likely not a repeatable operation (as in "it doesn't necessarily give you the same file every time").

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 27, 2016

comment:8

Would downloading thebe-9624e0a07a00026103dce1a3e32bbfbf90a6d0f9.zip instead of master.zip be satisfying ?

@kiwifb
Copy link
Member

kiwifb commented Aug 27, 2016

comment:9

It would probably work :)

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 27, 2016

Changed branch from u/nthiery/package_thebe_js to u/tmonteil/package_thebe_js

@sagetrac-tmonteil

This comment has been minimized.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 27, 2016

comment:12

OK i have done that. The hash of the zipball changed, but according to dirdiff the content is the same.


New commits:

8af8f5e#21309 : fetch the archive named by the commit of the master branch.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 27, 2016

Changed commit from e1b914a to 8af8f5e

@vbraun vbraun modified the milestones: sage-7.4, sage-7.5 Aug 27, 2016
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2016

Changed commit from 8af8f5e to 520cdd0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 4, 2016

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

520cdd0#21309 : thebejs -> thebe ; use hash in pkg name ; link to trac URL.

@sagetrac-tmonteil

This comment has been minimized.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 4, 2016

comment:19

Replying to @jdemeyer:

You should have the git commit in the version number. It should be possible to exactly
reconstruct the tarball. The date on which it was packaged is not sufficient for that.

Done.

When you do this, also adjust the instructions in SPKG.txt (and perhaps replace #20690 by the actual Trac URL).

Done.

Replying to @jdemeyer:

Also: upstream calls itself thebe, why rename to thebejs?

Most javascript libs have dual names, for example node vs nodejs or d3 vs d3js. Since we used the *js name for in other packages, i took that one, but indeed, in the case of thebe there is no *js version. So i renamed the package thebeas suggested.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Sep 5, 2016

comment:21

Looks good.

@jdemeyer
Copy link

jdemeyer commented Sep 5, 2016

Changed reviewer from Nicolas M. Thiéry to Nicolas M. Thiéry, Jeroen Demeyer

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Sep 5, 2016

comment:22

Just expanding the ticket description to make it more explanatory.

@slel slel changed the title Package thebe.js Package Thebe Sep 5, 2016
@slel

This comment has been minimized.

@kiwifb
Copy link
Member

kiwifb commented Sep 7, 2016

comment:24

I am going through this for inclusion in sage-on-gentoo. Are we taking a tarball of the whole repo to install only a 1010KB text file out of it? Why not ship it directly in src/doc/common/themes/? With documentation on origin and how to update it.

@slel
Copy link
Member

slel commented Sep 7, 2016

comment:25

Replying to @kiwifb:

I am going through this for inclusion in sage-on-gentoo.
Are we taking a tarball of the whole repo to install only a 1010KB text file out of it?

Reminds me of talks and blog posts such as
http://idlewords.com/talks/website_obesity.htm
and
https://medium.com/friendship-dot-js/i-peeked-into-my-node-modules-directory-and-you-wont-believe-what-happened-next-b89f63d21558

Why not ship it directly in src/doc/common/themes/?
With documentation on origin and how to update it.

Sounds like a good idea.

@kiwifb
Copy link
Member

kiwifb commented Sep 7, 2016

comment:26

Well I guess it ties in with my earlier comment on #20690 #20690 comment:47 so it is also not totally disinterested.

But it has to be overkill, I must not be the only thinking it is, once the realisation sets in.

@nthiery
Copy link
Contributor

nthiery commented Sep 7, 2016

comment:27

#20690 originally was just including thebe.js in the Sage sources as you suggest, and it was requested that we made a proper package ... See the comments there:

[#20690 comment:24](https://github.com/sagemath/sage/issues/20690#comment:24)

I don't mind either solution, but let's decide quickly to get this in!

@kiwifb
Copy link
Member

kiwifb commented Sep 7, 2016

comment:28

I can understand the point of tracking upstream, but shipping a whole tarball and only installing only one file out of it because the rest is not of interest is overkill, we don't even use the rest to build something. And it creates the problem of pointing out to the file later. Another point is that the whole repo is actually not in an installable form, it is to be used as is, the fact that upstream doesn't do release means that is not that much easier to keep track of new version.

Not pretending that there is a good solution to the problem. An additional consideration is licensing, this is MIT, can we ship it with the rest of the code of the sage documentation?

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 7, 2016

comment:29

It is not overkill, because the tarball of "the whole repo" is 1.2M while "the only one file" is 1M. Indeed this only one file is the compiled version of the rest (from coffee+less+js to js), which is the source (+ a few examples). Most js library ship things that way with the compiled result included. Compressed, it would make a 300K archive. Of course, we could make a sage-src script to recreate such a tarball. The proper way would be to ship only the source part and recompile the js "binary" ourselves, but it will require packaging coffescript and dotless (and perhaps other things). With the current situation, we have a well-defined origin for the zip-ball, easy to fetch from upstream, easy to update, easy to maintain, which follows the standard Sage packaging process.

Hardcoding things within src/ is much worse, since:

  • almost nobody will notice when the package bitrots
  • we create a separate process than standard packaging, so only experts will be able to update it
  • each time the package is updated, we get one additional magabyte in the Sage git repository (note that thebe.js is a compiled version, so the file is basically a single 1M line of js that will change at each update). I prefer by far having the git tree only explode by a few bytes (version number + hashes) at each update than 1M at each update.
  • actually, i have hesitated to rewrite the git history of Live documentation in Jupyter using Thebe #20690 and remove the fact that this 1M of js is already once in Sage source repository.

The idea behind the current solution is in minimizing maintenance work, while staying transparent. If you insist, i can write a sage-src script to shrink the zipball to save 900K and let us become upstream, but i doubt there is a benefit.

@kiwifb
Copy link
Member

kiwifb commented Sep 7, 2016

comment:30

Fine. You do the maintenance here, not me.

@slel
Copy link
Member

slel commented Sep 7, 2016

comment:31

Replying to @sagetrac-tmonteil:

It is not overkill, because the tarball of "the whole repo" is 1.2M while "the only one file" is 1M.

Thanks for your thorough explanation. It's very different from the examples I quoted.

@jdemeyer
Copy link

jdemeyer commented Sep 7, 2016

comment:32

I agree with Thierry. Distributions will be happy when we don't bundle external stuff in the Sage library.

@kiwifb
Copy link
Member

kiwifb commented Sep 7, 2016

comment:33

Replying to @jdemeyer:

I agree with Thierry. Distributions will be happy when we don't bundle external stuff in the Sage library.

I generally agree with you on that point. I started getting agitated on this ticket after looking into packaging this for Gentoo and going WTF - what am I even supposed to do. Possibly I don't have experience in packaging .js stuff but it really left me wondering. If someone with experience packaging for debian would comment on that kind of case I'd be delighted.

@vbraun
Copy link
Member

vbraun commented Sep 8, 2016

Changed branch from u/tmonteil/package_thebe_js to 520cdd0

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 27, 2019

Changed keywords from sd75 to sd75, sdl

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

5 participants