Bug 1103414 - Review Request: js-jquery-migrate - APIs and features removed from jQuery core
Summary: Review Request: js-jquery-migrate - APIs and features removed from jQuery core
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dominik 'Rathann' Mierzejewski
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: js-jquery1
Blocks: js-reviews
TreeView+ depends on / blocked
 
Reported: 2014-05-31 08:42 UTC by T.C. Hollingsworth
Modified: 2015-06-20 02:14 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-06-20 02:09:04 UTC
dominik: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description T.C. Hollingsworth 2014-05-31 08:42:59 UTC
Spec: http://patches.fedorapeople.org/jquery/js-jquery-migrate.spec
SRPM: http://patches.fedorapeople.org/jquery/js-jquery-migrate-1.2.1-1.fc20.src.rpm
FAS:  patches
Description:
This plugin can be used to detect and restore APIs or features that have been
deprecated in jQuery and removed as of version 1.9.

The plugin can be included with versions of jQuery as old as 1.6.4 to identify
potential upgrade issues via its JQMIGRATE console warnings. However, the plugin
is only required for version 1.9.0 or higher to restore deprecated and removed
functionality.

Additionally, this package automatically maintains a copy of jQuery and
jQuery Migrate combined into a single file to permit an easier transition to
newer versions of jQuery.

Comment 1 T.C. Hollingsworth 2014-06-03 09:39:33 UTC
Fixed based on feedback from the js-sizzle review.

Spec: http://patches.fedorapeople.org/jquery/js-jquery-migrate.spec
SRPM: http://patches.fedorapeople.org/jquery/js-jquery-migrate-1.2.1-2.fc20.src.rpm

* Tue Jun 03 2014 T.C. Hollingsworth <tchollingsworth@gmail.com> - 1.2.1-2
- follow the github SourceURL guidelines

Comment 2 T.C. Hollingsworth 2014-10-21 18:53:31 UTC
Spec: https://patches.fedorapeople.org/jquery/js-jquery-migrate.spec
SRPM: https://patches.fedorapeople.org/jquery/js-jquery-migrate-1.2.1-3.fc20.src.rpm

* Tue Oct 21 2014 T.C. Hollingsworth <tchollingsworth@gmail.com> - 1.2.1-3
- typo and whitespace fixes

Comment 3 Dominik 'Rathann' Mierzejewski 2014-12-22 01:58:48 UTC
A quick fedora-review run reveals one issue:

Issues:
=======
- Package does not contain duplicates in %files.
  Note: warning: File listed twice: /usr/share/javascript/jquery-
  migrate/jquery+migrate.js
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

I think you need to specify %dir before:
%{_jsdir}/jquery-migrate
and list the files inside one by one:
%{_jsdir}/jquery-migrate/jquery-migrate.js
%{_jsdir}/jquery-migrate/jquery-migrate.min.js

Licence seems ok (MIT), but it's not specified in any of the source files (not a blocker).

mock build.log shows some warnings during %build:
Registering "grunt-git-authors" local Npm module tasks.
>> Local Npm module "grunt-git-authors" not found. Is it installed?

and similar for grunt-contrib-watch, grunt-contrib-jshint and grunt-contrib-qunit.

Are these anything to worry about?

I will try to do a more complete review during the Christmas break.

Comment 4 T.C. Hollingsworth 2014-12-29 06:55:17 UTC
(In reply to Dominik 'Rathann' Mierzejewski from comment #3)
> A quick fedora-review run reveals one issue:
> 
> Issues:
> =======
> - Package does not contain duplicates in %files.

Fixed.  I guess the %ghost files caused this.

I also fixed a wrong-end-of-line-encoding rpmlint warning I hadn't noticed before.

> Licence seems ok (MIT), but it's not specified in any of the source files
> (not a blocker).

Yeah that's unfortunately pretty common.  :-(

> mock build.log shows some warnings during %build:
> Registering "grunt-git-authors" local Npm module tasks.
> >> Local Npm module "grunt-git-authors" not found. Is it installed?
> 
> and similar for grunt-contrib-watch, grunt-contrib-jshint and
> grunt-contrib-qunit.
> 
> Are these anything to worry about?

No.  grunt-git-authors is only needed for releasing (changelog generation).  grunt-contrib-watch is for developers only: if you execute `grunt watch` it places inotify watches on the various projects and rebuilds everything if one changes.  grunt-contrib-qunit is the only thing we _should_ have, as it's needed for running the tests.  But, as the "q" in the name hints, it runs tests in a QtWebKit frame and thus won't work well in koji.  :-(

Note that grunt only displays these warnings because I request verbose output via `-v` in the build.  I guess I could turn that off to avoid confusing messages, but I do find it convenient to have the extra output always there on the offchance it is needed when a problem arises in some future update.

> I will try to do a more complete review during the Christmas break.

Thanks for the review so far!  I hope you and your family had a wonderful holiday.

--

Spec: https://patches.fedorapeople.org/jquery/js-jquery-migrate.spec
SRPM: https://patches.fedorapeople.org/jquery/js-jquery-migrate-1.2.1-4.fc20.src.rpm

* Mon Dec 29 2014 T.C. Hollingsworth <tchollingsworth@gmail.com> - 1.2.1-4
- correct duplicate in files list
- use correct end-of-line encoding in README.md file

Comment 5 Dominik 'Rathann' Mierzejewski 2015-01-11 23:09:23 UTC
Looks like you forgot to build js-jquery1 (bug 1078371), so this package is not yet installable. I rebuilt your js-jquery1 in a local mock and it seems fine.

Two more questions:

1. Why are you rebuilding the provided scripts upon every new jquery1 install instead of building them once at build time and adding strict version dependencies?

2. What are the missing dependencies required for %check?

[...]
# missing dependencies
#%%check
#grunt
[...]

Formally taking review.

Comment 6 Dominik 'Rathann' Mierzejewski 2015-01-27 13:05:33 UTC
Ping?

Comment 7 Dominik 'Rathann' Mierzejewski 2015-02-09 23:17:19 UTC
Ping2?

Comment 8 T.C. Hollingsworth 2015-02-19 10:25:25 UTC
(In reply to Dominik 'Rathann' Mierzejewski from comment #5)
> Looks like you forgot to build js-jquery1 (bug 1078371), so this package is
> not yet installable. I rebuilt your js-jquery1 in a local mock and it seems
> fine.

Sorry, I'll build it ASAP.  (Would have built it already, but dist-git seems to be down at the moment, even though I just used it an hour ago to update some existing packages...)

> Two more questions:
> 
> 1. Why are you rebuilding the provided scripts upon every new jquery1
> install instead of building them once at build time and adding strict
> version dependencies?

That would require rebuilding js-jquery-migrate on every js-jquery1 update.  That's easy enough for me, but a provenpackager who updates js-jquery1 might not be aware of this strict dependency, until it breaks updates-testing that is.  It also makes things more difficult for power users that might want to apply a patch to js-jquery1 or update it locally.  It also makes downgrades more difficult for system administrators.

By doing it this way, the concatenated copy always uses whatever version of js-jquery1 the system administrator wants to install with no hassle.

If this required an actual build process with nodejs as the main jquery-migrate.js file does, I wouldn't have done it this way, as the extra dependencies would be unacceptable and make the package unavailable on arches that nodejs does not run on.  But since we're just concatenating two text files with standard shell tools that are already installed, it seems nicer to defer it to install-time to allow end users and packagers both more flexibility.

> 2. What are the missing dependencies required for %check?
> 
> [...]
> # missing dependencies
> #%%check
> #grunt
> [...]

npm(grunt-contrib-qunit) and its dependencies.  This is now documented in the spec file.

> Formally taking review.

Thanks!

--

Spec: https://patches.fedorapeople.org/jquery/js-jquery-migrate.spec
SRPM: https://patches.fedorapeople.org/jquery/js-jquery-2.1.1-4.fc20.src.rpm

* Thu Feb 19 2015 T.C. Hollingsworth <tchollingsworth@gmail.com> - 1.2.1-5
- document the postinstall scriptlets
- indicate the missing dependencies for tests

Comment 9 Dominik 'Rathann' Mierzejewski 2015-05-15 10:30:17 UTC
Sorry it took so long. Here are some issues found upon review.

1. %license is not used for marking license text file
2. qunit is bundled
3. (SHOULD item) tests are not being run

Fixing 1 and 2 would be enough for this to be APPROVED, but it's be much appreciated if you took a stab at enabling the testsuite as well.

Full details below:

MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.

rpmlint is clean
built packages:
[rathann@sakura ~/build/mock/fedora-rawhide-x86_64/result]$ rpmlint js-jquery-migrate-1.2.1-5.fc23.*.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
installed package:
Spawning container 0ea55ffe3e794d5d9661f659a90d59e5 on /var/lib/mock/fedora-rawhide-x86_64/root.
Press ^] three times within 1s to kill container.
sh-4.3# rpmlint js-jquery-migrate
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

MUST: The package must meet the Packaging Guidelines .

While I don't like triggers and having files generated at install time, the actual method of using
this library recommended by upstream is including both jquery.js and jquery-migrate.js, so what
you're doing is simply a convenience for package consumers, so I guess this outweighs the issue
of not being able to easily verify the jquery+migrate{,.min}.js contents using rpm -V.

MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .

The source files don't have any license information, but the included LICENSE-MIT is sufficient.
-> Please ask upstream to include license information in the source files as well.

MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %license.

-> Please mark the LICENSE-MIT file with %license macro.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use sha256sum for this task as it is used by the sources file once imported into git. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.

$ spectool -g js-jquery-migrate.spec 
Getting https://github.com/jquery/jquery-migrate/archive/54ee1ae5928423a0e44ba1861112a746181fbc35/js-jquery-migrate-54ee1ae5928423a0e44ba1861112a746181fbc35.tar.gz to ./js-jquery-migrate-54ee1ae5928423a0e44ba1861112a746181fbc35.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   163    0   163    0     0     36      0 --:--:--  0:00:04 --:--:--    38
100 52329  100 52329    0     0   7064      0  0:00:07  0:00:07 --:--:-- 24407

[rathann@sakura ~/build/SOURCES/js-jquery-migrate]$ sha256sum js-jquery-migrate-54ee1ae5928423a0e44ba1861112a746181fbc35.tar.gz js-jquery-migrate-54ee1ae5928423a0e44ba1861112a746181fbc35.tar.gz.orig 
1b78a63145f11f2e946fb9dc36aa9b2a0d3e40aba558b89e24122f30b92e8625  js-jquery-migrate-54ee1ae5928423a0e44ba1861112a746181fbc35.tar.gz
1b78a63145f11f2e946fb9dc36aa9b2a0d3e40aba558b89e24122f30b92e8625  js-jquery-migrate-54ee1ae5928423a0e44ba1861112a746181fbc35.tar.gz.orig

MUST: Packages must NOT bundle copies of system libraries.

It's bundling qunit (https://qunitjs.com), even if it isn't used unless enable_tests is set to 1.
-> Please unbundle.

SHOULD: The reviewer should test that the package builds in mock.

Builds fine in fedora-rawhide-x86_64 mock.

SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.

You should try to run the testsuite at build time. You have the following comment in your spec:
# disabled due to missing dependencies (that likely won't run in koji anyway)
What is missing apart from qunit?

Comment 10 T.C. Hollingsworth 2015-05-16 22:38:56 UTC
(In reply to Dominik 'Rathann' Mierzejewski from comment #9) 
> While I don't like triggers and having files generated at install time, the
> actual method of using
> this library recommended by upstream is including both jquery.js and
> jquery-migrate.js, so what
> you're doing is simply a convenience for package consumers, so I guess this
> outweighs the issue
> of not being able to easily verify the jquery+migrate{,.min}.js contents
> using rpm -V.

I added a %verifyscript to address this concern.  Now if these generated files are not consistent with the already-verified RPM-installed files, rpm -V will complain:

----------
% rpm -Vv js-jquery-migrate 
.........    /usr/share/doc/js-jquery-migrate
.........  d /usr/share/doc/js-jquery-migrate/AUTHORS.txt
.........  d /usr/share/doc/js-jquery-migrate/CONTRIBUTING.md
.........  d /usr/share/doc/js-jquery-migrate/LICENSE-MIT
.........  d /usr/share/doc/js-jquery-migrate/README.md
.........    /usr/share/javascript/jquery-migrate
.........    /usr/share/javascript/jquery-migrate/jquery-migrate.js
.........    /usr/share/javascript/jquery-migrate/jquery-migrate.min.js
% sudo truncate --size=10 /usr/share/javascript/jquery-migrate/jquery+migrate.js   
% rpm -Vv js-jquery-migrate
.........    /usr/share/doc/js-jquery-migrate
.........  d /usr/share/doc/js-jquery-migrate/AUTHORS.txt
.........  d /usr/share/doc/js-jquery-migrate/CONTRIBUTING.md
.........  d /usr/share/doc/js-jquery-migrate/LICENSE-MIT
.........  d /usr/share/doc/js-jquery-migrate/README.md
.........    /usr/share/javascript/jquery-migrate
.........    /usr/share/javascript/jquery-migrate/jquery-migrate.js
.........    /usr/share/javascript/jquery-migrate/jquery-migrate.min.js
The file /usr/share/javascript/jquery+migrate.js does not match the
installed versions of js-jquery1 and js-jquery-migrate.  Please reinstall
these packages to resolve this issue.
-----------
 
> MUST: If (and only if) the source package includes the text of the
> license(s) in its own file, then that file, containing the text of the
> license(s) for the package must be included in %license.
> 
> -> Please mark the LICENSE-MIT file with %license macro.

Fixed.
 
> MUST: Packages must NOT bundle copies of system libraries.
> 
> It's bundling qunit (https://qunitjs.com), even if it isn't used unless
> enable_tests is set to 1.
> -> Please unbundle.

Fixed.

> SHOULD: The reviewer should test that the package functions as described. A
> package should not segfault instead of running, for example.
> 
> You should try to run the testsuite at build time. You have the following
> comment in your spec:
> # disabled due to missing dependencies (that likely won't run in koji anyway)
> What is missing apart from qunit?

Its dependencies.  (Including what will be the fourth nodejs command line argument parsing library we've packaged.  Yeesh!)

--

Spec: https://patches.fedorapeople.org/jquery/js-jquery-migrate.spec
SRPM: https://patches.fedorapeople.org/jquery/js-jquery-migrate-1.2.1-6.fc20.src.rpm

* Sat May 16 2015 T.C. Hollingsworth <tchollingsworth@gmail.com> - 1.2.1-6
- use %%license where available
- remove bundled qunit
- add %%verifyscript for files generated post-install

Comment 11 Dominik 'Rathann' Mierzejewski 2015-05-17 15:22:57 UTC
Excellent! This package is APPROVED.

Comment 12 T.C. Hollingsworth 2015-05-18 21:05:43 UTC
New Package SCM Request
=======================
Package Name: js-jquery-migrate
Short Description: APIs and features removed from jQuery core
Upstream URL: https://github.com/jquery/jquery-migrate/
Owners: patches jamielinux
Branches: f22 f21 f20 epel7 el6 el5
InitialCC:

Comment 13 Gwyn Ciesla 2015-05-18 23:28:35 UTC
Git done (by process-git-requests).

Comment 14 Dominik 'Rathann' Mierzejewski 2015-06-19 11:50:09 UTC
T.C., it looks like this isn't built yet. Could you build the package and close this bug?

Comment 15 T.C. Hollingsworth 2015-06-20 02:09:04 UTC
Sorry for the delay.

http://koji.fedoraproject.org/koji/buildinfo?buildID=663845

Comment 16 Fedora Update System 2015-06-20 02:12:22 UTC
js-jquery-migrate-1.2.1-6.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/js-jquery-migrate-1.2.1-6.el7

Comment 17 Fedora Update System 2015-06-20 02:13:09 UTC
js-jquery-migrate-1.2.1-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/js-jquery-migrate-1.2.1-6.el6

Comment 18 Fedora Update System 2015-06-20 02:14:01 UTC
js-jquery-migrate-1.2.1-6.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/js-jquery-migrate-1.2.1-6.fc22

Comment 19 Fedora Update System 2015-06-20 02:14:59 UTC
js-jquery-migrate-1.2.1-6.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/js-jquery-migrate-1.2.1-6.fc21


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