Opened 6 months ago

Closed 5 months ago

#28007 closed enhancement (fixed)

Keep upstream three.js directory structure

Reported by: gh-timokau Owned by:
Priority: major Milestone: sage-8.9
Component: packages: standard Keywords: threejs
Cc: arojas, fbissey, saraedum, gh-timokau, paulmasson Merged in:
Authors: Timo Kaufmann Reviewers: Paul Masson
Report Upstream: N/A Work issues:
Branch: 4a69f3b (Commits) Commit: 4a69f3bacfbc7c0864ab4097080773bfe6590422
Dependencies: Stopgaps:

Description

Currently sage flattens the threejs directory structure. It still needs to take the actual structure into account when using threejs from a CDN though.

I don't see a reason to flatten the structure. It should be easier for distributions (certainly is for nixos) to keep the structure.

I have only tested this with the nix package, since it is a pain to get the sage build system working on nixos. Would appreciate it if someone else could test the build/ changes (simple make ptestlong).

Attachments (2)

threejs-r100.tar.gz (139.9 KB) - added by gh-timokau 6 months ago.
Soruce tarball
threejs-r105.tar.gz (145.2 KB) - added by gh-timokau 6 months ago.
Up-to-date source tarball

Download all attachments as: .zip

Change History (41)

comment:1 Changed 6 months ago by gh-timokau

  • Authors set to Timo Kaufmann
  • Status changed from new to needs_review

comment:2 Changed 6 months ago by git

  • Commit changed from 38e420749bcd8a71319a06ebf8962ba5074335b0 to 68b6f8c3f88c9893807e55f00a8ce8adb6f50fd1

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

68b6f8cKeep the upstream threejs directory structure

comment:3 Changed 6 months ago by fbissey

If you do that, the package will need a rev-bump to be re-installed.

comment:4 Changed 6 months ago by egourgoulhon

  • Cc paulmasson added

comment:5 follow-up: Changed 6 months ago by paulmasson

  • Keywords threejs added

Timo, please explain how this makes things simpler for a distribution. It seems to me like extra complication for two files. Are you thinking of having some automatic mechanism for shifting between local files and a CDN?

comment:6 Changed 6 months ago by fbissey

What are you installing in nix to find it a burden to change the structure? Because it didn't appear that we are using an upstream tarball I just used sage's one in sage-on-gentoo. Admittedly, I call the package threejs-sage-extension and it is installed in a sage related location rather then something more generic.

I am curious of your install procedure and if it make sense for me to follow suite.

comment:7 Changed 6 months ago by fbissey

Regardless of anything else THREEJSDIR should be used like you do in this branch even if we don't end up flattening the structure. That would be worth a ticket by itself.

comment:8 Changed 6 months ago by git

  • Commit changed from 68b6f8c3f88c9893807e55f00a8ce8adb6f50fd1 to 1f7d28e6728143f6dc496c19b1f93abcc620d107

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

ed5477aConsistently THREEJS_DIR for threejs location
1f7d28eKeep the upstream threejs directory structure

comment:9 Changed 6 months ago by gh-timokau

I bumped the patch version and made a separate commit for the THREEJS_DIR usage.


New commits:

ed5477aConsistently THREEJS_DIR for threejs location
1f7d28eKeep the upstream threejs directory structure

comment:10 in reply to: ↑ 5 ; follow-up: Changed 6 months ago by gh-timokau

Replying to paulmasson:

Timo, please explain how this makes things simpler for a distribution. It seems to me like extra complication for two files. Are you thinking of having some automatic mechanism for shifting between local files and a CDN?

Well sage doesn't actually build anything, it just cherry-picks "binaries" (or as close as javascript can come to binaries) from a CDN. If you actually "build" the whole threejs package, you'll end up with the upstream directory structure. I'd need to patch sage or create a separate "interface" package that only symlinks the two files to the places sage expects them.

@fbissey we have some (suboptimal) npm infrastructure to package javascript packages, that is what I'm using. As far as I understand it (I haven't really looked into it) it scrapes npm for the dependencies, generates the build recipes, fetches each package from the npm registry and then builds them. So basically a wrapper around npm that takes over all network activity while still using npm for the actual building.

comment:11 follow-up: Changed 6 months ago by fbissey

Right, npm stuff is a pain. I don't see an updated package-version.txt and checksums.ini. But technically the real problem is you want to change the tarball without changing the version. That is a bit of a challenge. The nice way to do that is to do an upgrade at the same time so the problem with the tarball change is avoided.

comment:12 Changed 6 months ago by git

  • Commit changed from 1f7d28e6728143f6dc496c19b1f93abcc620d107 to 20b010c09cfd33f035371c79a3466372018bf763

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

20b010cKeep the upstream threejs directory structure

comment:13 follow-up: Changed 6 months ago by git

  • Commit changed from 20b010c09cfd33f035371c79a3466372018bf763 to 4a69f3bacfbc7c0864ab4097080773bfe6590422

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

4a69f3bUpdate threejs to r105

comment:14 follow-up: Changed 6 months ago by gh-timokau

Right, somehow failed to push that.

Why is that a challenge? Can't I just add .p1 to the version? In any case, I also added an update.

comment:15 in reply to: ↑ 11 Changed 6 months ago by gh-timokau

Replying to fbissey:

Right, npm stuff is a pain.

Life was good when there was only make and autotools. At least for packagers that is.

comment:16 in reply to: ↑ 10 ; follow-up: Changed 6 months ago by paulmasson

Replying to gh-timokau:

Replying to paulmasson:

Timo, please explain how this makes things simpler for a distribution. It seems to me like extra complication for two files. Are you thinking of having some automatic mechanism for shifting between local files and a CDN?

Well sage doesn't actually build anything, it just cherry-picks "binaries" (or as close as javascript can come to binaries) from a CDN. If you actually "build" the whole threejs package, you'll end up with the upstream directory structure. I'd need to patch sage or create a separate "interface" package that only symlinks the two files to the places sage expects them.

As the main author of the Three.js viewer, I can practically guarantee that Sage will never build all of Three.js since only a small portion of the entire library will ever get used in practice. Given that, keeping the original directory structure really is an extra complication without much practical gain.

comment:17 in reply to: ↑ 13 ; follow-up: Changed 6 months ago by paulmasson

  • Status changed from needs_review to needs_work

Replying to git:

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

4a69f3bUpdate threejs to r105

In the context of Three.js this is a bad idea. Upgrades are very often not backward compatible. An upgrade of Three.js should always happen on a separate ticket to be sure the template I wrote still works properly with the new version. Believe me, there were definite issues the last time I did an upgrade. Unless you use Three.js on a regular basis and are certain nothing is broken, please remove this upgrade.

comment:18 in reply to: ↑ 16 ; follow-up: Changed 6 months ago by gh-timokau

Replying to paulmasson:

As the main author of the Three.js viewer, I can practically guarantee that Sage will never build all of Three.js since only a small portion of the entire library will ever get used in practice

Yes, but there is value in standardizing the directory structure. While sage only packages threejs for a single purpose, distributions usually have multiple consumers in mind. If every package expects the threejs files in a different place, everyones lives are harder.

I don't really see much of a complication here. Since it makes the offline code closer to the one that fetches from the CDN, it may even simplify in the future. But in any case, I don't see any complication for sage.

comment:19 Changed 6 months ago by git

  • Commit changed from 4a69f3bacfbc7c0864ab4097080773bfe6590422 to 20b010c09cfd33f035371c79a3466372018bf763

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

comment:20 in reply to: ↑ 17 ; follow-up: Changed 6 months ago by gh-timokau

Replying to paulmasson:

Replying to git:

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

4a69f3bUpdate threejs to r105

In the context of Three.js this is a bad idea. Upgrades are very often not backward compatible. An upgrade of Three.js should always happen on a separate ticket to be sure the template I wrote still works properly with the new version. Believe me, there were definite issues the last time I did an upgrade. Unless you use Three.js on a regular basis and are certain nothing is broken, please remove this upgrade.

Okay, I've undone that commit. Does anyone know how spkg-src works with volkers workflow? Do I need to upload the new tarball anywhere?

comment:21 in reply to: ↑ 20 Changed 6 months ago by paulmasson

Replying to gh-timokau:

Replying to paulmasson:

Replying to git:

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

4a69f3bUpdate threejs to r105

In the context of Three.js this is a bad idea. Upgrades are very often not backward compatible. An upgrade of Three.js should always happen on a separate ticket to be sure the template I wrote still works properly with the new version. Believe me, there were definite issues the last time I did an upgrade. Unless you use Three.js on a regular basis and are certain nothing is broken, please remove this upgrade.

Okay, I've undone that commit. Does anyone know how spkg-src works with volkers workflow? Do I need to upload the new tarball anywhere?

Just attach it to this ticket. Volker does the rest.

comment:22 in reply to: ↑ 18 Changed 6 months ago by paulmasson

Replying to gh-timokau:

Replying to paulmasson:

As the main author of the Three.js viewer, I can practically guarantee that Sage will never build all of Three.js since only a small portion of the entire library will ever get used in practice

Yes, but there is value in standardizing the directory structure. While sage only packages threejs for a single purpose, distributions usually have multiple consumers in mind. If every package expects the threejs files in a different place, everyones lives are harder.

Then I guess I don’t have an objection, but would like other packagers to chime in.

Changed 6 months ago by gh-timokau

Soruce tarball

comment:23 Changed 6 months ago by gh-timokau

  • Status changed from needs_work to needs_review

comment:24 in reply to: ↑ 14 Changed 6 months ago by fbissey

Replying to gh-timokau:

Right, somehow failed to push that.

Why is that a challenge? Can't I just add .p1 to the version? In any case, I also added an update.

Because pkg-$version and pkg-$version.p$number are supposed to have the same tarball in sage's package management system. Which is a problem when the online package repo has to hold tarballs for several versions of sage, see http://mirror.aarnet.edu.au/pub/sage/spkg/upstream/threejs/index.html for a relevant example. This is where patchbot gets their packages by the way.

comment:25 Changed 6 months ago by gh-timokau

  • Status changed from needs_review to needs_work

comment:26 follow-up: Changed 6 months ago by gh-timokau

Ah, I see. That is annoying. @paulmasson since you've added the package and made the last two updates, do you plan to update it again in the future? If so, would you mind just basing that update on my branch?

That'd be the easiest solution, and I'm not in a hurry (I'm already patching sage anyways).

comment:27 in reply to: ↑ 26 Changed 6 months ago by paulmasson

Replying to gh-timokau:

Ah, I see. That is annoying. @paulmasson since you've added the package and made the last two updates, do you plan to update it again in the future? If so, would you mind just basing that update on my branch?

That'd be the easiest solution, and I'm not in a hurry (I'm already patching sage anyways).

Just did some local testing with r105 and didn't see any major problems with this version. If you want to add the upgrade back in then I'll review the ticket. Sorry for creating the extra work for you, but I always try to remind people of possible issues with upgrading Three.js.

comment:28 Changed 6 months ago by git

  • Commit changed from 20b010c09cfd33f035371c79a3466372018bf763 to 4a69f3bacfbc7c0864ab4097080773bfe6590422

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

4a69f3bUpdate threejs to r105

Changed 6 months ago by gh-timokau

Up-to-date source tarball

comment:29 Changed 6 months ago by gh-timokau

Not a problem, dealing with backwards incompatibility is always annoying. Update re-added.


New commits:

4a69f3bUpdate threejs to r105

comment:30 follow-up: Changed 6 months ago by saraedum

gh-timokau: #26434 might also make this problem go away? (In a somewhat brutal manner.)

comment:31 Changed 6 months ago by fbissey

I think it may indeed. It certainly addresses some points raised here. I quite like #26434 (beside the obvious reason of removing installed_packages).

comment:32 follow-up: Changed 6 months ago by fbissey

To be clear, there should be coordination between the tickets. #26434 does a lot of the dealing internally with script locations. But this ticket is talking about changing the way threejs is packaged and installed. And because the cleanest way to achieve this is an upgrade, it does that too.

Adopting #26434 means this ticket can do its packaging changes without touching "sage"'s code.

comment:33 in reply to: ↑ 30 Changed 6 months ago by paulmasson

Replying to saraedum:

gh-timokau: #26434 might also make this problem go away? (In a somewhat brutal manner.)

That ticket appears to remove the online, i.e. CDN scripts, completely and only uses local scripts. How will that allow people to create and share notebooks? The CDN fallback was fixed in #27575 and would likely have addressed the issues you encountered at the conference.

comment:34 Changed 6 months ago by fbissey

@paulmasson the issue that #26434 originally seeks to address is that sage is calling its internal package management to figure out the version of threejs to look for. For us on distro it is not feasible or desirable.

comment:35 in reply to: ↑ 32 ; follow-up: Changed 6 months ago by gh-timokau

Replying to fbissey:

Adopting #26434 means this ticket can do its packaging changes without touching "sage"'s code.

How is that? #26434 still contains this line:

scripts = [os.path.join(THREEJS_DIR, script) for script in ['three.min.js', 'OrbitControls.js']]

comment:36 in reply to: ↑ 35 Changed 6 months ago by fbissey

Replying to gh-timokau:

Replying to fbissey:

Adopting #26434 means this ticket can do its packaging changes without touching "sage"'s code.

How is that? #26434 still contains this line:

scripts = [os.path.join(THREEJS_DIR, script) for script in ['three.min.js', 'OrbitControls.js']]

You are right. I misread something this morning, I thought it was walking in any subdirectories for some reason.

comment:37 Changed 6 months ago by paulmasson

  • Milestone changed from sage-8.8 to sage-8.9
  • Reviewers set to Paul Masson
  • Status changed from needs_work to positive_review

LGTM

comment:38 Changed 6 months ago by gh-timokau

Thank you for the review!

comment:39 Changed 5 months ago by vbraun

  • Branch changed from u/gh-timokau/threejs-directory-structure to 4a69f3bacfbc7c0864ab4097080773bfe6590422
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.