Bug 948345

The header files all have the execute bit set, is it something we should try to fix up in the downstream spec file?


> Source0:        http://ftp.mozilla.org/pub/mozilla.org/js/mozjs17.0.0.tar.gz

Should use the %{version} macro to make future updates easier.


> %package devel
> ...
> Requires: pkgconfig

rpmbuild generates the pkgconfig dep automatically for packages that have .pc files, no need for the manual requires line.


> %package devel
> ...
> Requires: ncurses-devel readline-devel

Does it really need ncurses/readline headers? I couldn't find any ncurses/readline includes in the js header files, at least.


> %prep
> ...
> (cd js/src && autoconf-2.13)

Not sure the autoconf is necessary, might be cleaner to just remove it.


> %clean
> rm -rf %{buildroot}

The clean section is no longer needed with recent rpmbuild.


> %files
> %defattr(-,root,root,-)
> ...
> %files devel
> %defattr(-,root,root,-)

rpmbuild sets a sane defattr itself nowadays, no need to specify it here.


> %doc js/src/README.html
Not sure the README.html is very interesting, just contains a link. Maybe add README instead? One file that's definitely needs adding is the LICENSE file.


> %{_bindir}/js17-config

This should be in -devel I guess, not in the main library package.

(In reply to comment #1)
>
> Does it really need ncurses/readline headers? I couldn't find any
> ncurses/readline includes in the js header files, at least.

Looks like it has an internal copy of editline =/

I will work on de-bundling this.

> Not sure the autoconf is necessary, might be cleaner to just remove it.

I tend to prefer always rerunning the autotools, since
1) It makes patching Makefile.am/configure.ac work
2) Updates config.{sub,guess}
3) It's a step along the road to building directly from git


Fixed everything else, thanks!  New version uploaded, but I am working on the debundling of libedit.

> > Does it really need ncurses/readline headers? I couldn't find any
> > ncurses/readline includes in the js header files, at least.

Just to clarify, I wasn't talking about BuildRequires. I was talking about the -devel package's runtime requires on the ncurses/readline headers. The dependency seemed to indicate that the ncurses/readline headers need to be present to use the installed js public headers. I was just objecting to that and saying that the public headers (in the -devel package) don't appear to include anything else besides glibc headers.


> Looks like it has an internal copy of editline =/
> 
> I will work on de-bundling this.

Cool!

Is the internal editline only used for building the command line "js17" interpreter?

From the spec file:
> # Not clear to me we should ship this at all really,
> # but the real problem is it is statically linked
> # which bloats it significantly.
> rm -f %{buildroot}%{_bindir}/js17

I guess statically linking with internal editline can account for some of the bloat.

If you want to avoid shipping the js17 executable in any case, then it doesn't really matter for Fedora if the tarball has an internal editline or not. If we remove the binary, we aren't shipping the statically linked internal copy anyway.

(In reply to comment #3)
> > > Does it really need ncurses/readline headers? I couldn't find any
> > > ncurses/readline includes in the js header files, at least.
> 
> Just to clarify, I wasn't talking about BuildRequires. I was talking about
> the -devel package's runtime requires on the ncurses/readline headers. The
> dependency seemed to indicate that the ncurses/readline headers need to be
> present to use the installed js public headers. I was just objecting to that
> and saying that the public headers (in the -devel package) don't appear to
> include anything else besides glibc headers.

Ah, yes; that looks right.

> > Looks like it has an internal copy of editline =/
> > 
> > I will work on de-bundling this.
> 
> Cool!

This was easier than I thought, there's a --enable-readline we weren't passing.

> If you want to avoid shipping the js17 executable in any case, then it
> doesn't really matter for Fedora if the tarball has an internal editline or
> not. If we remove the binary, we aren't shipping the statically linked
> internal copy anyway.

I decided to ship it for now for testing purposes; the static linking issue is annoying but I need to learn their funky build system in order to fix it.

One other note, I just saw it bundles libffi, but it's not actually built becuase we're not enabling ctypes.  It's now deleted in %prep too.

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5219049

rpmlint output:
$ rpmlint mozjs17 \
          mozjs17-devel  \
          mozjs17-debuginfo-17.0.0-1.x86_64.rpm \
          mozjs17-17.0.0-1.src.rpm

mozjs17.x86_64: W: spelling-error %description -l en_US superset -> super set, super-set, supersede
mozjs17.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmozjs-17.0.so /lib64/libplds4.so
mozjs17.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmozjs-17.0.so /lib64/libplc4.so
mozjs17.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmozjs-17.0.so /lib64/libdl.so.2
mozjs17.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmozjs-17.0.so /lib64/libgcc_s.so.1
mozjs17.x86_64: W: no-manual-page-for-binary js17
mozjs17-devel.x86_64: W: no-documentation
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jscpucfg.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/HashFunctions.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsproxy.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/RangedPtr.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/WeakPtr.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/js/LegacyIntTypes.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/js/Utility.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/RefPtr.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/CheckedInt.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/js/TemplateLib.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/TypeTraits.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsgc.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsdhash.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsversion.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/ThreadLocal.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/LinkedList.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsperf.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/lib64/pkgconfig/mozjs-17.0.pc
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsdbgapi.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/Util.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/gc/Barrier.h
mozjs17-devel.x86_64: E: script-without-shebang /usr/bin/js17-config
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/MSStdInt.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/js/MemoryMetrics.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/BloomFilter.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/GuardObjects.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jswrapper.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsatom.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jspubtd.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jstypes.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/Assertions.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsprvtd.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/js/HashTable.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/NullPtr.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsclist.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/gc/StoreBuffer.h
mozjs17-devel.x86_64: E: script-without-shebang /usr/include/js-17.0/js.msg
mozjs17-devel.x86_64: E: script-without-shebang /usr/include/js-17.0/jsproto.tbl
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsprf.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/ds/BitArray.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jslock.h
mozjs17-devel.x86_64: E: script-without-shebang /usr/include/js-17.0/jsatom.tbl
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsutil.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/Constants.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsclass.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/gc/Statistics.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/json.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/MathAlgorithms.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/SHA1.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/FloatingPoint.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsalloc.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/Attributes.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/js/RequiredDefines.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsval.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/Scoped.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsfriendapi.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/StandardInteger.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/jsapi.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/js-config.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/gc/Root.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/Likely.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/js/Vector.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/gc/Heap.h
mozjs17-devel.x86_64: W: spurious-executable-perm /usr/include/js-17.0/mozilla/Types.h
mozjs17-devel.x86_64: W: no-manual-page-for-binary js17-config
mozjs17.src: W: spelling-error %description -l en_US superset -> super set, super-set, supersede
mozjs17.src:52: W: macro-in-comment %{buildroot}
mozjs17.src:52: W: macro-in-comment %{_bindir}
4 packages and 0 specfiles checked; 4 errors, 71 warnings.

Rpmlint is pretty unhappy, but it's mostly warnings about wrongly set executable bits, plus 2 other issues:

1) E: script-without-shebang /usr/bin/js17-config

Probably worth fixing, although I would hope most packages use the .pc file instead.


2) W: macro-in-comment %{buildroot}
   W: macro-in-comment %{_bindir}

Harmless warnings in this case, buildroot and bindir don't expand into multiline macros.

Do you know if upstream is planning any new releases where we could get them to fix up the permissions + the shebang? If not, I guess we have to do a spec file hack instead.

(In reply to comment #6)
> Rpmlint is pretty unhappy, but it's mostly warnings about wrongly set
> executable bits, plus 2 other issues:
> 
> 1) E: script-without-shebang /usr/bin/js17-config
> 
> Probably worth fixing, although I would hope most packages use the .pc file
> instead.

So it turns out this is run through preprocessor.py, and there's no option to make it not eat comments =/  And no one should be using this anyways, so: nuked.

> 2) W: macro-in-comment %{buildroot}
>    W: macro-in-comment %{_bindir}
> 
> Harmless warnings in this case, buildroot and bindir don't expand into
> multiline macros.
> 
> Do you know if upstream is planning any new releases where we could get them
> to fix up the permissions + the shebang? If not, I guess we have to do a
> spec file hack instead.

Fixed it up here for now, but I'll file a bug.

New versions uploaded (and thanks for the review!).  I should have run rpmlint myself, sorry for not doing so first.

Sorry for question but how it related to js package which we already have version 1.8.5?

(In reply to comment #9)
> Sorry for question but how it related to js package which we already have
> version 1.8.5?

It's a new parallel-installable upstream stable standalone release.

I can't find info about inheritance. Is it different product and what differences? Or one just may be then dropped? I'm js maintainer. May be it just need to be updated without bring new name (and you may became (co)maintainer)? Or we have some sort of incompatibilities?

(In reply to comment #11)
> Or one just may be then dropped?

http://lists.fedoraproject.org/pipermail/devel/2013-April/181131.html

> It's a new parallel-installable upstream stable standalone release.

I agree, it makes a lot of sense to make it parallel installable. Whether it's called mozjs17, or js17, or js and the old js-1.8.5 gets renamed to something else, anything works. Just as long as the two versions are parallel installable during the transitional period.

No comments on the -devel list so I assume everyone is fine with the new naming.


Two more issues:

1) I've just noticed that /usr/bin/js17 is still 3.9M, even after linking with external readline. Would it make sense to split it to another subpackage, or do we need it in the default install for some reason? Does GNOME Shell need it?

'repoquery -q --whatrequires /usr/bin/js /bin/js' returns nothing, so it doesn't seem like anything uses it as an interpreter for script files, at least.

Maybe something like:

%files
%{_bindir}/js17

%files libs
%doc LICENSE README
%{_libdir}/*.so

%files devel
%{_libdir}/pkgconfig/*.pc
%{_includedir}/js-17.0/


2) It's missing the dist macro:

Release: 1%{?dist}

And the review checklist for mozjs17-17.0.0-1.src.rpm:

+ OK
! needs attention

rpmlint output:
$ rpmlint mozjs17 \
          mozjs17-devel  \
          mozjs17-debuginfo-17.0.0-1.x86_64.rpm \
          mozjs17-17.0.0-1.src.rpm

mozjs17.x86_64: W: spelling-error %description -l en_US superset -> super set, super-set, supersede
mozjs17.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmozjs-17.0.so /lib64/libplds4.so
mozjs17.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmozjs-17.0.so /lib64/libplc4.so
mozjs17.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmozjs-17.0.so /lib64/libdl.so.2
mozjs17.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmozjs-17.0.so /lib64/libgcc_s.so.1
mozjs17.x86_64: W: no-manual-page-for-binary js17
mozjs17-devel.x86_64: W: no-documentation
mozjs17.src: W: spelling-error %description -l en_US superset -> super set, super-set, supersede
mozjs17.src:55: W: macro-in-comment %{buildroot}
mozjs17.src:55: W: macro-in-comment %{_bindir}
4 packages and 0 specfiles checked; 0 errors, 10 warnings.

+ rpmlint warnings are harmless and can be ignored
+ The package is named according to Fedora packaging guidelines
+ The spec file name matches the base package name.
+ The package meets the Packaging Guidelines
+ The package is licensed with a Fedora approved license and meets the
  Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ The package contains the license file (LICENSE)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
  20b6f8f1140ef6e47daa3b16965c9202  mozjs17.0.0.tar.gz
  20b6f8f1140ef6e47daa3b16965c9202  Download/mozjs17.0.0.tar.gz
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane 
n/a The spec file handles locales properly
+ ldconfig in %post and %postun
+ Package does not bundle copies of system libraries
n/a Package isn't relocatable
+ Package owns all the directories it creates
+ No duplicate files in %files
+ Permissions are properly set
+ Consistent use of macros 
+ The package must contain code or permissible content
n/a Large documentation files should go in -doc subpackage
+ Files marked %doc should not affect package
+ Header files should be in -devel
n/a Static libraries should be in -static 
n/a Library files that end in .so must go in a -devel package
+ -devel must require the fully versioned base
+ Packages should not contain libtool .la files
n/a Proper .desktop file handling
+ Doesn't own files or directories already owned by other packages
+ Filenames are valid UTF-8

Looks good!

Please take a look at comment #13 to fix the dist tag before importing and possibly split the -libs subpackage out. (I'll be happy to sanity check the subpackage split again if you want me to, but we can move forward with the git repo creation here I think.)

APPROVED

Colin, thanks. I missed it.
Just naming small confusing me (initially I understand it as 1.7 just turned in name).

(In reply to comment #13)
> 
> 1) I've just noticed that /usr/bin/js17 is still 3.9M, even after linking
> with external readline. Would it make sense to split it to another
> subpackage, or do we need it in the default install for some reason? Does
> GNOME Shell need it?

I kept it to test the package, but yeah, deleted now.  

> 2) It's missing the dist macro:
> 
> Release: 1%{?dist}

Fixed, thanks!

New Package SCM Request
=======================
Package Name: mozjs17
Short Description: JavaScript interpreter and libraries 
Owners: walters
Branches: f19
InitialCC:

Git done (by process-git-requests).

Hey, did you forget to commit the two fixes from comment #16 to git?

(In reply to comment #19)
> Hey, did you forget to commit the two fixes from comment #16 to git?

Somehow apparently, yes; probinson fixed one, I just pushed the other.