The Wayback Machine - http://web.archive.org/web/20201209215516/https://github.com/linuxmint/cjs/issues/80
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

Switch to spidermonkey/mozjs 78 #80

Closed
mkg20001 opened this issue Nov 29, 2019 · 64 comments
Closed

Switch to spidermonkey/mozjs 78 #80

mkg20001 opened this issue Nov 29, 2019 · 64 comments
Assignees

Comments

@mkg20001
Copy link

@mkg20001 mkg20001 commented Nov 29, 2019

The Firefox ESR 52 is no longer maintained, as such it likely contains unfixed security vulnerabilities.

Firefox ESR 60 is the next ESR, which is currently maintained.

Could CJS be updated to support mozjs60?

@mkg20001 mkg20001 mentioned this issue Nov 29, 2019
2 of 10 tasks complete
@clefebvre
Copy link
Member

@clefebvre clefebvre commented Nov 29, 2019

Yes, nothing new here. CJS needs to bump towards a new mozjs now and then. The next bump is likely to be towards mozjs60. CJS 4.4 is using mozjs52, that won't change. The bump could happen for 4.6 in the next development cycle or failing that in 4.8.

@jtojnar
Copy link

@jtojnar jtojnar commented Nov 30, 2019

By that time we should make it mozjs68 https://gitlab.gnome.org/GNOME/gjs/issues/270

@mkg20001 mkg20001 changed the title Switch to spidermonkey/mozjs 60 Switch to spidermonkey/mozjs 68 Nov 30, 2019
schnitzeltony added a commit to schnitzeltony/meta-gnome-forks that referenced this issue Jan 11, 2020
* Stolen from meta-oe right before mozjs was upgraded to 60.x
* In the long term mozjs-52 can go because cinnamon intends to move to mozjs-68
  [1]. That is not yet available in any layer as far sa I know (off-topic have
  started work on mozjs-68 to get rid of python2 but did not yet get it to fly)
* build might fail on other arches than arm but that is good enough for me

[1] linuxmint/cjs#80

Signed-off-by: Andreas Müller <schnitzeltony@gmail.com>
@norbusan
Copy link

@norbusan norbusan commented Jul 1, 2020

Hi @clefebvre

Debian maintainer of Cinnamon here.

Yes, nothing new here. CJS needs to bump towards a new mozjs now and then. The next bump is likely to be towards mozjs60. CJS 4.4 is using mozjs52, that won't change. The bump could happen for 4.6 in the next development cycle or failing that in 4.8.

Do you have plans to implement the switch for 4.8? On the Debian side we are now phasing out the old mozjs and the switch is becoming a serious bug.

Thanks for all your work

Norbert

@mtwebster mtwebster self-assigned this Jul 2, 2020
@Fantu
Copy link
Contributor

@Fantu Fantu commented Aug 17, 2020

An update from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908234:

Neither is mozjs60, and it has been removed from Debian. All other
mozjs-dependent packages are now using mozjs68, based on Firefox 68 ESR.

Additionally, as a warning for the future: the upstream development
branch for GNOME/gjs has moved to the JavaScript engine from Firefox
78, which will be packaged in Debian as mozjs78, for GNOME 3.38. This
hasn't happened in Debian yet (see #968341 to track progress), but based
on the GNOME release schedule and the Debian freeze plans, I expect we
will ship bullseye with GNOME 3.38 and mozjs78.

@norbusan
Copy link

@norbusan norbusan commented Sep 27, 2020

Update 2020-09

And another Firefox ESR release cycle. We now have mozjs78 in unstable
and in use by gjs; this is the version that GNOME will be using in
Debian 11 'bullseye', unless the freeze happens much later than expected.

This means we now have:

mozjs52: cjs
(mozjs60 removed)
mozjs68: a libproxy plugin that doesn't actually work (probably removed soon)
mozjs78: gjs, policykit-1/experimental

We hope to remove mozjs68 soon, leaving only mozjs52 (for cjs) and mozjs78
(for gjs and policykit-1).

Any chance for an update here?

@Fantu
Copy link
Contributor

@Fantu Fantu commented Sep 28, 2020

#84 (thanks to leigh123linux)
fedora 34 seems already using it: https://src.fedoraproject.org/rpms/cjs/c/e92a7f16437e29a296621103a4f9daff7142e185?branch=master
If I'll have time next weekend I'll test it in debian unstable

about CI failing is for not related error, @clefebvre can you take a look please?:

  Error pulling image linuxmintd/mint20-amd64: error pulling image configuration: unknown blob... retrying
  image cache not found on this host, downloading linuxmintd/mint20-amd64
latest: Pulling from linuxmintd/mint20-amd64

1f796a1e: Already exists 
ea53ad12: Already exists 
71e02073: Already exists 
17bbf772: Already exists 
665c555b: Pulling fs layer 
90346da7: Pulling fs layer 
eb728a8a: Pulling fs layer 
4f53d3aa: Pulling fs layer 
e5bae1da: Pulling fs layer 
ab57eb86: Pulling fs layer 
218f1484: Pulling fs layer 
40121a67: Pulling fs layer 
a26d395c: Pulling fs layer 
e3eb6afc: Waiting >s layer 

error pulling image configuration: unknown blob
@mkg20001
Copy link
Author

@mkg20001 mkg20001 commented Sep 28, 2020

I'm currently rebuilding cinnamon on nixOS with the PR, I'll post an update once I have some results

@norbusan
Copy link

@norbusan norbusan commented Sep 28, 2020

@Fantu @mkg20001 thanks. I have taken a look at this PR, but a about 400 files and tens of thousands of lines touched, that looks - well - interesting. Let us hope. I will give it a try here, too.

@leigh123linux
Copy link
Member

@leigh123linux leigh123linux commented Sep 28, 2020 •

#84 (thanks to leigh123linux)
fedora 34 seems already using it: https://src.fedoraproject.org/rpms/cjs/c/e92a7f16437e29a296621103a4f9daff7142e185?branch=master
If I'll have time next weekend I'll test it in debian unstable

about CI failing is for not related error, @clefebvre can you take a look please?:

  Error pulling image linuxmintd/mint20-amd64: error pulling image configuration: unknown blob... retrying
  image cache not found on this host, downloading linuxmintd/mint20-amd64
latest: Pulling from linuxmintd/mint20-amd64

1f796a1e: Already exists 
ea53ad12: Already exists 
71e02073: Already exists 
17bbf772: Already exists 
665c555b: Pulling fs layer 
90346da7: Pulling fs layer 
eb728a8a: Pulling fs layer 
4f53d3aa: Pulling fs layer 
e5bae1da: Pulling fs layer 
ab57eb86: Pulling fs layer 
218f1484: Pulling fs layer 
40121a67: Pulling fs layer 
a26d395c: Pulling fs layer 
e3eb6afc: Waiting >s layer 

error pulling image configuration: unknown blob

I switched cinnamon for f34 to use gjs

https://src.fedoraproject.org/rpms/cinnamon/commits/master

The change to cjs was to keep it in stasis so it doesn't get retired, also a couple of third-party applets use cjs.
I have tested the cjs port and it seems ok.

The cjs port also requires some changes to cinnamon code

https://leigh123linux.fedorapeople.org/pub/patches/new_cjs.patch

@leigh123linux
Copy link
Member

@leigh123linux leigh123linux commented Sep 28, 2020

FTR I believe @clefebvre intends to use mozjs68 for Mint.
The thought of maintaining mozjs68 for the next 4 years horrifies me, hence the switch.

@norbusan
Copy link

@norbusan norbusan commented Sep 28, 2020

@leigh123linux I agree!

@mkg20001
Copy link
Author

@mkg20001 mkg20001 commented Sep 28, 2020

I got some warnings during session start

Sep 28 14:58:55 nixos .cinnamon-wrapp[1164]: JS LOG: [LookingGlass/info] Cinnamon.AppSystem.get_default() started in 600 ms
Sep 28 14:58:55 nixos .cinnamon-wrapp[1164]: Some code tried to set a deprecated GObject property.
                                             0 start() ["/nix/store/2s0m1lfi32qzp3jdvgvhj2wbs6x187ql-cinnamon-common-4.6.1/share/cinnamon/js/ui/main.js":330:4]
                                             1 <TOP LEVEL> ["<main>":1:46]

Sep 28 14:58:55 nixos .cinnamon-wrapp[1164]: JS LOG: [LookingGlass/info] added icon directory: /run/current-system/sw/share/themes/Mint-Y-Dark/cinnamon
Sep 28 14:58:55 nixos .cinnamon-wrapp[1164]: Some code accessed the property 'BackgroundManager' on the module 'backgroundManager'. That property was defined with 'let' or 'const' inside the module. This was previously supported, but is not correct according to the ES6 standard. Any symbols to be exported from a module must be defined with 'var'. The property access will work as previously for the time being, but please fix your code anyway.

Sep 28 14:58:58 nixos .cinnamon-wrapp[1164]: JS LOG: [LookingGlass/info] Loaded applet sound@cinnamon.org in 790 ms
Sep 28 14:58:59 nixos .csd-power-wrap[980]: abs_to_percentage: assertion 'max > min' failed

Besides that seems to work with @leigh123linux cjs & cinnamon patch

@Fantu
Copy link
Contributor

@Fantu Fantu commented Sep 28, 2020

I suppose that clem didn't switch to newer mozjs because want a mozjs present in both latest mint version, mint20 based on focal have mozjs 52 and 68, lmde 4 based on buster that have mozjs 52 and 60, then mozjs 52 is the only present on both.
So if we want to see cjs updated upstream before the next debian and ubuntu lts releases (instead of keeping large cjs and cinnamon patches maintained by fedora, debian (and other distros) I suppose we have to give clem very good reasons for moving to new mozjs and the easier/faster way to get it in buster and focal (or put in mint20 and lmde4)

@leigh123linux
Copy link
Member

@leigh123linux leigh123linux commented Sep 28, 2020

@mkg20001

This one is new

                                             0 start() ["/nix/store/2s0m1lfi32qzp3jdvgvhj2wbs6x187ql-cinnamon-common-4.6.1/share/cinnamon/js/ui/main.js":330:4]
                                             1 <TOP LEVEL> ["<main>":1:46]

The ES6 standard warnings were present in the virgin code.

@mkg20001
Copy link
Author

@mkg20001 mkg20001 commented Sep 28, 2020

@leigh123linux Ok, seems like it's just a warning tho for now, at least didn't see anything broken

Also there's a PR for nixOS to use the patched version for the time being, so we are at least no longer depending on the insecure mozjs52 NixOS/nixpkgs#99008

@leigh123linux
Copy link
Member

@leigh123linux leigh123linux commented Sep 28, 2020

I suppose that clem didn't switch to newer mozjs because want a mozjs present in both latest mint version, mint20 based on focal have mozjs 52 and 68, lmde 4 based on buster that have mozjs 52 and 60, then mozjs 52 is the only present on both.
So if we want to see cjs updated upstream before the next debian and ubuntu lts releases (instead of keeping large cjs and cinnamon patches maintained by fedora, debian (and other distros) I suppose we have to give clem very good reasons for moving to new mozjs and the easier/faster way to get it in buster and focal (or put in mint20 and lmde4)

I believe I've seen at least a 15% decrease in cinnamon startup time with mozjs78 using gjs or cjs.

Gjs-Message: 10:06:11.162: JS LOG: [LookingGlass/info] Cinnamon took 2093 ms to start
@mkg20001
Copy link
Author

@mkg20001 mkg20001 commented Sep 28, 2020

before:
Sep 28 16:19:08 nixos .cinnamon-wrapp[1169]: JS LOG: [LookingGlass/info] Cinnamon took 5128 ms to start

after:
Sep 28 16:17:27 nixos .cinnamon-wrapp[1175]: JS LOG: [LookingGlass/info] Cinnamon took 4546 ms to start

similar results here

@ItzSwirlz
Copy link

@ItzSwirlz ItzSwirlz commented Sep 28, 2020

Let's just try to make the switch, I'll try to build with mozjs78

Clem, I don't know if you are staying with 52 because it's in Ubuntu Focal and you can put it in Mint Repos, but I suggest that everyone as a whole, who packages Cinnamon and uses it for any distros other than Mint, such as leigh123linux for Fedora, need to get this fixed ASAP before this becomes project suicide.

@mkg20001 mkg20001 changed the title Switch to spidermonkey/mozjs 68 Switch to spidermonkey/mozjs 78 Sep 29, 2020
@leigh123linux
Copy link
Member

@leigh123linux leigh123linux commented Sep 30, 2020

@leigh123linux
Copy link
Member

@leigh123linux leigh123linux commented Sep 30, 2020

@Fantu @mkg20001 thanks. I have taken a look at this PR, but a about 400 files and tens of thousands of lines touched, that looks - well - interesting. Let us hope. I will give it a try here, too.

If that cjs patch is too large there is another path, use gjs instead.

https://src.fedoraproject.org/rpms/cinnamon/blob/master/f/switch_to_gjs.patch

@Fantu
Copy link
Contributor

@Fantu Fantu commented Sep 30, 2020

remove a fork too little maintained it would be beautiful but unfortunately if cinnamon is not keep updated for the compatibility of gjs versions as soon as they come out there is a risk of more problems, so keep the fork (but upstream and not only single distros patches, for less waste of collective time and better results) I suppose it is necessary to have less risk of bugs/unexpected events, especially for rolling releases distro (or similar)

@eli-schwartz
Copy link
Contributor

@eli-schwartz eli-schwartz commented Sep 30, 2020

Arch is not planning to remove either mozjs52 or cinnamon at the moment.

I've been watching this issue for ~6 months and I'm happy to see there's movement to getting this resolved as I'd quite like to consolidate on one recent mozjs too. But I'd really like to see the PR merged before packaging it, and I'm disinclined to apply a distro-specific patch. Even if the patch is specific to more than one distro.

This needs to be fixed properly rather than requiring every distro to maintain out of tree patches.

@eli-schwartz
Copy link
Contributor

@eli-schwartz eli-schwartz commented Sep 30, 2020

What are the advantages and disadvantages of using gjs directly? If it's simple to do, I'm a bit curious why cinnamon ended up forking it to cjs back in the day...

@leigh123linux
Copy link
Member

@leigh123linux leigh123linux commented Sep 30, 2020

What are the advantages and disadvantages of using gjs directly? If it's simple to do, I'm a bit curious why cinnamon ended up forking it to cjs back in the day...

gjs used to break abi every release so cjs was forked to solve this, since then gjs change between releases seems to have slowed/stopped.
The patch for cinnamon to use gjs supports many releases gjs >= 1.56.0

@norbusan
Copy link

@norbusan norbusan commented Oct 1, 2020

whatever the future choice I think it is better that it is applied upstream and shared by the distro otherwise more collective time would be wasted and there is risk of more bugs/unexpected cases

Agree in principle, but distributors have to do something now, and cannot wait for whatever might come at an unspecified time in the future. So for now I think the update as mentioned is necessary, at least for some here.

@Fantu
Copy link
Contributor

@Fantu Fantu commented Oct 1, 2020 •

for cinnamon itself from what i have read so far it seems better switch to gjs.
about applets and desklets I have not looked well but even from quick search at least some seem "forced" on cjs, if the change occurs upstream for the next version applets/desklets will be modified for compatibility with new cinnamon version otherwise they risk not working with the distros that will remove cjs.
( https://github.com/linuxmint/cinnamon-spices-desklets https://github.com/linuxmint/cinnamon-spices-applets )

@Fantu
Copy link
Contributor

@Fantu Fantu commented Oct 2, 2020 •

I did some tests:

cinnamon using cjs with mozjs52 (debian unstable, same of upstream):
info t=2020-10-02T12:05:46Z Cinnamon took 972 ms to start (without additional applets)

cinnamon using updated cjs with mozjs78 (debian experimental, using patches from leigh123linux):
info t=2020-10-02T11:33:37Z Cinnamon took 743 ms to start (without additional applets)
no additional warning/errors in melange (including some additional applets I had installed)

cinnamon using gjs (deb https://www.preining.info/debian unstable cinnamon main, using patches from leigh123linux) and removing cjs:
info t=2020-10-02T11:49:09Z Cinnamon took 776 ms to start (without additional applets)
no additional warning/errors in melange (including some additional applets I had installed)
but there are some applets or it parts (seems few) that call additional js files launching them with cjs, for example multicore-sys-monitor preferences didn't working without cjs because...
this.childProcessHandler = new SpawnProcess.ProcessSpawnHandler(this.metadata.path, ['cjs', 'prefs.js', currentPreferences]);
I suppose would be good to warn applets developers where there are parts that force cjs to modify them (for not have issues on future return to gjs, or if some distro will do it recently) and add in the readme of the repository with the instructions for the developers a note to avoid this in future applets

@clefebvre can you say something please?
If for example you want chooice to accept the cjs update but you don't have time to prepare mozjs78 backports for buster and focal (for upload in mint20 and lmde4) maybe I have time this weekend to make the necessary changes to the packages and test their build; similar for the case of gjs switch, where in focal gjs is enough new and would be needed only the the backports of gjs/mozjs in buster

thanks for any reply and sorry for my bad english

@norbusan
Copy link

@norbusan norbusan commented Oct 2, 2020

Thanks @Fantu for the detailed testing. I think the case that applets rely on cjs being present can be worked around by cinnamon conflicting with cjs, and shipping a cjs link to gjs.

@lestcape
Copy link

@lestcape lestcape commented Oct 6, 2020

This is just my opinion and not more, but as i mentioned more less the same in gjs, i think now is time to said it here.

I think it is possible to continue trying to convince someone who apparently don't want to be convinced or instead probably the best is contribute to port cjs to the last gjs code, but that without to lost support for the Mint spidermonkey active version. In that way should be reduced the number of changes that are need as all of that was included with enough time in the master branch. That move will also help to port cjs to a new version of gjs when Mint decide, because what will be needed was already included in cjs and all package that depend of it like the spices (extensions).

As it's important reduce the difference between cjs and gjs as possible, one way of how this could be done is if all different code between both can be assimilated by gjs and/or by cjs in the best way for both. Also, when something can not be assimilate in gjs or cjs, the best could be inject the difference in the Cinnamon shell instead of modify the cjs package. For example, this is used in Cinnamon to override some Gio functionalities that some GNOME developers don't want to change:
https://github.com/linuxmint/cinnamon/blob/master/js/ui/overrides.js#L58-L98

But exactly this code is not needed anymore because gjs assimilated it and create a much robust solution than what it is in cjs actually. Unfortunately no one ported the robust solution of gjs back to cjs and this is the kind of things that shouldn't happen, because it is a thing that was already done and then it only create a difference between the two package just for nothing.

There are a lot of change that are coming and they should be prevented with enough time:
https://gitlab.gnome.org/GNOME/gjs/-/merge_requests/324

@clefebvre
Copy link
Member

@clefebvre clefebvre commented Oct 7, 2020 •

Hi everyone,

Thanks for raising this issue and contributing to this discussion. Sorry for the time it took me to join you. We're going to prioritize this issue and resolve it ASAP and for everybody.

I'd like to thank @ptomato also for his message and talk a little bit about GJS. CJS is not a "real" fork. It's a fork of course, but in terms of project, it has no ambition to be any different than GJS. None of the changes which were added to CJS were envisioned or planned when the fork first occurred, some of them were ported upstream, and those which weren't can be put in Cinnamon directly. CJS has also been rebased quite a few times on GJS. It doesn't deviate from it, it just catches up. So when it comes to "differences" between GJS and CJS, they don't matter to us per se. We don't need our own interpreter. What we need is our own "version" of the interpreter. Let me try and explain this a little bit, it's quite important.

When a distribution ships a new version of GNOME it includes a version of GJS which is appropriate and which works with that version of GNOME. When it ships a new version of Cinnamon is ships with the CJS interpreter of the same version.

In the past, when Cinnamon used GJS directly, new versions of Cinnamon were designed with the version of GJS used in Linux Mint and LMDE in mind. So although Cinnamon worked very well in these distributions, it could break in distributions which shipped newer or older versions of GJS, and these distributions could not upgrade or downgrade or freeze their version of GJS because that version was required by the version of the GNOME they shipped with.

Even if CJS was 100% identical to GJS (and it could be), what it really brings to the table is this: It allows us to know that a particular version of Cinnamon runs with the same interpreter in all distributions. It doesn't matter who's running a brand new GJS and who's running a really old one, when the new Cinnamon comes out, there's a matching CJS and everybody can use that. Distributions can ship with GNOME, Cinnamon, GJS, CJS and two versions of mozjs, what they cannot do is ship with two versions of GJS.

Now, in terms of our own targets, in addition to yours, we have the following:

  • LMDE 4 (based on Buster): mozjs60, gjs 1.54
  • Mint 20 (based on Focal): mozjs68, gjs 1.64

I assume in rolling distributions we're looking at mozjs78 and gjs 1.66 and if we were to look at compatibility with the current LTS (thankfully we're not but it's relevant all the same) we'd be looking at mozjs52 and gjs 1.52.

If we switch to GJS we become more flexible but we risk seeing issues and behavior which differ between distributions and which we might not always be able to reproduce or even fix for everybody. If we jump to mozjs78 we fix the issue for all distributions, old and new, and ensure everyone can run the next version of Cinnamon without running in specific issues. The problem then of course is that although that solves the problem for now, we'll be having the same problem again next year.

Moving to GJS is a risk in terms of compatibility because of that. Even if GJS itself tries to minimize the impact on GNOME and Cinnamon, it's not just up to them, moving to a newer mozjs can mean mandatory code changes and unlike GNOME we're not just worried about having our latest version run on the latest mozjs, we'd have to juggle with both old and new here and work with different versions if we want to port the latest Cinnamon to LMDE/Debian and Mint/Ubuntu. With CJS the version of mozjs is the same everywhere, whether that's an old 52 or the brand new 78 and that allows us to run the upcoming Cinnamon anywhere we want.

@jbicha
Copy link

@jbicha jbicha commented Oct 7, 2020

Just a brief comment:

Although it's not really taking place yet in all distros, I believe the goal for GNOME developers is for gjs updates to be backported to LTS distributions. So both GNOME Shell and a Cinnamon-GJS would need updates to continue working with the supported version of GJS. This is because mozjs, as a part of Firefox, is only supported for about one year.

@ptomato
Copy link
Contributor

@ptomato ptomato commented Oct 7, 2020

I believe the goal for GNOME developers is for gjs updates to be backported to LTS distributions.

Just a minor note; while this is a worthy goal, I do want to be clear that I and the other upstream GJS contributors don't have enough bandwidth to support this, so this is really up to the LTS distributions themselves.

@eli-schwartz
Copy link
Contributor

@eli-schwartz eli-schwartz commented Oct 7, 2020 •

The other potential approach to ensuring every distro can use the same gjs/mozjs version for cinnamon, would be to make gjs capable of having multiple versions of gjs coexist and properly versioning itself based on semantic versioning + "when you need mandatory code changes". Currently, libgjs uses the soname "0.0.0" and every other file is installed to a "1.0" namespace. I'm not actually sure what the point of this "1.0" namespace is if it actually does break compatibility though. ;)

One could then depend on old versions of vanilla gjs and package them just like, currently, multiple versions of mozjs get packaged. In spirit this is even what cjs is.

@clefebvre
Copy link
Member

@clefebvre clefebvre commented Oct 7, 2020

I believe the goal for GNOME developers is for gjs updates to be backported to LTS distributions.

Just a minor note; while this is a worthy goal, I do want to be clear that I and the other upstream GJS contributors don't have enough bandwidth to support this, so this is really up to the LTS distributions themselves.

Same here, we wouldn't be updating past Cinnamon releases to switch to newer versions of mozjs. That said, if newer versions of mozjs were backported in Debian stable and Ubuntu LTS we would definitely switch to them faster.

@Fantu
Copy link
Contributor

@Fantu Fantu commented Oct 7, 2020

@clefebvre
you say in past you wan't use -backports (for example this caused a wait of 2 years for another thing), so you could exactly tell which one(s) solution would you accept (for a possible solution upstream in a short time) please?
from your reply seems to me you prefer remain with cjs and for update it you want newer mozjs available in both buster and focal repository or you will wait (long time) for future debian and ubuntu lts versions, or I'm wrong?
and about upload mozjs78 in mint/lmde repository if someone (probably I) will prepare and test the packages building with buster and focal (without use -backports) will be ok for you? (for add leigh123linux cjs/cinnamon patches for cinnamon 4.8)
wait one year or more for an update/change upstream with many distro that will use patches for use gjs (or updated cjs) and/or create a cinnamon fork seems a collective waste of time that I think is preferable to avoid

@jbicha
Copy link

@jbicha jbicha commented Oct 7, 2020

Ok, I guess the backporting gjs thing is just a Ubuntu LTS idea that's been on a wishlist for 2 years and no one is working on it yet.

@clefebvre
Copy link
Member

@clefebvre clefebvre commented Oct 8, 2020

hi @Fantu

No, we're not waiting on anyone. We'll be fixing this ASAP in preparation for Cinnamon 4.8.

We're testing both scenarios (sorry, I just can't get used to the Latin plural form). You're right, I prefer the CJS solution but I'm also interested in seeing if we can use GJS and even if we don't, if we can reduce the difference and make it easier to switch.

Right now we're preparing mozjs78 builds for LMDE 4 and Mint 20. We'll then make a PR to port CJS (and possibly Cinnamon if code changes are needed) to it.

@Fantu
Copy link
Contributor

@Fantu Fantu commented Oct 8, 2020

@clefebvre big thanks for your work trying to solve it fast
about mozjs78 packages I did a fast look, if you fork debian packaging https://salsa.debian.org/gnome-team/mozjs (debian/78/master branch) seems only to be needed revert these patches for have it working (I not have time to test it now) in buster and focal without -backports and update of other packages:

  • debian: Use debhelper 13 variables sobstitution in *.install files
  • debian/control: Bump dependency on dh 13
  • debian/control: build-depend on the first ICU version with fixed likely subtags
@ItzSwirlz
Copy link

@ItzSwirlz ItzSwirlz commented Oct 9, 2020

@mtwebster Thank you for this commit, you're a saint getting us somewhere
linuxmint/cinnamon@686dfbe

@ItzSwirlz
Copy link

@ItzSwirlz ItzSwirlz commented Oct 9, 2020

I don't think it's a good idea to be backporting stuff-mozjs52 is becoming ancient and with Focal being supported for 5 years and Mint following itself for 4 years with this, 4.8 use GJS. IMO best idea

ptomato referenced this issue in linuxmint/cinnamon Oct 13, 2020
@ewlsh
Copy link
Contributor

@ewlsh ewlsh commented Oct 14, 2020 •

As a GJS contributor, I would love to see Cinnamon use GJS moving forward. I think it would unify the communities and reduce the maintenance burden for the GJS and CJS maintainers, distributors, and packagers but I understand the hesitancy and @clefebvre's point on versioning and stability difficulties. Ultimately, I hope Cinnamon finds the best solution for the Mint community.

That said, I would just like to clarify any "API breakage" concerns and lay out how/in what circumstances GJS could be used.

GJS will not have any API breaks until it reaches v2.x (per @ptomato), at which point distributions could continue packaging GJS v1 until Cinnamon has upgraded.

What can change, however, is our support for JS language features as SpiderMonkey evolves.

Essentially two things can happen:

  1. SpiderMonkey removes a non-standard feature
  2. SpiderMonkey adds a new standard feature

As JavaScript is now a rigorously standardized language SpiderMonkey does not, as a matter of policy, add non-standard features anymore. Most of the obvious non-standard features have since been removed. (the migration to 60 in particular removed a lot)

If Cinnamon chooses to use GJS and a newer version of GJS loses support for a non-standard feature such as String.toUpperCase...

Non-Standard (removed a few years ago):

String.toUpperCase('lowercase') // LOWERCASE

Standard:

'lowercase'.toUpperCase() // LOWERCASE

Cinnamon can issue a patch to use the standard language feature and it will continue working on both older and rolling versions of GJS.

To use GJS you just have to avoid using new features until your lowest GJS version supports them and if you're using non-standard features you should fix the usages to maintain future and current compatibility.

From what I know nothing has been removed without a standard implementation that works.

You can read through the GJS release docs and look for backwards(-incompatible) and remove for everything that has ever changed across mozjs versions.

@ewlsh
Copy link
Contributor

@ewlsh ewlsh commented Oct 14, 2020 •

Also @clefebvre thank you for your work regardless, I'm glad to see CJS moving toward mozjs78 regardless of a GJS switchover!

@lestcape
Copy link

@lestcape lestcape commented Oct 15, 2020 •

@ewlsh I think that one idea to contribute with the downstream (not only for Cinnamon), is create a way for run gjs in compatibility mode with an older version. That could be implemented internally just adding something like an script file to be loaded only when gjs is running in compatibility mode with an older version.

I will use your own example as an example to run gjs in compatibility mode with mozjs < 60. In that case we should probably add something like that:

if (!String.hasOwnProperty("toUpperCase")) {
    String.toUpperCase = function(text) {
        return text.toUpperCase();
    }
}

This could be used to reduce the gap between the gjs versions and will let gjs to move on. Of course, this don't need to be perfectly because for example, i don't see how to implement in a good way a compatibility procedure to the case of the introduction of ByteArray (Maybe there are a good approximation?). The introduction of BigInt could be also problematic to create a procedure like that if needed. Also I don' t see the case of introduce a patch to something that nobody use or when the use is reduced to a pretty specific cases.

But I think that for most common things this should help a lot. Especially for cases like Cinnamon.

testCaseApp.zip

@lestcape
Copy link

@lestcape lestcape commented Oct 15, 2020 •

For the ByteArray probably the best should be just override the most commons usage of ByteArray, that will be something like this (note, the code not work because of https://gitlab.gnome.org/GNOME/gjs/-/issues/189) :

 if (Gio.File.prototype.load_contents) {
    Gio._real_load_contents = Gio.File.prototype.load_contents;
    Gio.File.prototype.load_contents = function(cancel) {
        let contents = Gio._real_load_contents(cancel);
        if (contents[1] instanceof Uint8Array) {
            contents[1] = imports.byteArray.toString(contents[1]);
        }
        return contents;
    };
}

This code works instead. It's crappy, but the only way to go thanks to the gjs issue:

if (Gio._LocalFilePrototype && Gio._LocalFilePrototype.load_contents) {
    Gio._real_local_load_contents = Gio._LocalFilePrototype.load_contents;
    Gio._LocalFilePrototype.load_contents = function(cancel, ...args) {
        let contents = Gio.File.prototype.load_contents.call(this, cancel, ...args);
        if (contents[1] instanceof Uint8Array) {
            contents[1] = imports.byteArray.toString(contents[1]);
        }
        return contents;
    };
}

testCaseApp.zip

@clefebvre
Copy link
Member

@clefebvre clefebvre commented Oct 17, 2020 •

Hi everyone,

I've got some good news. Mozjs78 is now in Mint and LMDE and we'll be merging support for it into the master branches of CJS and Cinnamon early next week. Some of the devs have been testing this for a little while now and it looks pretty stable. As an additional bonus we're observing slight performance improvements, in particular during the init phase.

The new Cinnamon will adapt to the new JS syntax (load_contents/toString) but the spices will need to work with both old and new versions of Cinnamon. We had a few solutions in mind and one of them consisted in overriding load_contents. @mtwebster wasn't successful with trying that. Maybe @lestcape's code above will help.

So far the chosen solution is simply to use spice multiversioning (https://projects.linuxmint.com/reference/git/cinnamon-tutorials/xlet-versioning.html). 15 of them are impacted, it's quite manageable. It's not the most elegant solution but it's a proper one, multiversioning was designed specifically for retro-compatibility, whether that's older versions of Cinnamon itself or the JS interpreter it uses.

We'll also bump CJS and Cinnamon to unstable version 4.7 so we can implement proper dependencies in the packaging and multiversioning in the spices without having to wait for 4.8.

@mtwebster
Copy link
Member

@mtwebster mtwebster commented Oct 17, 2020

@lestcape I tried the first one and it didn't work, I sort of figured it was because GFile was only an interface. I missed the local file patch somehow in the overrides file.

This stinks this only addresses file:// uris but that covers probably 99% of any likely uses, I'll try that second approach, thanks.

@ewlsh
Copy link
Contributor

@ewlsh ewlsh commented Oct 17, 2020 •

@mtwebster and @clefebvre I think only fourteen are impacted because this one is using a fairly common runtime check:

const [, content] = file.load_contents(null);
let converted;

if (content instanceof Uint8Array) { // mozjs60 future-proofing
      converted = ByteArray.toString(logs);
} else {
      converted = logs.toString();
}

The other ones will print this warning if you don't patch the environment:

(gjs:62025): Gjs-WARNING **: 09:01:19.619: Some code called array.toString() on a Uint8Array instance. Previously this would have interpreted the bytes of the array as a string, but that is nonstandard. In the future this will return the bytes as comma-separated digits. For the time being, the old behavior has been preserved, but please fix your code anyway to explicitly call ByteArray.toString(array).
(Note that array.toString() may have been called implicitly.)

But can be quite easily updated with the above check and they should still function normally (albeit with a warning until updated). Several spices seem to use JSON.parse(byteArray) which should ideally be avoided moving forward regardless.

If you are going to patch the environment I'd recommend a few modifications...

if (Gio._LocalFilePrototype && Gio._LocalFilePrototype.load_contents) {
    Gio._real_local_load_contents = Gio._LocalFilePrototype.load_contents;
    Gio._LocalFilePrototype.load_contents = function(cancel, ...args) {
        let contents = Gio.File.prototype.load_contents.call(this, cancel, ...args);
        if (contents[1] instanceof Uint8Array) {
            contents[1] = new ByteArray.ByteArray(contents[1]); // wrap it like this!
        }
        return contents;
    };
}

Otherwise the fix won't work if any spices attempt to use the contents as an actual ByteArray and you'll be passing a string instead which some might then rely on.

The only other usecase for Gio.File in an extension might be resources, which you could also patch.

@lestcape
Copy link

@lestcape lestcape commented Oct 17, 2020 •

@mtwebster Yes the first don't work as i mention. The second suggestion works, but this was a suggestion for gjs, to support the older code. It can be used in the Cinnamon case to not need to update the code, but the patch recommend by @ewlsh i think could be better if works (but it don't work for me).

@ptomato
Copy link
Contributor

@ptomato ptomato commented Oct 17, 2020

Be advised that the underscore in Gio._LocalFilePrototype means it's semi-internal / experimental and may change, especially if we fix it so that patching Gio.File.prototype works. I wouldn't recommend using it without a check for whether it is present.

@lestcape
Copy link

@lestcape lestcape commented Oct 18, 2020 •

@mtwebster I will add another possibility that Cinnamon have. This is port all the Cinnamon code related with Gio.File.prototype.load_contents to the last version of gjs, like this and then add support for the older versions using something like this two options:

const Gio = imports.gi.Gio;
const ByteArray = imports.byteArray;

if (!ByteArray.hasOwnProperty("toString")) {
    // We can assume that we have an 'UTF-8' encoding like was in the older gjs 
    ByteArray.toString = function(data, encoding) {
        return data.toString();
    };
}

if (Gio._LocalFilePrototype && Gio._LocalFilePrototype.load_contents) {
    Gio._real_local_load_contents = Gio._LocalFilePrototype.load_contents;
    Gio._LocalFilePrototype.load_contents = function(cancel, ...args) {       
        let contents = Gio._real_local_load_contents.call(this, cancel, ...args);
        // We can assume that this the older ByteArray implementation is not Uint8Array.
        if (!(contents[1] instanceof Uint8Array)) { 
            // Wrap the simple text in a ByteArray to be possible use later ByteArray.toString
            contents[1] = ByteArray.fromString(contents[1]);
        }
        return contents;
    };
}

let data = Gio.File.new_for_path("testCaseApp.js").load_contents(null)[1];
// The data is ByteArray or Uint8Array and ByteArray.toString exist in both cases.
// So, we can do now always:
print(ByteArray.toString(data));

In that way all the Cinnamon code belong to the last version of gjs and when you remove the monkey patch, you don' t need to change the code again.

Also (probably a better idea) you can monkey patch the ByteArray.toString and nothing more, to add support for all the possibilities you have in the older and new gjs versions. This should be something like this:

const Gio = imports.gi.Gio;
const ByteArray = imports.byteArray;

ByteArray._real_toString = (ByteArray.hasOwnProperty("toString")) ? ByteArray.toString : null;
ByteArray.toString = function(data, encoding) {
     if ((data instanceof Uint8Array) && ByteArray.hasOwnProperty("_real_toString")) {
        return ByteArray._real_toString(data);
     }
     if (data.hasOwnProperty("toString")) {
         return data.toString();
     }
     return data;
};

let data = Gio.File.new_for_path("testCaseApp.js").load_contents(null)[1];
// ByteArray.toString exist and it don't care if data is ByteArray or Uint8Array or also a simple text,
// it will return an string of it always.
print(ByteArray.toString(data));

Note: All that code is inspired in this gjs commit and are only tested in gjs 1.64.3

@clefebvre
Copy link
Member

@clefebvre clefebvre commented Oct 19, 2020

Mozjs78 support was merged today in CJS/Cinnamon. Both projects were bumped to unstable version 4.7.

We'll probably modify the deprecation warning in

cjs/cjs/deprecation.cpp

Lines 53 to 67 in ddff379

const char* messages[] = {
// None:
"(invalid message)",
// ByteArrayInstanceToString:
"Some code called array.toString() on a Uint8Array instance. Previously "
"this would have interpreted the bytes of the array as a string, but that "
"is nonstandard. In the future this will return the bytes as "
"comma-separated digits. For the time being, the old behavior has been "
"preserved, but please fix your code anyway to explicitly call ByteArray"
".toString(array).\n"
"(Note that array.toString() may have been called implicitly.)",
// DeprecatedGObjectProperty:
"Some code tried to set a deprecated GObject property.",
to link to https://projects.linuxmint.com/reference/git/cinnamon-tutorials/xlet-versioning.html and a commit example, to ensure spices smoothly fix the warning without breaking compatibility with Cinnamon 4.6 and older.

@mtwebster
Copy link
Member

@mtwebster mtwebster commented Nov 12, 2020

389f6fa

Closing this, as I think this issue has been sufficiently addressed.

@mtwebster mtwebster closed this Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.