Bug 1267037 - Review Request: perl-Mojolicious-Plugin-Bootstrap3 - Mojolicious + http://getbootstrap.com/
Summary: Review Request: perl-Mojolicious-Plugin-Bootstrap3 - Mojolicious + http://get...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Petr Šabata
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1267036
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-09-28 22:00 UTC by Emmanuel Seyman
Modified: 2016-02-03 16:16 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-02-03 14:37:01 UTC
Type: ---
psabata: fedora-review+


Attachments (Terms of Use)

Description Emmanuel Seyman 2015-09-28 22:00:16 UTC
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 11:22:14 UTC
* 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 19:20:29 UTC
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-16 00:40:10 UTC
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 11:52:58 UTC
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 08:35:57 UTC
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 12:57:17 UTC
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 14:58:19 UTC
(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 15:10:38 UTC
(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 18:04:55 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/perl-Mojolicious-Plugin-Bootstrap3

Comment 10 Emmanuel Seyman 2016-02-03 14:37:01 UTC
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 16:16:04 UTC
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.