https://bugzilla.redhat.com/show_bug.cgi?id=1670656 --- Comment #7 from Mark Goodwin <mgoodwin@xxxxxxxxxx> --- (In reply to Xavier Bachelot from comment #6) > Thanks Mark, the specfile definitely starts to look better. > Thanks Xavier, yes we're getting closer > Again, more comments : > - About the webpack tarball. How is it generated ? Can you add either an > URL, a comment or a script ? > The spec needs to make clear how to recreate it from scratch. > (Later comment after looking at the git repo, you do have the script already, the make_webpack.sh script is included in Patch0, and thus will eventually be part of grafana source at packaging/rpm/spec/make_webpack.sh > add it as a Source with a comment above) ok, have added it as Source2 in the spec, along with a comment. It's still in the patch, as mentioned. > Also, I'm not sure to which extend such bundling is allowed or not, but I'll > leave it alone for now and defer to people who know better. > I know from first hand experience that unbundling may consume a lot of time, > and keeping it that way is likely the best short term solution. Yes, it is still a bit debatable, though I'm confident this is the right approach. Using a pre-built webpack is how cockpit is (intending to) handle the nodejs dependencies problem, so this is how grafana can too. There are other packages heading this in the same way too, e.g. vector and others. Unfortunately, as with most web apps, all required nodejs deps are not available in Fedora .. and so tools such as npm, yarn and grunt need to be network-enabled so they can fetch packages from an npm registry. As you know, Fedora build policy does not allow this. Yarn can be told to use a pre-downloaded cache of node modules, but a webpack is better. Using a webpack is a reasonable compromise - it allows dependent node packages to be downloaded apriori, security checked by the maintainer, then bundled into the webpack and shipped with the app. The webpack is obfuscated and stamped with the build-id and then included as a Source tarball in the package. In a sense a webpack can be considered a version stamped private library of "compiled javascript" for use by the application. We thus get secure offline reproducible builds that yeild QE'able packages - this is clearly consistent with tried-and-tested, sane Fedora packaging principles. > Meanwhile, you should add versioned Provides: bundled(nodejs-blahblah) = > x.y.z for each of the bundled module. Since the bundled javascript is in a private webpack, is this still necessary? Only grafana will be using this webpack and so the grafana package does not "provide" any nodejs modules for any other package. The webpack is basically a shahash'ed private static library. > Same for go modules, indeed. > https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling The grafana golang code seems to be self contained, or only uses go libs that are already included by the BR for golang. > > - The -p when creating directories is useless, there's nothing to preserve. ok, removed those. > > - You don't need to create %{buildroot}/%{_unitdir}, it's already there > because of the BR: systemd. it seems to be needed, at least for local builds (where BUILDROOT may not have been populated with all BR directories outside of /). Official Fedora builds probably don't needed it. > > - The rundir needs to be handled by tmpfiles.d > https://docs.fedoraproject.org/en-US/packaging-guidelines/Tmpfiles.d/ this one is confusing - what's currently there seems to be working. > - About doc installation, you can make it simpler > Remove '''install -p -m 644 docs/VERSION *.md > %{buildroot}/%{_datadir}/doc/%{name}''' > The 'VERSION' file is useless. OK, removed > 'NOTICE.md' is not a license file, move it to %doc. ok done. > Modify the %files section to read: > %license LICENSE.md > %doc CHANGELOG.md CODE_OF_CONDUCT.md CONTRIBUTING.md NOTICE.md > %doc PLUGIN_DEV.md README.md ROADMAP.md UPGRADING_DEPENDENCIES.md ok done > - Still about doc, you should build the content of docs/ and ship it, > possibly in a separate sub-package if it is too big' yes I have considered a subpackage, but grafana.org themselves do not ship the docs. Instead they are available online and there are links in the grafana UI to browse the documentation. I also have added man pages for grafana-server(1) and grafana-cli(1) which mention the URLs to the online documentation. > - About %post, don't source the sysconfig file to get the grafana user, > group and home. > It doesn't make sense, as the sysconfig file shipped by the rpm have that > set already. ok agree > Anyway, the user/group creation belongs to %pre, not %post, as the > user/group is needed by rpm to set ownership of the dirs and files it > deploys. > And when in %pre, you don't have the sysconfig file at all. ok, added/updated %pre, %preun, %post, %postun as per docs. Please check the new spec. > Also add 'Requires(pre): shadow-utils' added > > - %preun/%postun are missing. > From > https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/ > #_scriptlets > %systemd_post grafana-server.service > ''' > %preun > %systemd_preun grafana-server.service > > %postun > %systemd_postun_with_restart grafana-server.service done. as above > - Add a blank line between changelog entries for legibility. done > > - Missing logrotate conf ? Or is grafana taking care of rotating its log on > its own ? Grafana already handles log rotation - see grafana.ini where the default parameters are: log_rotate = true # Max line number of single file, default is 1000000 max_lines = 1000000 # Max size shift of single file, default is 28 means 1 << 28, 256MB max_size_shift = 28 # Segment log daily, default is true daily_rotate = true # Expired days of log file(delete after max days), default is 7 max_days = 7 There are also config parameters for log level/verbosity, rsyslog, etc. New reference URLs after all of the above changes: Spec URL: https://raw.githubusercontent.com/goodwinos/grafana/native-rpm-spec/packaging/rpm/spec/grafana.spec SRPM URL: http://people.redhat.com/~mgoodwin/grafana/grafana-5.4.3-5.fc28.src.rpm COPR URL: https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana/ -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
