JSMin minifier is non-free (suggest JSqueeze or JShrink as alternatives)

[AdamWill]

As explained at #11430 (comment) , the JSMin minifier which is used by that PR is non-free according to FSF, Debian, and Fedora. Also, its original upstream developer abandoned it years ago and explicitly recommends other alternatives. mrclay is maintaining the copy in his repo, but probably not as actively as the newer alternatives.

I ran some benchmarks which indicated JSqueeze might be the best replacement, at least from a numbers perspective. It's also actively maintained by a well-known developer. Its lack of namespacing is a bit of a pain for distros but in the worst case (if they don't take my suggestion to namespace) can be dealt with.

More importantly, though, I managed to get a hacky build of master up and running and tested it with JSqueeze, and it, er, doesn't work (for me, anyway, confirmation would be welcome). Upstream seems fairly responsive to bug reports, so I'm hoping they'll get to that one soon.

The other alternative that looked decent to me was JShrink. It's currently-maintained, namespaced, travis-ified, psr-0 compliant, lots of buzzword boxes. It doesn't perform as well as JSqueeze in terms of speed or file size, but it does work with OC for basic use (I just tested), which is a significant advantage.

I'll note an ancillary benefit, here: pulling in mrclay/minify just to get a JavaScript minifier is kind of atomic-weapon-nutcracker stuff. As my blog post (linked earlier) points out, it's a sort of poor man's Assetic with eight different CSS/JS minification implementations and a whole bunch of other nutty stuff in it. Switching to JShrink or JSqueeze would allow us to just have a JavaScript minifier, not a JavaScript minifier tied to three other JavaScript minifiers and lord knows what else. As it stands, OC 8 will be shipping (by my count) four CSS minifiers (the three in mrclay/minify, plus the one it actually uses), two full PHP JavaScript minifiers, and two different Closure Compiler wrappers/interfaces (and that's not even counting any others lying around in the source tree...)

I can send PRs for switching to either JSqueeze or JShrink pretty easily - the changes to OC itself are trivial, then it's just a 3rdparty commit that drops mrclay/minify and pulls in the one we want - but it seemed prudent to wait for a response from JSqueeze to my bug report.

Edit: oh, for the record, a drawback of JShrink is that Assetic doesn't currently ship a filter for it. There's a github project which adds one - for my test I just dropped that straight into the Assetic tree and it worked fine, so it could be sent upstream pretty easily if they'd take it. But they do have a JSqueeze filter already, so it's one less step there.

[DeepDiver1975]

We don't use JSMin - but https://github.com/mrclay/minify - see https://github.com/owncloud/3rdparty/tree/master/mrclay/minify

[DeepDiver1975]

JSMin was used long time back until OC5 or 6

[AdamWill]

Yes, yes you do. Follow the breadcrumbs:

https://github.com/owncloud/core/blob/master/lib/private/templatelayout.php#L8 has use Assetic\Filter\JSMinFilter;

https://github.com/kriswallsmith/assetic/blob/master/src/Assetic/Filter/JSMinFilter.php does \JSMin::minify()

What do you get for \JSMin? Let's take a look at what Composer's giving ownCloud:

https://github.com/owncloud/3rdparty/blob/master/composer/autoload_classmap.php#L73 'JSMin' => $vendorDir . '/mrclay/minify/min/lib/JSMin.php',

So what's /mrclay/minify/min/lib/JSMin.php ? Well, let's take a look (edited extract of the comment block):

/**
* JSMin.php - modified PHP implementation of Douglas Crockford's JSMin.
* Copyright (c) 2002 Douglas Crockford (www.crockford.com)
* The Software shall be used for Good, not Evil.

Yes, you really are using JSMin...:) mrclay has continued to update the copy in his tree since Ryan Grove abandoned the original repo, but it is very definitely the same code with minor fixes.

[DeepDiver1975]

I see - THX - and there is the license issue:
https://github.com/mrclay/minify/blob/master/min/lib/JSMin.php#L36

[DeepDiver1975]

Okay - we better go for jsqueeze then - https://packagist.org/packages/patchwork/jsqueeze

@karlitschek We shall fix this for OC8 - will take care about this asap. Agreed? THX

[AdamWill]

if time's tight and we can't get jsqueeze or jshrink in shape in time, JSMinPlus from mrclay/minify should be license-safe (despite the name it's an independent implementation) and the patch would be, like, 8 characters, but personally I'd really like to avoid pulling in mrclay at all if possible, it's a messy thing :/ and JSMinPlus seems to be unmaintained these days.

[karlitschek]

fuck. Yes. One more challenge! Definitely important to fix.

[AdamWill]

Tested and confirmed fix with JSqueeze 2.0.1, but 2.0.1 also namespaces the lib (at my request, sorry :>) so Assetic's filter doesn't work OOTB any more. I've sent a PR to Assetic for that: kriswallsmith/assetic#698

[MorrisJobke]

IBM has a changed version of this:

IBM, its customers, partners, and minions, to use JSLint for evil.

http://dev.hasenj.org/post/3272592502/ibm-and-its-minions

;)

[MorrisJobke]

@AdamWill Thanks for spotting this!

[AdamWill]

@MorrisJobke that's still not really sufficient as the exception doesn't apply to all (unless you consider everyone a 'minion' of IBM...), so you still effectively can't distribute the software on free terms to all. Everyone who's not a 'customer, partner or minion' of IBM still can't use it for evil. In any case I don't think you could hold that it applies to the copy of JSMin.php in minify.

In any case, JSqueeze or JShrink or Matthias' 'minify' would be superior choices for several other reasons - they're currently maintained, follow good current coding practices, are faster and/or more efficient than JSMin, and don't come attached to the rest of the crazy stuff in mrclay/minify. Switching just seems like the sensible thing to do. I'm hoping upstream Assetic will pick up my PR soon, then it should be fine to use JSqueeze, I think (though if folks can help test it in various setups that'd obviously help).

[MorrisJobke]

@AdamWill Sure

[AdamWill]

assetic's now merged my filter change onto master, but not onto the 1.2 branch, yet - I've asked if they can merge it to 1.2.

[MorrisJobke]

@karlitschek @DeepDiver1975 @AdamWill As we are in feature freeze what if the upstream project can't provide the update fast enough? Is there an alternative plan?

[karlitschek]

@DeepDiver1975 What is your opinion?

[AdamWill]

@MorrisJobke I can see three options:

1. carry a downstream tweak of some kind to allow use of JSqueeze (backport the filter file patch to Assetic, or just dump an extra filter file in there, or patch the bug fix into JSqueeze 1.x, or something)
2. Use JShrink - current OC master works with current upstream JShrink fine, for me. This requires changing a couple of lines in lib/private/templatelayout.php, pulling JShrink into 3rdparty, and ideally dropping minify from 3rdparty (as it wouldn't be needed any more).
3. Use JSMinPlus, which is already in the current minify codebase - all this requires is a change of a couple of lines in lib/private/templatelayout.php. I don't know your rules/guidelines about non-free code, but in this case you might want/need to strip the non-free bits from the bundled minify (anything JSMin-derived).

[DeepDiver1975]

Okay - let's go for JSMinPlus for OC8 because it requires NO updates of 3rdparty libs - which reduces risk and test effort. For OC8.1 we can reevaluate (and maybe even allow some config option for installations who want to run e.g. YUI tools ???)

@AdamWill delivery of non-free code is not that of an issue of owncloud itself - but we want to collaborate with linux distributions as good as possible and respect the guidelines to not ship non-free code. For OC8 these code pieces have to be stripped. I hope this works out for you.

I'll take care about updating to JSMinPlus .....

[AdamWill]

shouldn't be a huge deal for downstreams - my approach will probably be either to package jsminplus on its own, bundle it with OC since it'll only be used temporarily, or just carry a patch to use jsqueeze instead.

[DeepDiver1975]

grrr - JSMinPlus generates broken JS - https://github.com/owncloud/core/tree/use-jsminplus

Uncaught ReferenceError: OC is not defined9bb5e912b2fd6c92104660fae0364d1b.js:217 (anonymous function)

[AdamWill]

bah :( I guess I didn't test it, I must've only tested JSqueeze and JShrink.

[AdamWill]

So to get something helpful done, I drafted up the necessary changes to use JShrink:

#13185
owncloud-archive/3rdparty#149

if it would help I can, as an alternative, draft up the changes to use JSqueeze (which would be along the same lines, but probably with an added Assetic filter called 'JSqueeze2Filter' or so in lib/private/assetic , along the lines of the recently-added SeparatorFilter).

[karlitschek]

@DeepDiver1975 What do you suggest?

[AdamWill]

@karlitschek there's been more activity on the PR I submitted to switch to JSqueeze: #13229 . At first we found several cases where the minified JS misbehaved, but most of them seem to be fixed now, and it's close to being mergeable, I think.

[PVince81]

Any update on this ? Or defer to 8.2 ?

[AdamWill]

sorry, I'm on vacation till Jun 8, I'll get back to it then.

[ghost]

@AdamWill any update? We need to make a quick determination to keep this in 8.2. Thanks!

[AdamWill]

This has been done, so far as I'm concerned, for months. It just never gets merged when I update it so every two fucking months I have to jump through the 're-generate the 3rdparty PR' hoop again. I'm sick of it.

#13229
owncloud-archive/3rdparty#150

the last time I jumped through the hoops was July 18, where @PVince81 asked some people to review it, and no-one really did (except for one comment on the 3rdparty PR which I explained is just what composer does when you use it as documented; if someone knows an alternative way to do it, i'm all ears).