Bug 1352650

I am going to fix it.

(In reply to Jun Aruga from comment #0)
> When I tried to do mock build, that was failed.
> Because depended rubygem-therubyracer was retired.
> We can fix this issue to use nodejs instead of the rubygem-therubyracer.

Yes, sure ... that was always the intention.

Hi, Vit

I have a question to fix this issue.

I found the rubygem-sprockets depends on rubygem-closure-compiler actually.
And it is commented out, because rubygem-closure-compiler had not been created 
yet at that time. but Now it has created.


But I got one problem to use it.
http://pkgs.fedoraproject.org/cgit/rpms/rubygem-sprockets.git/tree/rubygem-sprockets.spec
L68-72


So, I thought I wanted to use it now.
I got an error to build it with rubygem-closure-compiler.
Because the Autoload::Closure::COMPILER_VERSION is commented out in rubygem-closure-compiler. But sprockets is using the variable.

https://github.com/rails/sprockets/blob/v3.6.3/lib/sprockets/closure_compressor.rb#L39

So, which way do you like to fix this?
I have 3 ideas to fix this.

1. Modify lib/sprockets/closure_compressor.rb in rubygem-sprockets.spec to replace COMPILER_VERSION comment out logic, for example replace to COMPILER_VERSION = ''
2. Modify rubygem-closure-compiler.spec to add replacement logic to replace the logic in lib/sprockets/closure_compressor.rb .
3. Keep comment out of rubygem-sprockets.spec L68-72 logic. Do not load rubygem-closure-compiler.

> Because the Autoload::Closure::COMPILER_VERSION is commented out in rubygem-closure-compiler.

http://pkgs.fedoraproject.org/cgit/rpms/rubygem-closure-compiler.git/tree/rubygem-closure-compiler.spec L51

If I understand it right, then I'd opt for the 3rd option, i.e. keep the closure-compiler tests commented out ATM. If you could open new ticket with subject like "Enable closure-compiler test suite" for sprockets to address this later, that would be nice.

(In reply to Vít Ondruch from comment #5)
> If I understand it right, then I'd opt for the 3rd option, i.e. keep the
> closure-compiler tests commented out ATM. If you could open new ticket with
> subject like "Enable closure-compiler test suite" for sprockets to address
> this later, that would be nice.

Yes, I am going to proceed on 3rd option.
I have created new ticket for this issue.
https://bugzilla.redhat.com/show_bug.cgi?id=1353473

Created attachment 1177249 [details]
Fix a JavaScript runtime issue

Hi,

I fixed a JavaScript runtime issue. (rhbz#1352650)
I updated to Sprockets 3.6.3.

Also
- Dependency for rawhide, f24: ok.
- Install: ok
- Rpmlint: ok
- License check: ok

I am going to patch this to rawhide and f24.
Could you check my patch?
Thanks.

Sorry I made a mistake. f24 is no problem.
I am going to patch this to only rawhide.

(In reply to Jun Aruga from comment #7)
> Created attachment 1177249 [details]
> Fix a JavaScript runtime issue
> 
> Hi,
> 
> I fixed a JavaScript runtime issue. (rhbz#1352650)
> I updated to Sprockets 3.6.3.
> 
> Also
> - Dependency for rawhide, f24: ok.
> - Install: ok
> - Rpmlint: ok
> - License check: ok
> 
> I am going to patch this to rawhide and f24.
> Could you check my patch?
> Thanks.

(In reply to Jun Aruga from comment #7)
> Could you check my patch?

Looks quite OK to me, but I would use probably:

BuildRequires: /usr/bin/node

i.e. I would depend on the executable instead of the nodejs package name, because the executable is what the ExecJS requires [1] at the end.





[1] https://github.com/rails/execjs/blob/master/lib/execjs/runtimes.rb#L23

Hi, Vit
Thanks for the review.

Okay, I will use your way, and push it to master branch.
And thank for your adding ACL commits to me in PkgDB. I forgot to apply it.


(In reply to Vít Ondruch from comment #9)
> (In reply to Jun Aruga from comment #7)
> > Could you check my patch?
> 
> Looks quite OK to me, but I would use probably:
> 
> BuildRequires: /usr/bin/node
> 
> i.e. I would depend on the executable instead of the nodejs package name,
> because the executable is what the ExecJS requires [1] at the end.
> 
> 
> 
> 
> 
> [1] https://github.com/rails/execjs/blob/master/lib/execjs/runtimes.rb#L23

I am going to use 

BuildRequires: %{_bindir}/node

instead of

> BuildRequires: /usr/bin/node


It looks better.