Spec URL: https://github.com/goodwinos/grafana/blob/rpm-spec-build/packaging/rpm/spec/grafana.spec SRPM URL: http://people.redhat.com/~mgoodwin/grafana/grafana-5.4.3-2.fc28.src.rpm COPR URL: https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana/ Description: Grafana is an open source, feature rich metrics dashboard and graph editor. This packaging request is to build and package Grafana in Fedora. A native RPM spec is provided that builds Grafana packages on all supported architectures based on current upstream stable sources (v5.4.3 tagged and included as Source0 in the SRPM). A script is provided to create a webpack from the dependent node modules and this is included as Source1 in the SRPM. See https://github.com/goodwinos/grafana/blob/rpm-spec-build/packaging/rpm/spec/make_webpack.sh. The webpack allows offline multiarch reproducible builds, as per Fedora packaging guidelines. A small patch is also provided for FHS compliance, e.g. the Grafana config database is moved to /var, the log is moved to /var/log and a few other tweaks. A pull-request for the upstream Grafana project has NOT yet been submitted for the patch. Fedora Account System Username: mgoodwin
Just some preliminary comments. Spec URL shouild point at the raw file. Summary don't need to duplicate the package name. Source0 should be a full URL. https://github.com/grafana/grafana/archive/v%{version}/%{name}-%{version}.tar.gz # Fedora/FSH and config patch Typo s/FSH/FHS/ also elsewhere in the spec. %defattr(-,root,root,-) Not needed, this is the default. %post could probably use some systemd macros to be made simpler. https://fedoraproject.org/wiki/Packaging:Scriptlets#Scriptlets Generally speaking, %post and %posttrans looks quite complicated and doing too much. Maybe that could be made simpler ? Some config files are no marked %config(noreplace).
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-3.fc28.src.rpm COPR URL: https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana/ > Just some preliminary comments. > thanks. Note this is now in https://github.com/goodwinos/grafana native-rpm-spec (note: native-rpm-spec branch, which is based on the upstream v5.4.3 tag) > Spec URL shouild point at the raw file. > fixed, see new URLs listed above > Summary don't need to duplicate the package name. > fixed > Source0 should be a full URL. > https://github.com/grafana/grafana/archive/v%{version}/%{name}-%{version}.tar.gz fixed > > # Fedora/FSH and config patch > Typo s/FSH/FHS/ also elsewhere in the spec. fixed > > %defattr(-,root,root,-) > Not needed, this is the default. removed > > %post could probably use some systemd macros to be made simpler. > https://fedoraproject.org/wiki/Packaging:Scriptlets#Scriptlets > > Generally speaking, %post and %posttrans looks quite complicated and doing too much. Maybe that could be made simpler ? > these scriptlets are actually taken directly from grafana source See packaging/rpm/control/{postinst,posttrans}. So I'd need to discuss upstream before changing them. > Some config files are no marked %config(noreplace). > fixed Also: - patched various shebangs and file modes that rpmbuild was complaining about. - updated the patch and spec to preserve current defaults.ini and to add distro-defaults.ini - the spec now installs one or the other in /etc/grafana/grafana.ini depending whether it's a fedora/rhel build or some other build. So the existing defaults remain unchanged for grafana.com builds. This will make the patch more palatable upstream. - since the webpack contains nodejs javascript, added: ExclusiveArch: %{nodejs_arches} as per https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_exclusivearch
Thanks for the update Mark. Here's a couple more comments: - Seems Summary is not actually fixed: Summary: Grafana is an open source, feature rich metrics dashboard and graph editor s/Grafana is an open source, feature rich m/M/ - When installing files, use install -p to preserve timestamps. - Some of the %attr are useless in %files. All of them that force the owner/group to root/root are redundant with the default %defattr and those that also force the perms are also redundant with the install calls. Remove the %attr in the following lines: %attr(0755, root, root) %{_sbindir}/%{name}-server %attr(0755, root, root) %{_sbindir}/%{name}-cli %config(noreplace) %attr(-, root, root) %{_sysconfdir}/sysconfig/grafana-server %config(noreplace) %attr(-, root, root) %{_unitdir}/grafana-server.service %attr(-, root, root) %doc %{_datadir}/doc/%{name}/*.md %attr(-, root, root) %doc %{_datadir}/doc/%{name}/VERSION - The systemd unit file should not be %config(noreplace). Sorry if I did make change you that the other way with my earlier comment. https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/#_packaging - Rather than use an %exclude in %files, rm the unneeded docs dir in %install %exclude %{_datadir}/%{name}/docs - %files is missing a %license. The license file might be installed elsewhere, I haven't looked, but it should be marked as such. (In reply to Mark Goodwin from comment #2) > > > > %post could probably use some systemd macros to be made simpler. > > https://fedoraproject.org/wiki/Packaging:Scriptlets#Scriptlets > > > > Generally speaking, %post and %posttrans looks quite complicated and doing too much. Maybe that could be made simpler ? > > > > these scriptlets are actually taken directly from grafana source > See packaging/rpm/control/{postinst,posttrans}. So I'd need to > discuss upstream before changing them. > I understand and if the Fedora review can benefit upstream package too, I'm sure none would complain :-) Anyway, let's start looking at what's in there. I think that's the part of the specfile that need the more work. %posttrans #!/bin/sh set -e echo "POSTTRANS: Running script" [ -f /etc/sysconfig/grafana-server ] && . /etc/sysconfig/grafana-server # copy config files if missing if [ ! -f /etc/grafana/grafana.ini ]; then echo "POSTTRANS: Config file not found" if [ -f /etc/grafana/grafana.ini.rpmsave ]; then echo "POSTTRANS: /etc/grafana/grafana.ini.rpmsave config file found." mv /etc/grafana/grafana.ini.rpmsave /etc/grafana/grafana.ini echo "POSTTRANS: /etc/grafana/grafana.ini restored" if [ -f /etc/grafana/ldap.toml.rpmsave ]; then echo "POSTTRANS: /etc/grafana/ldap.toml.rpmsave found" mv /etc/grafana/ldap.toml.rpmsave /etc/grafana/ldap.toml echo "POSTTRANS: /etc/grafana/ldap.toml restored" fi echo "POSTTRANS: Restoring config file permissions" chown -Rh root:$GRAFANA_GROUP /etc/grafana/* chmod 755 /etc/grafana find /etc/grafana -type f -exec chmod 640 {} ';' find /etc/grafana -type d -exec chmod 755 {} ';' fi chown $GRAFANA_USER /usr/share/grafana/data chmod 755 /usr/share/grafana/data fi It's not silent, and scriptlets are supposed to be. It checks for files that the rpm deploys and override them with saved config files, which voids the purpose of %config(noreplace). It changes owner/group and perms on files that already have been properly set in the rpm. I might be missing something, but it seems part useless and part dangerous to me. I won't detail %post for now, but it seems to suffer from the same symptoms(set owner/group/perms, install conf file that are/should be deployed by the rpm). Enabling services on rpm install is also probably wrong. Again, unless I'm missing something, the only parts that should be kept are the grafana user and group creation and the conditional restart of the service on upgrade. https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/ https://docs.fedoraproject.org/en-US/packaging-guidelines/DefaultServices/ https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_systemd
(In reply to Xavier Bachelot from comment #3) > Thanks for the update Mark. > Here's a couple more comments: thanks, much appreciated > > - Seems Summary is not actually fixed: > Summary: Grafana is an open source, feature rich metrics dashboard > and graph editor > s/Grafana is an open source, feature rich m/M/ oh, I thought you ment in this BZ summary (Doh!). I'll fix it in the spec too. > > - When installing files, use install -p to preserve timestamps. > > - Some of the %attr are useless in %files. All of them that force the > owner/group to root/root are redundant with the default %defattr and those > that also force the perms are also redundant with the install calls. ok > Remove the %attr in the following lines: > %attr(0755, root, root) %{_sbindir}/%{name}-server > %attr(0755, root, root) %{_sbindir}/%{name}-cli > %config(noreplace) %attr(-, root, root) > %{_sysconfdir}/sysconfig/grafana-server > %config(noreplace) %attr(-, root, root) %{_unitdir}/grafana-server.service > %attr(-, root, root) %doc %{_datadir}/doc/%{name}/*.md > %attr(-, root, root) %doc %{_datadir}/doc/%{name}/VERSION ok will do > > - The systemd unit file should not be %config(noreplace). Sorry if I did > make change you that the other way with my earlier comment. > https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/#_packaging will fix that up > > - Rather than use an %exclude in %files, rm the unneeded docs dir in %install > %exclude %{_datadir}/%{name}/docs ok. Might also consider a subpackage for grafana-docs. And will need a man page for grafana-server(1) and grafana-cli(1) > > - %files is missing a %license. The license file might be installed > elsewhere, I haven't looked, but it should be marked as such. ok will investigate that and add as appropriate > > (In reply to Mark Goodwin from comment #2) > > > > > > %post could probably use some systemd macros to be made simpler. > > > https://fedoraproject.org/wiki/Packaging:Scriptlets#Scriptlets > > > > > > Generally speaking, %post and %posttrans looks quite complicated and doing too much. Maybe that could be made simpler ? > > > > > > > these scriptlets are actually taken directly from grafana source > > See packaging/rpm/control/{postinst,posttrans}. So I'd need to > > discuss upstream before changing them. > > > I understand and if the Fedora review can benefit upstream package too, I'm > sure none would complain :-) really, all %post should need is: systemctl condrestart grafana-server (so it's restarted ONLY if it's already running). And the user/group creation. However, since current grafana.com RPMs have the config DB in /usr/share/grafana/data we might want to migrate that to it's new location in /var/lib/grafana/data or this could be left for a sysadmin to tackle. > > Anyway, let's start looking at what's in there. I think that's the part of > the specfile that need the more work. yep > > It's not silent, and scriptlets are supposed to be. > It checks for files that the rpm deploys and override them with saved config > files, which voids the purpose of %config(noreplace). yep agree - shouldn't be necessary with correct config(noreplace) > It changes owner/group and perms on files that already have been properly > set in the rpm. agree, shouldn't need to do that either > I might be missing something, but it seems part useless and part dangerous > to me. > > I won't detail %post for now, but it seems to suffer from the same > symptoms(set owner/group/perms, install conf file that are/should be > deployed by the rpm). > Enabling services on rpm install is also probably wrong. against fedora and rhel policy > Again, unless I'm missing something, the only parts that should be kept are > the grafana user and group creation and the conditional restart of the > service on upgrade. yep - I'll work through that and post a new update later today (I'm in Australia TZ) > https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/ > https://docs.fedoraproject.org/en-US/packaging-guidelines/DefaultServices/ > https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/ > #_systemd Cheers
All changes mentioned in Comment #4 are done, bumped to grafana-5.4.3-4. Note new GH branch 'fedora-spec-build'. The %post scriptlet has been reduced to the bare minimum, as discussed. Spec URL: https://raw.githubusercontent.com/goodwinos/grafana/fedora-spec-build/packaging/rpm/spec/grafana.spec SRPM URL: http://people.redhat.com/~mgoodwin/grafana/grafana-5.4.3-4.fc28.src.rpm COPR URL: https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana/
Thanks Mark, the specfile definitely starts to look better. 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, add it as a Source with a comment above) 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. Meanwhile, you should add versioned Provides: bundled(nodejs-blahblah) = x.y.z for each of the bundled module. Same for go modules, indeed. https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling - The -p when creating directories is useless, there's nothing to preserve. - You don't need to create %{buildroot}/%{_unitdir}, it's already there because of the BR: systemd. - The rundir needs to be handled by tmpfiles.d https://docs.fedoraproject.org/en-US/packaging-guidelines/Tmpfiles.d/ - 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. 'NOTICE.md' is not a license file, move it to %doc. 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 - Still about doc, you should build the content of docs/ and ship it, possibly in a separate sub-package if it is too big' - 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. 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. Also add 'Requires(pre): shadow-utils' - %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 ''' - Add a blank line between changelog entries for legibility. - Missing logrotate conf ? Or is grafana taking care of rotating its log on its own ?
(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/
Hi Mark, Thanks for the update. I've stripped all the points that don't need to be discussed anymore to concentrate on the ones that are still open. (In reply to Mark Goodwin from comment #7) > (In reply to Xavier Bachelot from comment #6) ...snip... > > 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 if no one packages them, it will not change. Chicken and egg :-) > 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. > I agree to the compromise, but I wouldn't be against a stronger wording in these guidelines. > > 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. > Yes, it is, for security audit purpose. Say, a security issue is spotted in nodejs-foo. A bug will be filed against nodejs-foo package and the security issue will be patched in an updated rpm, which will benefit all packages using it as a dep. For bundled deps, the Provides: bundled(nodejs-foo) allows to query the repository for such things and also notify maintainer of packages that bundle it. If there's no Provides: bundled(nodejs-foo), there's no way to accomplish this task. Again from first hand experience, relying on upstream to monitor and fix issue in the libs they are bundling is not a safe bet. > > 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. > Ok. It might still be a good idea to explicitly list all Requires: that are needed at runtime. For example, perl guidelines mandates that. > > - 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. > This is not the "supported" way to build. Mock is the way to go, even for local builds, this is much safer. Anyway, your call, this is not a blocker. > > > > - 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. > Yes, it is working, but is not the way mandated in the guidelines. ...snip... > > - 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. > Still makes sense to ship them, imho. I can easily imagine a setup where the grafana instance doesn't have access to the outside world. Maybe the URls also need to be patched to point at the local install ?
I've tried my first build of the rpm then ran rpmlint. - There are a lot of files which are shipped with exec bit set, triggering lots of spurious script-without-shebang and wrong-script-end-of-line-encoding errors. - I believe most if not all of /usr/share/grafana/scripts/build/ is not needed. Fixing these 2 items should shorten rpmlint output quite a lot. Other rpmlint stuff : grafana.x86_64: E: non-readable /etc/grafana/grafana.ini 640 grafana.x86_64: E: non-readable /etc/grafana/ldap.toml 640 In the spec, there is : install -p -m 644 conf/distro-defaults.ini %{buildroot}/%{_sysconfdir}/%{name}/grafana.ini install -p -m 644 conf/ldap.toml %{buildroot}/%{_sysconfdir}/%{name}/ldap.toml but later %config(noreplace) %attr(0640, root, %{GRAFANA_GROUP}) %{_sysconfdir}/%{name}/grafana.ini %config(noreplace) %attr(0640, root, %{GRAFANA_GROUP}) %{_sysconfdir}/%{name}/ldap.toml So, 640 or 644 ? Choose one and make the spec consistent. 640 should be choosen if there is any security sensitive information in the file, 644 if not. Also, if the perms on a file or dir are already correct you don't need to specify it again in %attr and can use '-' instead. eg.: %attr(-,root,grafana) Now, parsing the specfile again. %define GRAFANA_USER %{name} %define GRAFANA_GROUP %{name} %define GRAFANA_HOME %{_datadir}/%{name} --> use %global # license and other package docs, but not APP docs (they're online) install -p -m 644 LICENSE.md %{buildroot}/%{_defaultlicensedir}/%name install -p -m 644 README.md ROADMAP.md CHANGELOG.md PLUGIN_DEV.md %{buildroot}/%{_docdir}/%{name} install -p -m 644 CODE_OF_CONDUCT.md CONTRIBUTING.md NOTICE.md %{buildroot}/%{_docdir}/%{name} then in %files : # other docs and license %doc %{_datadir}/doc/%{name} %license %{_defaultlicensedir}/%name/LICENSE.md --> Just use the following in %files, the %doc and %license macro take care of installing the files in the proper location : %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 %pre/%post/%postun don't need the '||:' safeguard, this is already taken care of by the %systemd_* macros Also, usually, the scriptlet sections are before the %files section. Also, about the run dir, I just checked again and /run is on tmpfs, hence the need for using tmpfiles.d. # other shared files, public html, webpack and scripts cp -rpa conf public scripts %{buildroot}/%{_datadir}/%{name} --> -a implies recursive and preserve so 'cp -a' should be enough. Source1: grafana_webpack-%{?version}.tar.gz --> spurious ? in macro, version is always set. %global __brp_ldconfig /bin/true # no libs --> what is the purpose of this line ? Sorry for the quite messy comment, it started clean, but then I started to raw dump any potential issue. I hope it makes sense and is useful anyway.
(In reply to Xavier Bachelot from comment #8 and comment #9) ... > > Unfortunately, as with most web apps, all required nodejs deps are not > > available in Fedora .. > > And if no one packages them, it will not change. Chicken and egg :-) true, however the are literally zillions of nodejs packages - the task of packaging them as RPMs and maintaini9ng them is impossible. A better approach may be to set up a moderated fedora npm registry and allow builds to access that. But for now it seems, webpack is the best we have. > > > Meanwhile, you should add versioned Provides: bundled(nodejs-blahblah) = > > > x.y.z for each of the bundled module. done. > > > 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. > > > Ok. It might still be a good idea to explicitly list all Requires: > that are needed at runtime. For example, perl guidelines mandates that. After install, 'ldd /usr/sbin/grafana-server' shows there are no go libraries required at run-time so likely no additional Requires are needed. > > > - 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. > > > This is not the "supported" way to build. Mock is the way to go, even for local builds, this is much safer. > Anyway, your call, this is not a blocker. ok yep - I should be building with mock, rather than just running rpmbuild directly. I am also building in copr (mgoodwin/grafana), which doesn't need %{buildroot}/%{_unitdir} created either. I added a comment. > > > - 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. > > > Yes, it is working, but is not the way mandated in the guidelines. OK done, now installing %{_tmpfiles}/grafana.conf for /run/grafana/grafana.pid So it will now be created again after a reboot. > > ...snip... > > > > - 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. > > > Still makes sense to ship them, imho. I can easily imagine a setup where the > grafana instance doesn't have access to the outside world. > Maybe the URls also need to be patched to point at the local install ? I'm starting to become out of time - perhaps we could add a -doc subpackage in a later update. > > I've tried my first build of the rpm then ran rpmlint. > - There are a lot of files which are shipped with exec bit set, triggering lots > of spurious script-without-shebang and wrong-script-end-of-line-encoding errors. I found the cause of that: installing with mode 0755 the entire public directory (so even the files were mode 755, clearly wrong). No fixed, and rpmlint is much happier - just a false positive on some svg files and a few other things. > - I believe most if not all of /usr/share/grafana/scripts/build/ is not needed. > Fixing these 2 items should shorten rpmlint output quite a lot. scripts are now omitted - they're not needed for the shipping package afaict. > > Other rpmlint stuff : > > grafana.x86_64: E: non-readable /etc/grafana/grafana.ini 640 > grafana.x86_64: E: non-readable /etc/grafana/ldap.toml 640 expected > > In the spec, there is : > install -p -m 644 conf/distro-defaults.ini %{buildroot}/%{_sysconfdir}/%{name}/grafana.ini > install -p -m 644 conf/ldap.toml %{buildroot}/%{_sysconfdir}/%{name}/ldap.toml > but later > %config(noreplace) %attr(0640, root, %{GRAFANA_GROUP}) %{_sysconfdir}/%{name}/grafana.ini > %config(noreplace) %attr(0640, root, %{GRAFANA_GROUP}) %{_sysconfdir}/%{name}/ldap.toml > > So, 640 or 644 ? Choose one and make the spec consistent. > 640 should be choosen if there is any security sensitive information in the file, 644 if not. > Also, if the perms on a file or dir are already correct you don't need to specify it again in %attr and can use '-' instead. > eg.: %attr(-,root,grafana) should be 640 root/grafana since the config and sysconf have a couple of default passwords and need to be readable by the grafana-server process, but only writeable by root. > Now, parsing the specfile again. > > %define GRAFANA_USER %{name} > %define GRAFANA_GROUP %{name} > %define GRAFANA_HOME %{_datadir}/%{name} > > --> use %global fixed > # license and other package docs, but not APP docs (they're online) > install -p -m 644 LICENSE.md %{buildroot}/%{_defaultlicensedir}/%name > install -p -m 644 README.md ROADMAP.md CHANGELOG.md PLUGIN_DEV.md %{buildroot}/%{_docdir}/%{name} > install -p -m 644 CODE_OF_CONDUCT.md CONTRIBUTING.md NOTICE.md %{buildroot}/%{_docdir}/%{name} > then in %files : > # other docs and license > %doc %{_datadir}/doc/%{name} > %license %{_defaultlicensedir}/%name/LICENSE.md > > --> Just use the following in %files, the %doc and %license macro take care of installing the files in the proper location : > %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 The %doc macro isn't finding those files correctly when they're named individually - it cd's to the wrong directory. So I've left this as-is for now, as follows %doc %{_datadir}/doc/%{name} %license %{_defaultlicensedir}/%name/LICENSE.md > > %pre/%post/%postun don't need the '||:' safeguard, this is already taken care of by the %systemd_* macros ok, removed > Also, usually, the scriptlet sections are before the %files section. > ok, moved scriptlets to before %files so now %files is just before changelog. > > Also, about the run dir, I just checked again and /run is on tmpfs, hence the need for using tmpfiles.d. > fixed now, as mentioned above. > # other shared files, public html, webpack and scripts > cp -rpa conf public scripts %{buildroot}/%{_datadir}/%{name} > --> -a implies recursive and preserve so 'cp -a' should be enough. fixed. > > Source1: grafana_webpack-%{?version}.tar.gz > --> spurious ? in macro, version is always set. fixed. > > > %global __brp_ldconfig /bin/true # no libs > --> what is the purpose of this line ? to avoid the following warning message when ldconfig is run by rpmbuild : /sbin/ldconfig: Warning: ignoring configuration file that cannot be opened: /home/mgoodwin/rpmbuild/BUILDROOT/grafana-5.4.3-6.fc28.x86_64/etc/ld.so.conf: No such file or directory I've removed it now since it seems like a bit of a hack, and not related to grafana per-se. > Sorry for the quite messy comment, it started clean, but then I started > to raw dump any potential issue. I hope it makes sense and is useful anyway. all makes sense, thanks for the on-going reviewing. Here's the rpmlint now: rpmlint /home/mgoodwin/rpmbuild/RPMS/x86_64/grafana-5.4.3-6.fc28.x86_64.rpm grafana.x86_64: W: non-standard-gid /etc/grafana/grafana.ini grafana grafana.x86_64: E: non-readable /etc/grafana/grafana.ini 640 grafana.x86_64: W: non-standard-gid /etc/grafana/ldap.toml grafana grafana.x86_64: E: non-readable /etc/grafana/ldap.toml 640 grafana.x86_64: E: non-readable /etc/sysconfig/grafana-server 640 grafana.x86_64: E: non-readable /usr/share/grafana/conf/defaults.ini 640 grafana.x86_64: E: script-without-shebang /usr/share/grafana/public/app/plugins/datasource/elasticsearch/img/elasticsearch.svg grafana.x86_64: E: script-without-shebang /usr/share/grafana/public/fonts/grafana-icons.svg grafana.x86_64: W: hidden-file-or-dir /usr/share/grafana/public/sass/.sass-lint.yml grafana.x86_64: W: hidden-file-or-dir /usr/share/grafana/public/test/.jshintrc grafana.x86_64: W: non-standard-uid /var/lib/grafana/data grafana grafana.x86_64: W: non-standard-gid /var/lib/grafana/data grafana grafana.x86_64: W: non-standard-uid /var/log/grafana grafana grafana.x86_64: W: non-standard-gid /var/log/grafana grafana grafana.x86_64: W: log-files-without-logrotate ['/var/log/grafana'] 1 packages and 0 specfiles checked; 6 errors, 9 warnings. 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-6.fc28.src.rpm COPR URL: https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana/ Note: after installing, prior to starting the service, you'll need to: sudo chgrp grafana /usr/share/grafana/conf/defaults.ini (that file is supposed to be readable by group grafana - I'll fix it in the morning) Cheers
(In reply to Mark Goodwin from comment #10) > > # license and other package docs, but not APP docs (they're online) > > install -p -m 644 LICENSE.md %{buildroot}/%{_defaultlicensedir}/%name > > install -p -m 644 README.md ROADMAP.md CHANGELOG.md PLUGIN_DEV.md %{buildroot}/%{_docdir}/%{name} > > install -p -m 644 CODE_OF_CONDUCT.md CONTRIBUTING.md NOTICE.md %{buildroot}/%{_docdir}/%{name} > > then in %files : > > # other docs and license > > %doc %{_datadir}/doc/%{name} > > %license %{_defaultlicensedir}/%name/LICENSE.md > > > > --> Just use the following in %files, the %doc and %license macro take care of installing the files in the proper location : > > %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 > > The %doc macro isn't finding those files correctly when they're named > individually > - it cd's to the wrong directory. So I've left this as-is for now, as > follows > > %doc %{_datadir}/doc/%{name} > %license %{_defaultlicensedir}/%name/LICENSE.md > Ok, got it. That comes from the way the %prep step is done. Look at the attached patched for a simplified approach.
Created attachment 1527626 [details] simplify %prep and thus %doc handling
Thanks for the patch Xavier 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-7.fc28.src.rpm COPR URL: https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana/ Note after applying the patch in Comment #12 I had to disable the debug packages to avoid a build error due to empty debugsourcefiles.list, with the following in the spec : %global debug_package %{nil} # avoid empty debugsourcefiles.list There are mentions of this issue in various forums, but no definite solution has emerged. Any ideas? Regards
Everything in the vendor directory is a bundled Go package that should be unbundled (many already are), or, less preferably, marked as such. Please also consider using the Go macros: https://fedoraproject.org/wiki/More_Go_packaging
Hi Elliot, thanks for the additional comments. I've added BuildRequires for the grafana/vendor golang sources that are available in Fedora, and these sources are removed prior to building. For the remainder, I've added Provides: bundled(foo). There is no unbundling for rhel <= 7 or fedora < 28 - see the new spec. Have also bumped to 5.4.3-8 and rebuilt the copr repo. 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-8.fc28.src.rpm COPR URL: https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana/ How's this looking now then? Thanks -- Mark
Hi Mark, Glad Elliott commented about bundled go, I started to look at it too because of the weird issue with the debug package. I think more go packages can be unbundled and you missed some of them because at least some of the go packages name in the never unbundled list are wrong. I will try to tidy up this list with fixed packages names. Hopefully, the list will become short enough the missing deps can be packaged. On a more personal note, go stuff is scary, somewhat statically linking everything seems a maintenance and security nightmare. Regards, Xavier
There's definitely something up with the bundled provides. A GitHub repo should be at something like https://github.com/<project>/<repo>, which should translate to golang-github-project-repo-devel, but a lot of the provides only seem to have the project name. Also, a condition like: %if 0%{?fedora} <= 27 || 0%{?rhel} <= 7 is incorrect and will always be true. If you're on Fedora, then the second condition will be `0 <= 7`, which is true, and vice versa if you're on RHEL.
Created attachment 1535917 [details] List of available and not available go packages After massaging the vendor dir a bit, I built the following list of packages. The check for available packages was done against Fedora 29.
You should check using the golang provides: golang(import path) > golang-github-gopherjs-gopherjs-devel Should be in golang-github-gopherjs-devel. > golang-github-Unknwon-com-devel Seems to have previously existed, but was retired for some reason. > golang-golangorg-oauth2-devel > golang-golangorg-sync-devel > golang-golangorg-sys-devel These should be available: golang-github-golang-sys-devel, golang-googlecode-goauth2-devel, golang-x-sync-devel. There are also several protobuf packages. I don't know if they're the same thing with a different/old import path, or something entirely different.
Created attachment 1536286 [details] List of available and not available go packages Thanks Elliott, I updated the file. Now Mark needs to make use of it. We're down to 30 unavailable go packages (and 55 available).
I've packaged github.com/hashicorp/go-version for an unrelated package in bug 1679057.
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-10.fc28.src.rpm COPR URL: https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana/ Comments - updated golang BuildRequires and Provides based on Xavier's list in Comment #20, updated the list of vendor sources removed during the build, and made a few additional tweaks. Bumped to 5.4.3-10 and ran new COPR builds. Tested on f28 and f29 (and also with the new PCP datasource currently under development). Here's the remaining list of bundled golang packages (in grafana vendor sources) : Provides: bundled(golang-github-benbjohnson-clock-devel) Provides: bundled(golang-github-codahale-hdrhistogram-devel) Provides: bundled(golang-github-facebookgo-inject-devel) Provides: bundled(golang-github-facebookgo-structtag-devel) Provides: bundled(golang-github-go-macaron-binding-devel) Provides: bundled(golang-github-go-macaron-gzip-devel) Provides: bundled(golang-github-go-macaron-session-devel) Provides: bundled(golang-github-gosimple-slug-devel) Provides: bundled(golang-github-go-stack-stack-devel) Provides: bundled(golang-github-go-xorm-builder-devel) Provides: bundled(golang-github-go-xorm-core-devel) Provides: bundled(golang-github-go-xorm-xorm-devel) Provides: bundled(golang-github-grafana-grafana-plugin-model-devel) Provides: bundled(golang-github-hashicorp-go-version-devel) Provides: bundled(golang-github-inconshreveable-log15-devel) Provides: bundled(golang-github-oklog-run-devel) Provides: bundled(golang-github-opentracing-opentracing-go-devel) Provides: bundled(golang-github-rainycape-unidecode-devel) Provides: bundled(golang-github-teris-io-shortid-devel) Provides: bundled(golang-github-uber-jaeger-client-go-devel) Provides: bundled(golang-github-uber-jaeger-lib-devel) Provides: bundled(golang-github-Unknwon-com-devel) Provides: bundled(golang-github-VividCortex-mysqlerr-devel) Provides: bundled(golang-github-yudai-gojsondiff-devel) Provides: bundled(golang-github-yudai-golcs-devel) Provides: bundled(golang-gopkg-alexcesaro-quotedprintable-3-devel) Provides: bundled(golang-gopkg-bufio-1-devel) Provides: bundled(golang-gopkg-macaron-1-devel) Provides: bundled(golang-gopkg-mail-2-devel) Provides: bundled(golang-gopkg-redis-2-devel)
Some of the unavailable go packages have review bugs filed already. It seems there was a previous attempt at packaging grafana that has not been completed. Maybe some of the below bugs can be revived: https://bugzilla.redhat.com/show_bug.cgi?id=1376719 https://bugzilla.redhat.com/show_bug.cgi?id=1377262 https://bugzilla.redhat.com/show_bug.cgi?id=1377227 https://bugzilla.redhat.com/show_bug.cgi?id=1679057 https://bugzilla.redhat.com/show_bug.cgi?id=1377229 https://bugzilla.redhat.com/show_bug.cgi?id=1376387 Other notes: - My understanding is the BuildRequires on go packages may need to be expressed as BR: golang(import_path) rather than BR: golang-forge-owner-repo-devel. - Does the bundled provides need to be expressed as P: bundled(golang(import_path)) ? - Similarly to ExclusiveArch: %{nodejs_arches}, there is an ExclusiveArch: %{go_arches}, I guess we want only arches matching both of them. I'm probably reaching the limit of the help I can provide with this package review, I'm not knowledgeable enough about go (although I learned some tricks along the way). Elliott, you seem to have more insight, would you be able to provide guidance ?
In addition to Xavier's notes and questions in Comment #23, rpmlint issues warnings that Provides: bundled(foo) should be versioned. Elliott, do you have any suggestions or answers here? # rpmlint RPMS/x86_64/grafana-5.4.3-10.fc28.x86_64.rpm grafana.x86_64: W: unstripped-binary-or-object /usr/sbin/grafana-cli grafana.x86_64: W: unstripped-binary-or-object /usr/sbin/grafana-server grafana.x86_64: W: non-standard-gid /etc/grafana/grafana.ini grafana grafana.x86_64: E: non-readable /etc/grafana/grafana.ini 640 grafana.x86_64: W: non-standard-gid /etc/grafana/ldap.toml grafana grafana.x86_64: E: non-readable /etc/grafana/ldap.toml 640 grafana.x86_64: E: non-readable /etc/sysconfig/grafana-server 640 grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/defaults.ini grafana grafana.x86_64: E: non-readable /usr/share/grafana/conf/defaults.ini 640 grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/distro-defaults.ini grafana grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/ldap.toml grafana grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning grafana grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/dashboards grafana grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/dashboards/sample.yaml grafana grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/datasources grafana grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/datasources/sample.yaml grafana grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/sample.ini grafana grafana.x86_64: E: script-without-shebang /usr/share/grafana/public/app/plugins/datasource/elasticsearch/img/elasticsearch.svg grafana.x86_64: E: script-without-shebang /usr/share/grafana/public/fonts/grafana-icons.svg grafana.x86_64: W: hidden-file-or-dir /usr/share/grafana/public/sass/.sass-lint.yml grafana.x86_64: W: hidden-file-or-dir /usr/share/grafana/public/test/.jshintrc grafana.x86_64: W: non-standard-uid /var/lib/grafana/data grafana grafana.x86_64: W: non-standard-gid /var/lib/grafana/data grafana grafana.x86_64: E: non-standard-dir-perm /var/lib/grafana/data 750 grafana.x86_64: W: non-standard-uid /var/lib/grafana/data/plugins grafana grafana.x86_64: W: non-standard-gid /var/lib/grafana/data/plugins grafana grafana.x86_64: E: non-standard-dir-perm /var/lib/grafana/data/plugins 750 grafana.x86_64: W: non-standard-uid /var/log/grafana grafana grafana.x86_64: W: non-standard-gid /var/log/grafana grafana grafana.x86_64: W: log-files-without-logrotate ['/var/log/grafana'] 1 packages and 0 specfiles checked; 8 errors, 22 warnings. # rpmlint SRPMS/grafana-5.4.3-10.fc28.src.rpm grafana.src:98: W: unversioned-explicit-provides bundled(golang-github-klauspost-compress-devel) grafana.src:99: W: unversioned-explicit-provides bundled(golang-gopkg-ini-1-devel) grafana.src:100: W: unversioned-explicit-provides bundled(golang-gopkg-square-jose-2-devel) grafana.src:105: W: unversioned-explicit-provides bundled(golang-github-aws-aws-sdk-go-devel) grafana.src:106: W: unversioned-explicit-provides bundled(golang-github-beorn7-perks-devel) grafana.src:107: W: unversioned-explicit-provides bundled(golang-github-bmizerany-assert-devel) grafana.src:108: W: unversioned-explicit-provides bundled(golang-github-bradfitz-gomemcache-devel) grafana.src:109: W: unversioned-explicit-provides bundled(golang-github-BurntSushi-toml-devel) grafana.src:110: W: unversioned-explicit-provides bundled(golang-github-codegangsta-cli-devel) grafana.src:111: W: unversioned-explicit-provides bundled(golang-github-davecgh-go-spew-devel) grafana.src:112: W: unversioned-explicit-provides bundled(golang-github-denisenkom-go-mssqldb-devel) grafana.src:113: W: unversioned-explicit-provides bundled(golang-github-fatih-color-devel) grafana.src:114: W: unversioned-explicit-provides bundled(golang-github-go-ini-ini-devel) grafana.src:115: W: unversioned-explicit-provides bundled(golang-github-golang-appengine-devel) grafana.src:116: W: unversioned-explicit-provides bundled(golang-github-golang-sys-devel) grafana.src:117: W: unversioned-explicit-provides bundled(golang-github-go-ldap-ldap-devel) grafana.src:118: W: unversioned-explicit-provides bundled(golang-github-go-macaron-inject-devel) grafana.src:119: W: unversioned-explicit-provides bundled(golang-github-google-go-genproto-devel) grafana.src:120: W: unversioned-explicit-provides bundled(golang-github-gopherjs-devel) grafana.src:121: W: unversioned-explicit-provides bundled(golang-github-gorilla-websocket-devel) grafana.src:122: W: unversioned-explicit-provides bundled(golang-github-go-sql-driver-mysql-devel) grafana.src:123: W: unversioned-explicit-provides bundled(golang-github-grpc-grpc-go-devel) grafana.src:124: W: unversioned-explicit-provides bundled(golang-github-hashicorp-go-hclog-devel) grafana.src:125: W: unversioned-explicit-provides bundled(golang-github-hashicorp-go-plugin-devel) grafana.src:126: W: unversioned-explicit-provides bundled(golang-github-hashicorp-yamux-devel) grafana.src:127: W: unversioned-explicit-provides bundled(golang-github-jmespath-go-jmespath-devel) grafana.src:128: W: unversioned-explicit-provides bundled(golang-github-jtolds-gls-devel) grafana.src:129: W: unversioned-explicit-provides bundled(golang-github-klauspost-compress-devel) grafana.src:130: W: unversioned-explicit-provides bundled(golang-github-klauspost-cpuid-devel) grafana.src:131: W: unversioned-explicit-provides bundled(golang-github-klauspost-crc32-devel) grafana.src:132: W: unversioned-explicit-provides bundled(golang-github-kr-pretty-devel) grafana.src:133: W: unversioned-explicit-provides bundled(golang-github-kr-text-devel) grafana.src:134: W: unversioned-explicit-provides bundled(golang-github-lib-pq-devel) grafana.src:135: W: unversioned-explicit-provides bundled(golang-github-mattn-go-colorable-devel) grafana.src:136: W: unversioned-explicit-provides bundled(golang-github-mattn-go-isatty-devel) grafana.src:137: W: unversioned-explicit-provides bundled(golang-github-mattn-go-sqlite3-devel) grafana.src:138: W: unversioned-explicit-provides bundled(golang-github-matttproud-golang_protobuf_extensions-devel) grafana.src:139: W: unversioned-explicit-provides bundled(golang-github-mitchellh-go-testing-interface-devel) grafana.src:140: W: unversioned-explicit-provides bundled(golang-github-patrickmn-go-cache-devel) grafana.src:141: W: unversioned-explicit-provides bundled(golang-github-pkg-errors-devel) grafana.src:142: W: unversioned-explicit-provides bundled(golang-github-prometheus-client_golang-devel) grafana.src:143: W: unversioned-explicit-provides bundled(golang-github-prometheus-client_model-devel) grafana.src:144: W: unversioned-explicit-provides bundled(golang-github-prometheus-common-devel) grafana.src:145: W: unversioned-explicit-provides bundled(golang-github-prometheus-procfs-devel) grafana.src:146: W: unversioned-explicit-provides bundled(golang-github-sergi-go-diff-devel) grafana.src:147: W: unversioned-explicit-provides bundled(golang-github-smartystreets-assertions-devel) grafana.src:148: W: unversioned-explicit-provides bundled(golang-github-smartystreets-goconvey-devel) grafana.src:149: W: unversioned-explicit-provides bundled(golang-golangorg-crypto-devel) grafana.src:150: W: unversioned-explicit-provides bundled(golang-golang-org-net-devel) grafana.src:151: W: unversioned-explicit-provides bundled(golang-golangorg-text-devel) grafana.src:152: W: unversioned-explicit-provides bundled(golang-googlecode-goauth2-devel) grafana.src:153: W: unversioned-explicit-provides bundled(golang-google-golangorg-cloud-devel) grafana.src:154: W: unversioned-explicit-provides bundled(golang-gopkg-ini-1-devel) grafana.src:155: W: unversioned-explicit-provides bundled(golang-gopkg-square-jose-2-devel) grafana.src:156: W: unversioned-explicit-provides bundled(golang-gopkg-yaml-devel) grafana.src:157: W: unversioned-explicit-provides bundled(golang-x-sync-devel) grafana.src:161: W: unversioned-explicit-provides bundled(golang-github-benbjohnson-clock-devel) grafana.src:162: W: unversioned-explicit-provides bundled(golang-github-codahale-hdrhistogram-devel) grafana.src:163: W: unversioned-explicit-provides bundled(golang-github-facebookgo-inject-devel) grafana.src:164: W: unversioned-explicit-provides bundled(golang-github-facebookgo-structtag-devel) grafana.src:165: W: unversioned-explicit-provides bundled(golang-github-go-macaron-binding-devel) grafana.src:166: W: unversioned-explicit-provides bundled(golang-github-go-macaron-gzip-devel) grafana.src:167: W: unversioned-explicit-provides bundled(golang-github-go-macaron-session-devel) grafana.src:168: W: unversioned-explicit-provides bundled(golang-github-gosimple-slug-devel) grafana.src:169: W: unversioned-explicit-provides bundled(golang-github-go-stack-stack-devel) grafana.src:170: W: unversioned-explicit-provides bundled(golang-github-go-xorm-builder-devel) grafana.src:171: W: unversioned-explicit-provides bundled(golang-github-go-xorm-core-devel) grafana.src:172: W: unversioned-explicit-provides bundled(golang-github-go-xorm-xorm-devel) grafana.src:173: W: unversioned-explicit-provides bundled(golang-github-grafana-grafana-plugin-model-devel) grafana.src:174: W: unversioned-explicit-provides bundled(golang-github-hashicorp-go-version-devel) grafana.src:175: W: unversioned-explicit-provides bundled(golang-github-inconshreveable-log15-devel) grafana.src:176: W: unversioned-explicit-provides bundled(golang-github-oklog-run-devel) grafana.src:177: W: unversioned-explicit-provides bundled(golang-github-opentracing-opentracing-go-devel) grafana.src:178: W: unversioned-explicit-provides bundled(golang-github-rainycape-unidecode-devel) grafana.src:179: W: unversioned-explicit-provides bundled(golang-github-teris-io-shortid-devel) grafana.src:180: W: unversioned-explicit-provides bundled(golang-github-uber-jaeger-client-go-devel) grafana.src:181: W: unversioned-explicit-provides bundled(golang-github-uber-jaeger-lib-devel) grafana.src:182: W: unversioned-explicit-provides bundled(golang-github-Unknwon-com-devel) grafana.src:183: W: unversioned-explicit-provides bundled(golang-github-VividCortex-mysqlerr-devel) grafana.src:184: W: unversioned-explicit-provides bundled(golang-github-yudai-gojsondiff-devel) grafana.src:185: W: unversioned-explicit-provides bundled(golang-github-yudai-golcs-devel) grafana.src:186: W: unversioned-explicit-provides bundled(golang-gopkg-alexcesaro-quotedprintable-3-devel) grafana.src:187: W: unversioned-explicit-provides bundled(golang-gopkg-bufio-1-devel) grafana.src:188: W: unversioned-explicit-provides bundled(golang-gopkg-macaron-1-devel) grafana.src:189: W: unversioned-explicit-provides bundled(golang-gopkg-mail-2-devel) grafana.src:190: W: unversioned-explicit-provides bundled(golang-gopkg-redis-2-devel) grafana.src:354: W: macro-in-comment %{_vendor} grafana.src:357: W: macro-in-comment %{_vendor} grafana.src:358: W: macro-in-comment %{_vendor} grafana.src:359: W: macro-in-comment %{_vendor} grafana.src:360: W: macro-in-comment %{_vendor} grafana.src:503: W: macro-in-%changelog %doc grafana.src: W: invalid-url Source1: grafana_webpack-5.4.3.tar.gz 1 packages and 0 specfiles checked; 0 errors, 93 warnings. Thanks
(In reply to Mark Goodwin from comment #24) > In addition to Xavier's notes and questions in Comment #23, rpmlint issues > warnings that Provides: bundled(foo) should be versioned. > > # rpmlint RPMS/x86_64/grafana-5.4.3-10.fc28.x86_64.rpm > grafana.x86_64: W: unstripped-binary-or-object /usr/sbin/grafana-cli > grafana.x86_64: W: unstripped-binary-or-object /usr/sbin/grafana-server This should be done somehow (automatically, I'd have thought), since debug information is in the -debuginfo subpackage. It seems like it might be because you've disabled debug packages; why is that? > grafana.x86_64: W: non-standard-gid /etc/grafana/grafana.ini grafana > grafana.x86_64: E: non-readable /etc/grafana/grafana.ini 640 > grafana.x86_64: W: non-standard-gid /etc/grafana/ldap.toml grafana > grafana.x86_64: E: non-readable /etc/grafana/ldap.toml 640 > grafana.x86_64: E: non-readable /etc/sysconfig/grafana-server 640 > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/defaults.ini grafana > grafana.x86_64: E: non-readable /usr/share/grafana/conf/defaults.ini 640 > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/distro-defaults.ini grafana > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/ldap.toml grafana > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning grafana > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/dashboards grafana > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/dashboards/sample.yaml grafana > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/datasources grafana > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/datasources/sample.yaml grafana > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/sample.ini grafana > grafana.x86_64: W: non-standard-uid /var/lib/grafana/data grafana > grafana.x86_64: W: non-standard-gid /var/lib/grafana/data grafana > grafana.x86_64: E: non-standard-dir-perm /var/lib/grafana/data 750 > grafana.x86_64: W: non-standard-uid /var/lib/grafana/data/plugins grafana > grafana.x86_64: W: non-standard-gid /var/lib/grafana/data/plugins grafana > grafana.x86_64: E: non-standard-dir-perm /var/lib/grafana/data/plugins 750 > grafana.x86_64: W: non-standard-uid /var/log/grafana grafana > grafana.x86_64: W: non-standard-gid /var/log/grafana grafana I assume you've followed the instructions on https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/ ? > grafana.x86_64: E: script-without-shebang /usr/share/grafana/public/app/plugins/datasource/elasticsearch/img/elasticsearch.svg > grafana.x86_64: E: script-without-shebang /usr/share/grafana/public/fonts/grafana-icons.svg These should not be executable. > grafana.x86_64: W: hidden-file-or-dir /usr/share/grafana/public/sass/.sass-lint.yml > grafana.x86_64: W: hidden-file-or-dir /usr/share/grafana/public/test/.jshintrc Delete these development files. > grafana.x86_64: W: log-files-without-logrotate ['/var/log/grafana'] See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_log_files > # rpmlint SRPMS/grafana-5.4.3-10.fc28.src.rpm > grafana.src:98: W: unversioned-explicit-provides ... It seems like you have bundled provides for things you've removed from the source. You don't need to add those (though yes, technically, it's statically bundled, the macros should do that sort of thing for you.) > grafana.src:503: W: macro-in-%changelog %doc Remove macro from changelog. Some other things: * No, I don't think you need to do `Provides: bundled(golang(github.com/repo/package))` instead of `Provides: bundled(golang-github-repo-package-devel)`. * From what I've read, nodejs macros will automatically add bundled provides for you; we should do this in the Go macros. * Fedora 27 is EOL; there's no need to support it. * golang(github.com/hashicorp/go-version) is available; you can unbundle it. * The %if 0%{?fedora} || 0%{?rhel} check seems redundant.
make_webpack.sh can be a simplified a little bit as you don't really need to use go to build it: https://qulogic.fedorapeople.org/make_webpack.sh Also, are you interested in maintaining golang-github-go-stack-stack (bug 1377227) and golang-github-Unknwon-com (bug 1376387), since you need them for grafana?
(In reply to Elliott Sales de Andrade from comment #25) > (In reply to Mark Goodwin from comment #24) > > In addition to Xavier's notes and questions in Comment #23, rpmlint issues > > warnings that Provides: bundled(foo) should be versioned. > > > > # rpmlint RPMS/x86_64/grafana-5.4.3-10.fc28.x86_64.rpm > > grafana.x86_64: W: unstripped-binary-or-object /usr/sbin/grafana-cli > > grafana.x86_64: W: unstripped-binary-or-object /usr/sbin/grafana-server > > This should be done somehow (automatically, I'd have thought), since debug > information is in the -debuginfo subpackage. It seems like it might be > because you've disabled debug packages; why is that? The build failed as follows : Empty %files file /home/mgoodwin/rpmbuild/BUILD/grafana-5.4.3/debugsourcefiles.list I've investigated some more and managed to get the debuginfo package building again (but omitting the debugsource package since it's empty) - it seems there are some issues with debuginfo for golang binaries. WHat I have now allows gdb backtraces, which is better than we had before. And the unstripped-binary-or-object complaints have gone away now (since we now have the *.debug versions too) > > grafana.x86_64: W: non-standard-gid /etc/grafana/grafana.ini grafana > > grafana.x86_64: E: non-readable /etc/grafana/grafana.ini 640 > > grafana.x86_64: W: non-standard-gid /etc/grafana/ldap.toml grafana > > grafana.x86_64: E: non-readable /etc/grafana/ldap.toml 640 > > grafana.x86_64: E: non-readable /etc/sysconfig/grafana-server 640 > > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/defaults.ini grafana > > grafana.x86_64: E: non-readable /usr/share/grafana/conf/defaults.ini 640 > > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/distro-defaults.ini grafana > > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/ldap.toml grafana > > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning grafana > > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/dashboards grafana > > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/dashboards/sample.yaml grafana > > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/datasources grafana > > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/provisioning/datasources/sample.yaml grafana > > grafana.x86_64: W: non-standard-gid /usr/share/grafana/conf/sample.ini grafana > > grafana.x86_64: W: non-standard-uid /var/lib/grafana/data grafana > > grafana.x86_64: W: non-standard-gid /var/lib/grafana/data grafana > > grafana.x86_64: E: non-standard-dir-perm /var/lib/grafana/data 750 > > grafana.x86_64: W: non-standard-uid /var/lib/grafana/data/plugins grafana > > grafana.x86_64: W: non-standard-gid /var/lib/grafana/data/plugins grafana > > grafana.x86_64: E: non-standard-dir-perm /var/lib/grafana/data/plugins 750 > > grafana.x86_64: W: non-standard-uid /var/log/grafana grafana > > grafana.x86_64: W: non-standard-gid /var/log/grafana grafana > > I assume you've followed the instructions on > https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/ ? yes I have followed that doc - using the 'dynamic allocation' method, as described in that document. It would seem rpmlint is complaining when it shouldn't. To resolve that, I would have to manually chmod/chgrp in the %post scriptlets. Install/testing the %pre stuff that's there now in the spec on systems with/without grafana user/group all seems to work correctly. > > grafana.x86_64: E: script-without-shebang /usr/share/grafana/public/app/plugins/datasource/elasticsearch/img/elasticsearch.svg > > grafana.x86_64: E: script-without-shebang /usr/share/grafana/public/fonts/grafana-icons.svg > > These should not be executable. Will add to Patch0 (000-grafana-fedora.patch). This is broken upstream but no big deal, svg files are not even scripts obviously. > > grafana.x86_64: W: hidden-file-or-dir /usr/share/grafana/public/sass/.sass-lint.yml > > grafana.x86_64: W: hidden-file-or-dir /usr/share/grafana/public/test/.jshintrc > > Delete these development files. now deleted via spec %build but might put that in the patch too since it's broken upstream. > > grafana.x86_64: W: log-files-without-logrotate ['/var/log/grafana'] > > See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_log_files Grafana has it's own log rotation and management, so we do not ship or rely on the logrotate config mechanism documented in the guidelines. > > # rpmlint SRPMS/grafana-5.4.3-10.fc28.src.rpm > > grafana.src:98: W: unversioned-explicit-provides ... > > It seems like you have bundled provides for things you've removed from the > source. You don't need to add those (though yes, technically, it's > statically bundled, the macros should do that sort of thing for you.) I need to audit some of the Provides - obviously all the stuff removed from the vendor source needs BuildRequires to replace it, and should not have Provides because it's no longer bundled ... if the macros can do that then even better, I only hve to get he BuildRequires stuff in shape and the rest is automatic. Is anyone actually working on those macros? They'd be similar to the nodejs stuff I guess. > > > grafana.src:503: W: macro-in-%changelog %doc > > Remove macro from changelog. done > > Some other things: > * No, I don't think you need to do `Provides: > bundled(golang(github.com/repo/package))` instead of `Provides: > bundled(golang-github-repo-package-devel)`. > * From what I've read, nodejs macros will automatically add bundled provides > for you; we should do this in the Go macros. yep, as above > * Fedora 27 is EOL; there's no need to support it. OK. when I started this, it wasn't ;) > * golang(github.com/hashicorp/go-version) is available; you can unbundle it. ok (and yes I saw your earlier update about that) > * The %if 0%{?fedora} || 0%{?rhel} check seems redundant. yes redundant for RH platforms, but not SuSE etc., if they were to use this spec too. > make_webpack.sh can be a simplified a little bit as you don't really need to use go to build it: > https://qulogic.fedorapeople.org/make_webpack.sh thanks - the go building in that script is left over from some earlier attempts at this > Also, are you interested in maintaining golang-github-go-stack-stack (bug 1377227) > and golang-github-Unknwon-com (bug 1376387), since you need them for grafana? I'm interested in maintaining Grafana since we need it for PCP (as do many other groups); if that also involves maintaiing some golang in order to get there, then so be it! Regarding go packaging, this site seems up-to-date, whereas apparently gofed is falling behind. https://fedoraproject.org/wiki/More_Go_packaging I'll post new spec URLs when ready Cheers and thanks for the feedback -- Mark
If you're on Fedora 28, then I think you should get gofed from updates-testing. It produces specs that match those templates in the wiki (though with one minor bug in not correctly setting goipath.)
Have updated to grafana-6.0.1-1 - this is now the current upstream stable version. For the Fedora spec build, have switched to a new branch 'grafana-native-spec-v6' (retaining the 'native-rpm-spec' branch for the 5.4.3 version). Have incorporated the review comments from Comment #25 Comment #26 and Comment #27. Spec URL: https://raw.githubusercontent.com/goodwinos/grafana/grafana-native-spec-v6/packaging/rpm/spec/grafana.spec SRPM URL: https://mgoodwin.fedorapeople.org/grafana/grafana-6.0.1-1.fc28.src.rpm COPR URL: https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana/ Additional notes: - golang vendor package unbundling efforts are being tracked in a spreadsheet in google docs. Reviewers have been invited to edit - let me know if you want to be added. - the fc28 build fails because grafana-v6 requires golang >= 1.11 Strangely, the el7 copr build is fine with this. - the f29 build is fine. - the rawhide build fails because golang-github-hashicorp-version seems to be missing from the f30 chroot (Elliott, maybe a build is needed - rawhide builds have been pretty unreliable lately) Thanks
Builds are there for all versions: https://koji.fedoraproject.org/koji/packageinfo?packageID=28148 If you're on copr, you probably need to wait for the Rawhide compose that finally finished to be synced out to mirrors. BTW, is there a way for the webpack to be unminified in the source tarball (and then combined in the rpm build)?
(In reply to Elliott Sales de Andrade from comment #30) > Builds are there for all versions: > https://koji.fedoraproject.org/koji/packageinfo?packageID=28148 > > If you're on copr, you probably need to wait for the Rawhide compose that > finally finished to be synced out to mirrors. it's there now and my grafana-6.01 build is now working for rawhide, thanks > > BTW, is there a way for the webpack to be unminified in the source tarball > (and then combined in the rpm build)? the fully populated node_modules directory built by make_webpack.sh (prior to minifying it into the webpack) is 443MB, which compresses down to 71MB. The resulting webpack is 30MB, compressing down to 5.6MB. So shipping node_modules in the Source tarball would result in a huge SRPM. In addition, I had great difficulty getting the grunt tasks that build the webpack to work without network access enabled in the build - against Fedora policy. So what you're asking is probably feasible (with a lot of effort), but not really practical. For the golang deps, I have a copr build of golang-github-go-stack-stack at https://copr.fedorainfracloud.org/coprs/mgoodwin/golang-github-go-stack-stack/ and this is now unbundled and being pulled in via BuildRequires for the grafana build at https://copr.fedorainfracloud.org/coprs/mgoodwin/grafana I'm going to continue with this process for the remaining golang vendor/bundled deps that are not yet in Fedora .. hopefully this will get somewhere :P Cheers
(In reply to Xavier Bachelot from comment #23) > - Similarly to ExclusiveArch: %{nodejs_arches}, there is an ExclusiveArch: > %{go_arches}, I guess we want only arches matching both of them. > From the command line, one can get the arches matching both nodejs_arches and go_arches with the following: comm -12 <(rpm -E %{go_arches} | tr ' ' '\n' | sort) <(rpm -E %{nodejs_arches} | tr ' ' '\n' | sort) | tr '\n' ' ' I don't know how to express that in the specfile, the subshells are getting in the way. Just adding this as a note and a reminder. Also, it's nice to see the go packages unbundling is progressing.
(In reply to Xavier Bachelot from comment #8) > (In reply to Mark Goodwin from comment #7) > > (In reply to Xavier Bachelot from comment #6) > > > > Unfortunately, as with most web apps, all required nodejs deps are not > > available in Fedora .. > > And if no one packages them, it will not change. Chicken and egg :-) > FWIW: I've orphaned a bunch of nodejs packages which I packaged for Grafana last week. If someone still intends to package them it might save some time to pick those up. https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/E33E4LI43GE5SPEAPQCCLKOX3C6NO2LG/
(In reply to Xavier Bachelot from comment #32) > (In reply to Xavier Bachelot from comment #23) > > > - Similarly to ExclusiveArch: %{nodejs_arches}, there is an ExclusiveArch: > > %{go_arches}, I guess we want only arches matching both of them. If we split 'grafana' into grafana and grafana-server, then each could have it's own ExclusiveArch, though grafana-server couldn't be a subpackage because the ExclusiveArch setting is global AIUI. However, such split is a big departure from upstream packaging. Alternatively, perhaps we could open an RFE for an rpm/lua macro or function that returns the space separated substring intersection of two (or more) strings, so we could find the intersection of %{nodejs_arches} with %{go_arches}. Maybe that already exists (?) > From the command line, one can get the arches matching both nodejs_arches > and go_arches with the following: > comm -12 <(rpm -E %{go_arches} | tr ' ' '\n' | sort) <(rpm -E > %{nodejs_arches} | tr ' ' '\n' | sort) | tr '\n' ' ' > I don't know how to express that in the specfile, the subshells are getting > in the way. > Just adding this as a note and a reminder. > > Also, it's nice to see the go packages unbundling is progressing. yes it's getting there, see BZs blocking this BZ. Out of about 30 vendor bundled packages that were not already in rawhide, we're down to the last few now. -- Regards
Here's a Lua script that might work: $ cat arch.lua -- Create a set (Lua table) of Go arches go_arches = {} for arch in rpm.expand("%{go_arches}"):gmatch("%S+") do go_arches[arch] = 1 end -- Check for Node.js arches in Go arches for arch in rpm.expand("%{nodejs_arches}"):gmatch("%S+") do if go_arches[arch] then print(arch .. " ") end end $ rpm --eval "%{lua: $(cat arch.lua)}" i386 i486 i586 i686 pentium3 pentium4 athlon geode x86_64 armv3l armv4b armv4l armv4tl armv5tl armv5tel armv5tejl armv6l armv6hl armv7l armv7hl armv7hnl aarch64 ppc64le s390x
Spec URL: https://mgoodwin.fedorapeople.org/grafana/grafana.spec SRPM URL: https://mgoodwin.fedorapeople.org/grafana/grafana-6.1.3-1.fc31.src.rpm koji rawhide scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=34106884 Additional notes: - update to latest upstream stable release v6.1.3, see CHANGELOG - unbundle all vendor sources, replace with BuildRequires, see the long list of blocker BZs linked to BZ#1670656 - BuildRequires go-plugin >= v1.0.0 for grpc_broker (thanks eclipseo) - tweak make_webpack to no longer use grunt, switch to prod build - add ExclusiveArch lua script (thanks quantum.analyst) - patch0 is for FHS compliance, man pages and minor rpmlint issues - patch1 removes jaeger-tracing - no golang support in Fedora rpmlint notes: log-files-without-logrotate is expected - grafana does it's own log rotation non-standard-gid and non-standard-uid - have used static user/group allocation, as documented in guidelines
The diff between the very first spec in this review and what it is now is really impressive, well done. More nitpicking, you're used to it now ;-) - Patch0 can probably be split in several pieces and part of them upstream'ed (manpages and file perms changes at least seem relevant) - Still about the patch, why change PLUGINS_DIR to /var/lib/grafana/data/plugins instead of /var/lib/grafana/plugins ? - For legibility, can the lua script be moved at the top of the spec and just the results be used in the ExclusiveArch tag ? - You can use %{?systemd_requires} in place of the explicit Requires(pre/post/postun): systemd - About Provides: bundled(golang-github-grafana-grafana-plugin-model-devel) = 0.0.1 if this is part of grafana, then this is not bundled. - There used to be Provides: bundled(..) for all the golang stuff in the bundle everything case (fedora <= 28 or EL <= 7). I think it should be put back in case you want to build for EL7 (I guess F28 doesn't really matter anymore). - Why "chmod 644 %{SOURCE2} # silence an rpmlint non-issue" in %install ? The file is already 644 in the srpm. Last point is I'm not sure how much I like the enormous pile of bundled nodejs stuff, but this is again an huge task and it seems to be somewhat accepted, so...
(In reply to Xavier Bachelot from comment #37) > The diff between the very first spec in this review and what it is now is > really impressive, well done. thanks once again Xavier > > More nitpicking, you're used to it now ;-) > - Patch0 can probably be split in several pieces and part of them > upstream'ed (manpages and file perms changes at least seem relevant) OK, I've split into 3 patches (see comments in the spec) > - Still about the patch, why change PLUGINS_DIR to > /var/lib/grafana/data/plugins instead of /var/lib/grafana/plugins ? fixed. This was actually leftover from grafana v5.x, where the /var shared data was installed below /usr/share/grafana. > - For legibility, can the lua script be moved at the top of the spec and > just the results be used in the ExclusiveArch tag ? done > - You can use %{?systemd_requires} in place of the explicit > Requires(pre/post/postun): systemd done > - About > Provides: bundled(golang-github-grafana-grafana-plugin-model-devel) = 0.0.1 > if this is part of grafana, then this is not bundled. agree, that makes sense. I've removed that "Provides: bundled(..) entirely now. > - There used to be Provides: bundled(..) for all the golang stuff in the > bundle everything case (fedora <= 28 or EL <= 7). > I think it should be put back in case you want to build for EL7 (I guess > F28 doesn't really matter anymore). Those Provides would only ever be for EPEL7. I can put it back if you really want, though for EPEL we'd only ever rebase to new upstream releases - and grafana upstream updates the vendor'd golang sources whenever needed. So it seems to me the spec pollution wasn't warranted and I removed it. > - Why "chmod 644 %{SOURCE2} # silence an rpmlint non-issue" in %install ? > The file is already 644 in the srpm. Removed (no longer needed) > Last point is I'm not sure how much I like the enormous pile of bundled > nodejs stuff, but this is again an huge task and it seems to be somewhat > accepted, so... yeah, agree. And yes webpack seems to be gaining precedent, e.g. see cockpit. (BTW, has anyone ever considered a "gopack" for bundled golang sources?) Spec URL: https://mgoodwin.fedorapeople.org/grafana/grafana.spec SRPM URL: https://mgoodwin.fedorapeople.org/grafana/grafana-6.1.3-1.fc31.src.rpm koji rawhide scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=34121032
(In reply to Mark Goodwin from comment #38) > (In reply to Xavier Bachelot from comment #37) > > The diff between the very first spec in this review and what it is now is > > really impressive, well done. > > thanks once again Xavier You and Nathan deserve the thanks, you did the work, I only commented ;-) > > > > > More nitpicking, you're used to it now ;-) > > - Patch0 can probably be split in several pieces and part of them > > upstream'ed (manpages and file perms changes at least seem relevant) > > OK, I've split into 3 patches (see comments in the spec) > 000-grafana-fedora.patch could be split again (Oauth / file perms). The mostly noop 3rd part could be removed. 001-grafana-fedora.patch could also be split (distro defaults / manpages / Specs + webpack). 002-grafana-fedora.patch looks ok. Also, the patches can probably use more descriptive names (eg. grafana-6.1.3-fix_file_perms.patch, grafana-6.1.3-fix_oauth.patch, etc...) Most of the patches (all but 002 ?) need to be submitted upstream and the PR/issue URLs referenced in the specfile. Upstream will likely want fine-grained commits, so the above patch split will be needed anyway. ...snip... > > - There used to be Provides: bundled(..) for all the golang stuff in the > > bundle everything case (fedora <= 28 or EL <= 7). > > I think it should be put back in case you want to build for EL7 (I guess > > F28 doesn't really matter anymore). > > Those Provides would only ever be for EPEL7. I can put it back if you really > want, though for EPEL we'd only ever rebase to new upstream releases - and > grafana upstream updates the vendor'd golang sources whenever needed. So it > seems to me the spec pollution wasn't warranted and I removed it. > You are making the assumption that both upstream developers and downstream fedora packagers will be properly doing their duty forever. Although this is probably unlikely, this might not be a safe assumption. I understand adding 100+ lines to the spec is not very appealing, but I tend to think we'd rather be on the safe side. I would welcome more opinions on this, maybe I'm too pedantic.
Hi Xavier, (In reply to Xavier Bachelot from comment #39) > > > More nitpicking, you're used to it now ;-) > > > - Patch0 can probably be split in several pieces and part of them > > > upstream'ed (manpages and file perms changes at least seem relevant) > > > > OK, I've split into 3 patches (see comments in the spec) > > > 000-grafana-fedora.patch could be split again (Oauth / file perms). > The mostly noop 3rd part could be removed. > > 001-grafana-fedora.patch could also be split (distro defaults / manpages / > Specs + webpack). > > 002-grafana-fedora.patch looks ok. > > Also, the patches can probably use more descriptive names (eg. > grafana-6.1.3-fix_file_perms.patch, grafana-6.1.3-fix_oauth.patch, etc...) > Most of the patches (all but 002 ?) need to be submitted upstream and the > PR/issue URLs referenced in the specfile. > Upstream will likely want fine-grained commits, so the above patch split > will be needed anyway. I have to ask, as somebody looking forward to having grafana available as a properly packaged rpm in fedora, and considering the herculean effort Mark has put in here already, are these kind of patch tweaks/splits and bikeshedding on patch names really now the only blockers for getting grafana into fedora? Or are they things Mark can work on doing over time? I understand and emphatically agree with the notion of getting patches back upstream, but are we going to block Grafana's inclusion until they're accepted upstream? or can Mark take care of adjusting those patches when Grafana does a release and he updates the fedora packaged version to match? In other words, how much of a blocker is taking the one patch and splitting it into 5 (6?) really bringing to the state of the package, such that it's worth blocking the release over it? It'd be nice if we could set a concrete goal at what remains to be done for the fedora-review+ flag to be given, and not a constantly moving set of goalposts. Thanks for spending the time reviewing the work. Looking forward to being able to run grafana through the proper channels!
I too am very glad Mark and Nathan did all the work for packaging grafana and I too agree this is/was a huge task that deserves a lot of thanks. At this point, only minor points are remaining and are likely not blockers. If you saw my comments as moving the goals, this is definitely a misunderstanding and not my intent. A package review is an iterative work, for both the packager and the reviewer(s). You start with the big items and you refine until only the minor ones are remaining. We are at this stage now. I would have loved to be able to point everything out in one go, but that is simply not something that is possible for a package like grafana :-) About the patches, the unclear name and content are not helping anyone, nor the reviewer, nor the packager, nor upstream. Splitting and renaming the patches is not a lot of work and that needs to be done to upstream the patches, which is mandated both by the packaging guidelines and Fedora's philosophy. The real blocker for approving the package now is finding time to do the formal review.
Have split into 6 patches, in the order they would be sent upstream as PRs and will begin the process. For details, see https://mgoodwin.fedorapeople.org/grafana/grafana-patches.txt Have also added %check section to run all 56 go tests. This is needed for distro gating requirements. Note 000-go-test-fixes.patch fixes some go test issues on 32bit platforms (fixes int32 overflow issues). Spec URL: https://mgoodwin.fedorapeople.org/grafana/grafana.spec SRPM URL: https://mgoodwin.fedorapeople.org/grafana/grafana-6.1.3-1.fc29.src.rpm koji rawhide scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=34204277
Hi Mark. Great, both for the patches and the test suite. Please add comments referencing the PRs for the patches once you have submitted them. Please file an issue upstream and reference it in the spec for the failing test. Use %global instead of %define in the following line: %define _debugsource_template %{nil} There's a typo in the tmpfs conf file creation, its content is likely invalid: {%GRAFANA_GROUP} --> %{GRAFANA_GROUP} A brp script is complaining: """ + /usr/lib/rpm/redhat/brp-mangle-shebangs BUILDSTDERR: *** WARNING: ./etc/grafana/ldap.toml is executable but has empty or no shebang, removing executable bit BUILDSTDERR: *** WARNING: ./etc/grafana/grafana.ini is executable but has empty or no shebang, removing executable bit BUILDSTDERR: *** WARNING: ./etc/sysconfig/grafana-server is executable but has empty or no shebang, removing executable bit BUILDSTDERR: *** WARNING: ./usr/share/grafana/conf/defaults.ini is executable but has empty or no shebang, removing executable bit """ Please fix the modes preferably in 003-file-mode-updates.patch and 004-grafana.ini-for-Linux-distros.patch or in %install. rpmlint doesn't catch this, because the brp script silently fixes it before. The unbundling of go packages should be done in %prep rather than %build. Same for the 2 hidden files removal. About unbundling, the more I look at the EL7 exception, the more I find it really ugly and likely does not conform to guidelines. Please remove the %if's around the go BuildRequires and the vendor dir cleaning, so they are always used. That way, it is not necessary to add back the 100+ lines of bundled go provides I wrote about in comment #39 and grafana can be built for EL7 once the missing golang packages are branched and built from Fedora. I have to check more closely, but the build is currently not using the Fedora flags. Sorry for not catching that earlier, because that is definitely not a minor point, despite what I wrote in comment #41. I think the upstream build script should not be used and the 2 binaries should be built using the %gobuild macro. After a bit of trial and error, something like that and a BR: compiler(go-compiler) works. %gobuild -o grafana-cli ./pkg/cmd/grafana-cli %gobuild -o grafana-server ./pkg/cmd/grafana-server This should allow to get rid of the arch symlinks at top of %install. Maybe that would also remove the need for %global _debugsource_template %{nil} ? Regards, Xavier
Xavier, bit of time off over Easter - everything in Comment #43 makes sense, thanks. Especially the typo :P (Will get a new update ready asap, likely tuesday)
Hi Mark, The %gobuild macro doesn't help with _debugsource_template. There are %gotest and %gochecks macros that shall be used in %check. https://fedoraproject.org/wiki/More_Go_packaging#.25check:_.25gochecks_and_.25.7Bgotest.7D Also, given the fair amount of likely Fedora/RHEL specific macros that are now in the spec, I'm wondering if the conditional around grafana.ini and defaults.ini installation makes sense anymore ? I guess adding openSuSE and Mageia to your copr repo will find out. Enjoy your weekend, it'll be a long one for me too !
(In reply to Xavier Bachelot from comment #43) > Please add comments referencing the PRs for the patches once you have > submitted them. will do > Please file an issue upstream and reference it in the spec for the failing > test. will do > Use %global instead of %define in the following line: > %define _debugsource_template %{nil} done > There's a typo in the tmpfs conf file creation, its content is likely > invalid: > {%GRAFANA_GROUP} --> %{GRAFANA_GROUP} well spotted, thanks. fixed > A brp script is complaining: > """ > + /usr/lib/rpm/redhat/brp-mangle-shebangs > ... fixed > The unbundling of go packages should be done in %prep rather than %build. > Same for the 2 hidden files removal. done > About unbundling, the more I look at the EL7 exception, the more I find it > really ugly and likely does not conform to guidelines. > Please remove the %if's around the go BuildRequires and the vendor dir > cleaning, so they are always used. > That way, it is not necessary to add back the 100+ lines of bundled go > provides I wrote about in comment #39 and grafana can be built for EL7 once > the missing golang packages are branched and built from Fedora. ok done > I think the upstream build script should not be used and the 2 binaries > should be built using the %gobuild macro. > After a bit of trial and error, something like that and a BR: > compiler(go-compiler) works. > %gobuild -o grafana-cli ./pkg/cmd/grafana-cli > %gobuild -o grafana-server ./pkg/cmd/grafana-server ok done (that %gobuild generates a LOT of messages) > This should allow to get rid of the arch symlinks at top of %install. yep, now no longer needed > Maybe that would also remove the need for %global _debugsource_template > %{nil} ? correct. And now we have a debugsource package too > The %gobuild macro doesn't help with _debugsource_template. it's working now, so maybe something in rawhide has been updated to fix that > There are %gotest and %gochecks macros that shall be used in %check. > https://fedoraproject.org/wiki/More_Go_packaging#.25check:_.25gochecks_and_.25.7Bgotest.7D now using %gochecks > Also, given the fair amount of likely Fedora/RHEL specific macros that are now in the spec, > I'm wondering if the > conditional around grafana.ini and defaults.ini installation > makes sense anymore ? I guess adding openSuSE and Mageia to your copr repo will find out. > the intent is for a more generic default ini, i.e. suitable for distros, as opposed to being grafana.com specific for things like checking for updates, usage stats being sent to grafana.com, etc. See diff conf/defaults.ini conf/distro-defaults.ini Have also updated to latest upstream sources, grafana-6.1.4 thanks Spec URL: https://mgoodwin.fedorapeople.org/grafana/grafana.spec SRPM URL: https://mgoodwin.fedorapeople.org/grafana/grafana-6.1.4-1.fc31.src.rpm koji rawhide build: https://koji.fedoraproject.org/koji/taskinfo?taskID=34378287
(In reply to Mark Goodwin from comment #46) > > > There are %gotest and %gochecks macros that shall be used in %check. > > https://fedoraproject.org/wiki/More_Go_packaging#.25check:_.25gochecks_and_.25.7Bgotest.7D > > now using %gochecks > The tests are not running anymore: Executing(%check): /bin/sh -e /var/tmp/rpm-tmp.vcEtmu + umask 022 + cd /builddir/build/BUILD + cd grafana-6.1.4 + rm -f pkg/services/provisioning/dashboards/file_reader_linux_test.go + PATH=/builddir/build/BUILD/grafana-6.1.4/_bin:/builddir/.local/bin:/builddir/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/sbin + GO_TEST_FLAGS='-buildmode pie -compiler gc' + GO_TEST_EXT_LD_FLAGS='-Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld ' + go-rpm-integration check -i /builddir/build/BUILD/src/github.com/grafana/grafana/pkg -p /builddir/build/BUILDROOT/grafana-6.1.4-1.fc31.x86_64 -g /usr/share/gocode -b /builddir/build/BUILD/grafana-6.1.4/_build -r '.*example.*' Testing: /builddir/build/BUILD/src/github.com/grafana/grafana/pkg Path /builddir/build/BUILD/src/github.com/grafana/grafana/pkg not found on /builddir/build/BUILD/grafana-6.1.4/_build + exit 0
(In reply to Xavier Bachelot from comment #47) > (In reply to Mark Goodwin from comment #46) > > > > > There are %gotest and %gochecks macros that shall be used in %check. > > > https://fedoraproject.org/wiki/More_Go_packaging#.25check:_.25gochecks_and_.25.7Bgotest.7D > > > > now using %gochecks > > > The tests are not running anymore: > [...] > Testing: /builddir/build/BUILD/src/github.com/grafana/grafana/pkg > Path /builddir/build/BUILD/src/github.com/grafana/grafana/pkg not found on > /builddir/build/BUILD/grafana-6.1.4/_build > + exit 0 The docs say to simply use: %check %gochecks But that doesn't actually run any tests, successfully. So I have reverted to what I had previously: "go test ./pkg/..." until the issues in the go-rpm-integration script can be sorted out in rawhide (seems to be a path issue with _build or something). I also removed the last remaining rhel / fedora conditional stuff (e.g. for the distro version of config.ini). Spec URL: https://mgoodwin.fedorapeople.org/grafana/grafana.spec SRPM URL: https://mgoodwin.fedorapeople.org/grafana/grafana-6.1.4-1.fc31.src.rpm koji rawhide build: https://koji.fedoraproject.org/koji/taskinfo?taskID=34429399
Realistically all is in readiness with the spec now, and from one more review pass I can see no remaining issues either. Chatting to Mark, he's continuing to work on some upstream PRs for Grafana to use the latest vendor sources and will add in additional spec comments and any other %check improvements that others can suggest. Lets get some repos and branches setup now, do some builds and and open up to a wider Fedora testing audience. Package approved. Thanks for sticking with it Mark, and all our other reviewers!
(In reply to Mark Goodwin from comment #48) > The docs say to simply use: > > %check > %gochecks > > But that doesn't actually run any tests, successfully. Maybe adding %gobuildroot in %build instead of symlinking yourself will fix it?
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/grafana
Updated to grafana v6.1.6, which includes the jQuery security update. Built all arches for rawhide. Still waiting for golang build deps to propagate to stable for f29 and f30. Will continue to work on %gocheck and to get the patches accepted upstream. Thanks for all the BR work from Nathan and to everyone for the review comments!
grafana-6.1.6-1.fc30 has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-6e8d7cf6a6
grafana-6.1.6-1.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-6e8d7cf6a6
grafana-6.1.6-1.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-6710741f2c
grafana-6.1.6-1.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-6710741f2c
grafana-6.1.6-1.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.
grafana-6.1.6-1.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.