Bug 1141878 - Review Request: nodejs-es6-transpiler - es6 -> es5
Summary: Review Request: nodejs-es6-transpiler - es6 -> es5
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Piotr Popieluch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1141876 1141877
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-09-15 16:02 UTC by Ralph Bean
Modified: 2015-09-02 15:20 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2015-09-02 15:20:20 UTC
Type: ---
Embargoed:
piotr1212: fedora-review-


Attachments (Terms of Use)

Comment 1 Piotr Popieluch 2014-11-29 18:48:35 UTC
some comments:

- update to latest upstream
- Requires can be omitted, they should be generated automatically from package.json
- cannot install on f21:
Error: nothing provides npm(es5-shim) < 3 needed by nodejs-es6-transpiler-0.7.8-1.fc21.noarch
- es6toes5 should be installed with executable bit in %install

Comment 2 Ralph Bean 2014-12-01 20:34:27 UTC
Hi Piotr:

Here's a new release:

- I neglected to package the latest upstream as it has new requirements that would also need to be packaged (es6-shim, for one).
- I commented out the Requires.
- I got around the es5-shim requirement with a nodejs_fixdep
- es6toes5 should have the executable bit set now.

Spec URL: http://threebean.org/rpm/SPECS/nodejs-es6-transpiler.spec
SRPM URL: http://threebean.org/rpm/SRPMS/nodejs-es6-transpiler-0.7.8-2.fc20.src.rpm

Comment 3 Piotr Popieluch 2014-12-07 12:43:33 UTC
Hi Ralph,

When I run the tests they fail, any idea why? All requirements for the tests are satisfied.

Comment 4 Ralph Bean 2014-12-10 16:25:33 UTC
No idea off hand.. :/

Comment 5 Piotr Popieluch 2014-12-14 15:56:48 UTC
It seems that there are bundled libraries in the package. See jshint_global and lib dirs. I think you should work with upstream to unbundle them or could they be marked as copylibs?

I am not sure how to handle this, will ask my sponsor (Parag) if he is willing to take a look.



Besides the bundled libs I found some other issues which are fixable, see:



When running the binary I get this error:

~  /usr/bin/es6toes5                                                
/usr/bin/env: node --harmony: No such file or directory

I can get around it by changing 
#!/usr/bin/env node --harmony
to
#!/usr/bin/node --harmony


After that it fails with:
Error: Cannot find module './es6-transpiler'

You can fix that by installing es6toes5 in the module directory and making a symlink in /usr/bin/


There are two directories not packaged which are needed for functioning:
- transpiler
- jshint_globals


Bundled libraries:
jshint_globals is a bundled library, I think this could be fixed by using https://www.npmjs.com/package/jshint-globals or could it be marked as a copylib?

lib dir:
I have checked two files in lib dir which were both bundled libraries.
- esprima_harmony.js 
- error.js is from lib/esprima_harmony is a bundled library



All files have windows line endings, rpmlint warns about:

nodejs-es6-transpiler.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/nodejs-es6-transpiler/README.md
nodejs-es6-transpiler.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/nodejs-es6-transpiler/LICENSE

Comment 6 Ralph Bean 2014-12-15 18:40:58 UTC
Without having looked more deeply at it yet, the approach I've used on other packages is to:

1) package the bundled dep
2) symlink the dep into %{nodejs_sitelib}/es6-transpiler/BUNDLED_LIB_NAME

Would that be acceptable to you, Piotr, Parag?

Comment 7 Piotr Popieluch 2014-12-15 20:22:47 UTC
The files in lib directory are not full packages but single files from other modules. I think the way they are included from source will have to be modified to make it work. Or could they be treated as copylibs? I think upstream should at least specify from where they are taken and add their license.

jshint_globals contains one file from jshint. jshint contains one file with an incompatible license (for good not for evil). Packaging jshint is currently not possible. I see that es6-transpiler is based on nodejs-defs which is approved by Parag in 1122219.

Parag, what do you think?

Comment 8 Parag AN(पराग) 2014-12-16 01:24:19 UTC
I am sorry but currently I can't have time and full speed internet to look here. Since last 3 days I am in the hospital to take care of my father. I can look here by next week.
Thanks.

Comment 9 Parag AN(पराग) 2014-12-24 09:39:18 UTC
For nodejs-defs, See
$ ls jshint_globals/
LICENSE.jshint  README  vars.js

shows license is same MIT 

Piotr,
   Can you provide me which files looks bundled along with their package names?
I tried to search esprima_harmony.js in Fedora but could not find which package already installing it.

Comment 10 Piotr Popieluch 2014-12-24 11:13:02 UTC
Haven't checked before if they are already in Fedora


lib/
error.js: is in Fedora nodejs-defs, MIT license
esprima_harmony.js: is from https://www.npmjs.com/package/esprima and has BSD license.
node_inject.js: unknown
regjsparser.js: is from https://www.npmjs.com/package/regjsparser, BSD license
scope.js: unknown
stats.js: nodejs-defs
tmpl.js: unknown

Comment 11 Parag AN(पराग) 2015-01-02 01:17:25 UTC
Okay then this need to be clarified by upstream if the sources are taken from other modules.

Comment 12 Parag AN(पराग) 2015-01-02 03:40:46 UTC
also, lets take advice from more experienced nodejs module maintainers. I have added them in this review. I have started work with nodejs modules since last few months only.

Comment 13 Ralph Bean 2015-01-05 17:08:37 UTC
Noting here that Parag filed a bug upstream asking about the bundling:  https://github.com/termi/es6-transpiler/issues/71  Thanks for that!

Comment 14 Piotr Popieluch 2015-08-30 19:10:28 UTC
Don't think we need this as the blocking package doesn't need this anymore. Upstream also seems dead. OK if I close/wontfix this one?

Comment 15 Ralph Bean 2015-09-02 15:20:20 UTC
Yeah, let's drop it.  Thanks Piotr!


Note You need to log in before you can comment on or make changes to this bug.