Bug 1267037 - Review Request: perl-Mojolicious-Plugin-Bootstrap3 - Mojolicious + http://getbootstrap.com/
Review Request: perl-Mojolicious-Plugin-Bootstrap3 - Mojolicious + http://get...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Petr Šabata
Fedora Extras Quality Assurance
:
Depends On: 1267036
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-28 18:00 EDT by Emmanuel Seyman
Modified: 2016-02-03 11:16 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-02-03 09:37:01 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
psabata: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Emmanuel Seyman 2015-09-28 18:00:16 EDT
Spec URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-Bootstrap3/perl-Mojolicious-Plugin-Bootstrap3.spec
SRPM URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-Bootstrap3/perl-Mojolicious-Plugin-Bootstrap3-3.3505-1.fc22.src.rpm
Description:
Mojolicious::Plugin::Bootstrap3 is used to include http://getbootstrap.com/
CSS and JavaScript files into your project.

Fedora Account System Username: eseyman
Comment 1 Petr Šabata 2015-10-02 07:22:14 EDT
* The package fails to build in mock -- see the next bullet.

* Define NO_PACKLIST or delete the .packlist files manually.
  You probably wanted to use the former but simply forgot to put there.

* Missing a builddep called in the spec file: make

* Missing a builddep used during the %check phase:
  - perl(Mojolicious::Plugin), lib/Mojolicious/Plugin/Bootstrap3.pm:233

* Mojolicious::Lite doesn't appear to be used anywhere; it's only
  mentioned in POD.

* According to META.*, the required version of Mojolicious::Plugin::AssetPack
  is 0.67, not 0.58.

* Mojolicious::Plugin::AssetPack won't be autodetected.
  Require it explicitly, including the correct version constraint.

* Even though it's not used directly, consider adding a runtime dependency
  on Mojolicious 6.00+.

* The minimum required version of Test::More is 0.88.  You could reflect
  that in your package as well.

* I understand it's the whole point of this package but this ships bundled
  fonts, JavaScript (jQuery and many others) and Sass content.
  That doesn't seem right.  The package should probably be patched to
  use the data provided by Fedora packages or acquire proper exceptions.
Comment 2 Adam Williamson 2015-10-08 15:20:29 EDT
Well, the bundling policy just got completely revised:

https://fedorahosted.org/fpc/ticket/575

even under the old rules, though, JavaScript bundling was explicitly allowed and CSS bundling was allowed in practice. There are still specific requirements for fonts, however:

https://fedoraproject.org/wiki/Packaging:Guidelines#Avoid_bundling_of_fonts_in_other_packages
Comment 3 Adam Williamson 2016-01-15 19:40:10 EST
Ping, anything happening here? This is the last dep I need to unblock openQA for the official repos...
Comment 4 Emmanuel Seyman 2016-01-31 06:52:58 EST
Okay, I've updated the package to 3.3600 (the current version), fixed all the issues that Petr pointed out and indicated that bootstrap and jquery are bundled.

Adam, if you have any improvements to suggest on the bundling part, I'm all ears.

Spec URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-Bootstrap3/perl-Mojolicious-Plugin-Bootstrap3.spec
SRPM URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-Bootstrap3/perl-Mojolicious-Plugin-Bootstrap3-3.3600-1.fc22.src.rpm
Comment 5 Adam Williamson 2016-02-01 03:35:57 EST
That looks fine to me, so far as I understand the new policy and the discussions around it. 'Unbundling' this package might have interesting consequences, though - things that use plugin-bootstrap but don't have write access to the 'bundled' files would have to pin their dependencies on the packages which actually provided the files, I think.

(the SUSE guys packaged openQA such that the assets are generated during package build, and the app cannot write to the asset dir; the benefit is reducing the amount of stuff the app needs write access to, but the drawback is the package must be rebuilt any time bootstrap3 changes. I'm still deciding whether to follow this for the Fedora package).
Comment 6 Petr Šabata 2016-02-01 07:57:17 EST
The SRPM link doesn't work, assuming it's actually http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-Bootstrap3/perl-Mojolicious-Plugin-Bootstrap3-3.3600-1.fc23.src.rpm

All points besides bundling addressed.  Since I understand the current bundling policy as `anything goes' and I'm not sure how this package is supposed to be used, I'm willing to approve this and leave it to you.

However, I think the License tag should include the content license as well.  So in this case it should be `Artistic 2.0 and MIT'.
Comment 7 Emmanuel Seyman 2016-02-01 09:58:19 EST
(In reply to Petr Šabata from comment #6)
> The SRPM link doesn't work, assuming it's actually
> http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-Bootstrap3/
> perl-Mojolicious-Plugin-Bootstrap3-3.3600-1.fc23.src.rpm

This is what happens when you do packaging work during a FOSDEM talk.

> However, I think the License tag should include the content license as well.
> So in this case it should be `Artistic 2.0 and MIT'.

Indeed.

Spec URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-Bootstrap3/perl-Mojolicious-Plugin-Bootstrap3.spec
SRPM URL: http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-Bootstrap3/perl-Mojolicious-Plugin-Bootstrap3-3.3600-2.fc23.src.rpm
Comment 8 Petr Šabata 2016-02-01 10:10:38 EST
(In reply to Emmanuel Seyman from comment #7)
> (In reply to Petr Šabata from comment #6)
> > The SRPM link doesn't work, assuming it's actually
> > http://people.parinux.org/~seyman/fedora/perl-Mojolicious-Plugin-Bootstrap3/
> > perl-Mojolicious-Plugin-Bootstrap3-3.3600-1.fc23.src.rpm
> 
> This is what happens when you do packaging work during a FOSDEM talk.

:)
 
> > However, I think the License tag should include the content license as well.
> > So in this case it should be `Artistic 2.0 and MIT'.
> 
> Indeed.

Okay, I'll approve this now.
Comment 9 Gwyn Ciesla 2016-02-01 13:04:55 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/perl-Mojolicious-Plugin-Bootstrap3
Comment 10 Emmanuel Seyman 2016-02-03 09:37:01 EST
Folks, this is in Rawhide and an update has been issued for F23.

Petr, thank you for the review. You went above and beyond the call of duty on this one.
Comment 11 Adam Williamson 2016-02-03 11:16:04 EST
That's awesome! Now openqa is unblocked :) Thank you very much for all your help with getting the deps packaged, Emmanuel!

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