Opened 2 years ago

Closed 2 years ago

#21309 closed enhancement (fixed)

Package Thebe

Reported by: tmonteil Owned by:
Priority: major Milestone: sage-7.5
Component: packages: standard Keywords: sd75
Cc: fcayre, vdelecroix, nthiery, slelievre Merged in:
Authors: Thierry Monteil Reviewers: Nicolas M. Thiéry, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 520cdd0 (Commits) Commit: 520cdd0796488eebc64c3796e7030504ab941661
Dependencies: Stopgaps:

Description (last modified by slelievre)

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

Change History (34)

comment:1 Changed 2 years ago by tmonteil

  • Branch set to u/tmonteil/package_thebe_js

comment:2 Changed 2 years ago by tmonteil

  • Cc fcayre vdelecroix nthiery slelievre added
  • Commit set to 7d4fe864b00482ed97a0fc1f98a8150e5c83fc84
  • Status changed from new to needs_review

New commits:

7d4fe86#21309 : package thebejs.

comment:3 Changed 2 years ago by tmonteil

  • Keywords sd75 added

comment:4 Changed 2 years ago by nthiery

  • Branch changed from u/tmonteil/package_thebe_js to u/nthiery/package_thebe_js

comment:5 Changed 2 years ago by git

  • Commit changed from 7d4fe864b00482ed97a0fc1f98a8150e5c83fc84 to e1b914a7ec935bd6fadb8eb1cd8f6ced77b26bd5

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

comment:6 Changed 2 years ago by nthiery

  • Description modified (diff)
  • Reviewers set to Nicolas M. Thiéry
  • Status changed from needs_review to positive_review

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!

comment:7 Changed 2 years ago by fbissey

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

comment:8 Changed 2 years ago by tmonteil

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

comment:9 Changed 2 years ago by fbissey

It would probably work :)

comment:10 Changed 2 years ago by tmonteil

  • Status changed from positive_review to needs_work

comment:11 Changed 2 years ago by tmonteil

  • Branch changed from u/nthiery/package_thebe_js to u/tmonteil/package_thebe_js

comment:12 Changed 2 years ago by tmonteil

  • Commit changed from e1b914a7ec935bd6fadb8eb1cd8f6ced77b26bd5 to 8af8f5e03450b475650b7177f243eb49e46b2808
  • Description modified (diff)
  • Status changed from needs_work to needs_review

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.

comment:13 Changed 2 years ago by vbraun

  • Milestone changed from sage-7.4 to sage-7.5

comment:14 Changed 2 years ago by tmonteil

Is it satisfactory like this ?

comment:15 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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.

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

comment:16 follow-up: Changed 2 years ago by jdemeyer

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

comment:17 Changed 2 years ago by nthiery

Hi Thierry! Just to avoid duplicated work: have you started implementing Jeroen's comments? otherwise I can have a go tomorrow morning with him.

comment:18 Changed 2 years ago by git

  • Commit changed from 8af8f5e03450b475650b7177f243eb49e46b2808 to 520cdd0796488eebc64c3796e7030504ab941661

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.

comment:19 in reply to: ↑ 16 Changed 2 years ago by tmonteil

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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.

comment:20 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:21 Changed 2 years ago by jdemeyer

  • Reviewers changed from Nicolas M. Thiéry to Nicolas M. Thiéry, Jeroen Demeyer
  • Status changed from needs_review to positive_review

Looks good.

comment:22 Changed 2 years ago by slelievre

  • Description modified (diff)
  • Summary changed from Package thebe.js to Package Thebe

Just expanding the ticket description to make it more explanatory.

comment:23 Changed 2 years ago by slelievre

  • Description modified (diff)

comment:24 follow-up: Changed 2 years ago by fbissey

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.

comment:25 in reply to: ↑ 24 Changed 2 years ago by slelievre

Replying to fbissey:

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.

comment:26 Changed 2 years ago by fbissey

Well I guess it ties in with my earlier comment on #20690 https://trac.sagemath.org/ticket/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.

comment:27 Changed 2 years ago by nthiery

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

https://trac.sagemath.org/ticket/20690#comment:24

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

comment:28 follow-up: Changed 2 years ago by fbissey

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?

comment:29 in reply to: ↑ 28 ; follow-up: Changed 2 years ago by tmonteil

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

comment:30 Changed 2 years ago by fbissey

Fine. You do the maintenance here, not me.

comment:31 in reply to: ↑ 29 Changed 2 years ago by slelievre

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

comment:32 follow-up: Changed 2 years ago by jdemeyer

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

comment:33 in reply to: ↑ 32 Changed 2 years ago by fbissey

Replying to jdemeyer:

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

<putting distro hat on> 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.

comment:34 Changed 2 years ago by vbraun

  • Branch changed from u/tmonteil/package_thebe_js to 520cdd0796488eebc64c3796e7030504ab941661
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.