https://bugzilla.redhat.com/show_bug.cgi?id=1770147
Bug ID: 1770147 Summary: Review Request: ntsclient - Golang NTS client Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: swehack@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://gitlab.com/hacklunch/ntsclient/raw/v0.4.1/contrib/ntsclient.spec SRPM URL: https://gitlab.com/hacklunch/ntsclient/-/jobs/343500225/artifacts/raw/artifa... Description: Network Time Security client in Golang, for secure NTP. Fedora Account System Username:stemid
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
Stefan Midjich swehack@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
--- Comment #1 from Stefan Midjich swehack@gmail.com --- Requesting sponsor for this. Have introduced myself on the devel mailing list and this is my first package submitted for review.
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
--- Comment #2 from Stefan Midjich swehack@gmail.com --- (In reply to Stefan Midjich from comment #1)
Requesting sponsor for this. Have introduced myself on the devel mailing list and this is my first package submitted for review.
My personal introduction on the lists: https://lists.fedoraproject.org/archives/search?mlist=devel%40lists.fedorapr...
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
Elliott Sales de Andrade quantum.analyst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |quantum.analyst@gmail.com
--- Comment #3 from Elliott Sales de Andrade quantum.analyst@gmail.com --- This needs to follow the Go packaging guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/Golang/ and systemd packaging guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/
and possibly some SELinux ones, but I don't see any. Probably best to ask on the mailing list for someone to review those.
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
Robert-André Mauchin zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zebob.m@gmail.com
--- Comment #4 from Robert-André Mauchin zebob.m@gmail.com --- - This is not defined anywhere:
Version: %{upstream_version}
Latest version seems to be 0.4.1
- What is the purpose of this??
test -d '%{name}-%{version}' && chmod -R u+w '%{name}-%{version}'
- Use the SystemD macros:
BuildRequires: systemd-rpm-macros
[...] %post %systemd_post %{name}.service
%preun %systemd_preun %{name}.service
%postun %systemd_postun_with_restart %{name}.service
- Changelog entry is not good:
* Tue Oct 29 2019 Stefan Midjich <swehack at gmail.com> - 0.4.1-1
- Use the Golang Packaging Guidelines:
https://docs.fedoraproject.org/en-US/packaging-guidelines/Golang/
Use go2rpm to help you:
# Generated by go2rpm 1 %bcond_without check
# https://gitlab.com/hacklunch/ntsclient %global goipath gitlab.com/hacklunch/ntsclient %global forgeurl https://gitlab.com/hacklunch/ntsclient Version: 0.4.1
%gometa
%global common_description %{expand: Small Network Time Security Client.}
%global golicenses LICENSE %global godocs README.md
Name: %{goname} Release: 1%{?dist} Summary: Small Network Time Security Client
License: ISC URL: %{gourl} Source0: %{gosource}
BuildRequires: golang(github.com/BurntSushi/toml) BuildRequires: golang(gitlab.com/hacklunch/ntp) BuildRequires: golang(gitlab.com/hacklunch/ntske)
%description %{common_description}
%prep %goprep
%build %gobuild -o %{gobuilddir}/bin/ntsclient %{goipath}
%install install -m 0755 -vd %{buildroot}%{_bindir} install -m 0755 -vp %{gobuilddir}/bin/* %{buildroot}%{_bindir}/
%if %{with check} %check %gocheck %endif
%files %license LICENSE %doc README.md %{_bindir}/*
%changelog
- You'll need to package first:
BuildRequires: golang(gitlab.com/hacklunch/ntp) BuildRequires: golang(gitlab.com/hacklunch/ntske)
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
--- Comment #5 from Stefan Midjich swehack@gmail.com --- First on my agenda is what was mentioned by Elliot to follow the Go packaging guidelines. I will also look at go2rpm that you recommended.
(In reply to Robert-André Mauchin from comment #4)
- This is not defined anywhere:
Version: %{upstream_version}
Latest version seems to be 0.4.1
- What is the purpose of this??
The RPM package is being built by a CI/CD pipeline and that is the reason I'm passing upstream_version from rpmbuild CLI.
test -d '%{name}-%{version}' && chmod -R u+w '%{name}-%{version}'
- Use the SystemD macros:
BuildRequires: systemd-rpm-macros
I will make another attempt at using them but again because of the CI/CD pipeline used to build the RPM packages I was forced to use the more basic method. In fact copied from the macros.
[...] %post %systemd_post %{name}.service
%preun %systemd_preun %{name}.service
%postun %systemd_postun_with_restart %{name}.service
- Changelog entry is not good:
- Tue Oct 29 2019 Stefan Midjich <swehack at gmail.com> - 0.4.1-1
- Use the Golang Packaging Guidelines:
https://docs.fedoraproject.org/en-US/packaging-guidelines/Golang/
Use go2rpm to help you:
# Generated by go2rpm 1 %bcond_without check
# https://gitlab.com/hacklunch/ntsclient %global goipath gitlab.com/hacklunch/ntsclient %global forgeurl https://gitlab.com/hacklunch/ntsclient Version: 0.4.1
%gometa
%global common_description %{expand: Small Network Time Security Client.}
%global golicenses LICENSE %global godocs README.md
Name: %{goname} Release: 1%{?dist} Summary: Small Network Time Security Client
License: ISC URL: %{gourl} Source0: %{gosource}
BuildRequires: golang(github.com/BurntSushi/toml) BuildRequires: golang(gitlab.com/hacklunch/ntp) BuildRequires: golang(gitlab.com/hacklunch/ntske)
%description %{common_description}
%prep %goprep
%build %gobuild -o %{gobuilddir}/bin/ntsclient %{goipath}
%install install -m 0755 -vd %{buildroot}%{_bindir} install -m 0755 -vp %{gobuilddir}/bin/* %{buildroot}%{_bindir}/
%if %{with check} %check %gocheck %endif
%files %license LICENSE %doc README.md %{_bindir}/*
%changelog
- You'll need to package first:
BuildRequires: golang(gitlab.com/hacklunch/ntp) BuildRequires: golang(gitlab.com/hacklunch/ntske)
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
--- Comment #6 from Stefan Midjich swehack@gmail.com --- Created attachment 1645283 --> https://bugzilla.redhat.com/attachment.cgi?id=1645283&action=edit New spec file based on go2rpm and commented to explain questionable methodology
I'm still trying to resolve some SElinux issues where ntsclient is running in init_t context even though its binary is labeled ntsclient_exec_t. Strangely because I've made no changes to the policy and this happened after uninstalling the old functional package, replacing it with the new package based on this new spec.
But this is an initial draft where I've addressed the comments and the parts I did not want to change I have commented to explain why.
Mainly it's because we're using a Debian based image in our CI/CD build pipeline and macros that might exist on Fedora do not exist there.
I'm also not 100% sure about the src package, the goal for me is to create a binary package but the source package requires manual installation of all Golang dependencies.
If our goal is a binary package, are we still forced to separately package all its dependencies?
I was unable to build the package using the %go_generate_buildrequires macro.
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
--- Comment #7 from Robert-André Mauchin zebob.m@gmail.com --- - Add a Summary
Summary: None
- The point of using %gobuild is that it set the Fedora build flags, %make_build does not.
# Easier to use %make_build than %gobuild macro when there is a Makefile. %make_build
- Use:
Version: 0.4.1 %global tag v0.4.1
with:
Source0: %{gosource}
- %generate_buildrequires doesn't work that way, it should go after goprep. We don't use it so far in Go packaging. Just put the BR the olld way for now:
BuildRequires: golang(github.com/BurntSushi/toml) BuildRequires: golang(gitlab.com/hacklunch/ntp) BuildRequires: golang(gitlab.com/hacklunch/ntske)
- Systemd rpm macros do not always exist in CI/CD environment
What does this has to do with Fedora? We're packaging in Koji, not in a custom CI/CD environment.
- I'm no expert on the SELinux stuff, I need someone to review this. Askk for some help on the devel ML.
- This is only usefil for the -devel subpackage, I don't think you need it for a binary package:
%gopkg
%gopkginstall
%gopkgfiles
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
--- Comment #8 from Robert-André Mauchin zebob.m@gmail.com --- - To expand a bit, your Makefile only contains:
$(BINARY): $(SRCS) go build -o $@
while the %gobuild macro contains Fedora LDFLAGS and also provision for the debuginfo to be correctly generated:
# https://bugzilla.redhat.com/show_bug.cgi?id=995136#c12 %ifnarch ppc64 GO111MODULE=off \ go build -buildmode pie -compiler gc -tags="rpm_crashtraceback ${BUILDTAGS:-}" -ldflags "${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -extldflags '-Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld '" -a -v -x ; %else GO111MODULE=off \ go build -compiler gc -tags="rpm_crashtraceback ${BUILDTAGS:-}" -ldflags "${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -extldflags '-Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld '" -a -v -x ; %endif
- You need to add:
BuildRequires: systemd-rpm-macros
for the SystemD scriptlet to work.
On older Fedora it was:
%{?systemd_requires} BuildRequires: systemd
(Not sure what your CI system is using)
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
--- Comment #9 from Robert-André Mauchin zebob.m@gmail.com --- The SELINUX scriptlets are not ok either, it should go likes this:
%post if [ "$1" -le "1" ] ; then # First install semodule -i %{_datadir}/selinux/packages/ntsclient.pp 2>/dev/null || : fi
%preun if [ "$1" -lt "1" ] ; then # Final removal semodule -r ntsclient 2>/dev/null || : fi
%postun if [ "$1" -ge "1" ] ; then # Upgrade semodule -i %{_datadir}/selinux/packages/ntsclient.pp 2>/dev/null || : fi
No need for setcap or restorecon here normally, this is handled by file triggers I believe.
Also you are missing some BR for this:
BuildRequires: selinux-policy-devel Requires(post): policycoreutils Requires(preun): policycoreutils Requires(postun): policycoreutils
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
--- Comment #10 from Robert-André Mauchin zebob.m@gmail.com --- Can't you use a Fedora image for your CI?
fedora-rawhide: image: registry.fedoraproject.org/fedora:rawhide when: always script: - dnf install --refresh -y @development-tools fedora-packager rpmdevtools
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
--- Comment #11 from Robert-André Mauchin zebob.m@gmail.com --- Then do a mockbuild:
fedpkg --release f32 mockbuild --mock-config fedora-rawhide-x86_64
that way everything is built within a mock chroot. The rpm will be in results_golang-gitlab-hacklunch-ntsclient/0.4.1/1.fc32/
Also you need to package the missing dependencies first:
BuildRequires: golang(gitlab.com/hacklunch/ntp) BuildRequires: golang(gitlab.com/hacklunch/ntske)
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
Elliott Sales de Andrade quantum.analyst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Whiteboard| |AwaitingSubmitter
https://bugzilla.redhat.com/show_bug.cgi?id=1770147
Stefan Midjich swehack@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Flags|needinfo?(swehack@gmail.com | |) | Resolution|--- |CANTFIX Last Closed| |2021-11-03 09:48:26
--- Comment #13 from Stefan Midjich swehack@gmail.com --- I am abandoning this as I don't have the time or understanding required to remedy all the issues with the packaging, and I haven't been involved in the package pipeline of the upstream project for over a year. There is NTS support in Chrony and I'm happy with that. At the time when this project was created we did not know when NTS support would make it into our distros.
Product: Fedora Version: rawhide Component: Package Review
Stefan Midjich swehack@gmail.com has canceled Package Review package-review@lists.fedoraproject.org's request for Stefan Midjich swehack@gmail.com's needinfo: Bug 1770147: Review Request: ntsclient - Golang NTS client https://bugzilla.redhat.com/show_bug.cgi?id=1770147
--- Comment #13 from Stefan Midjich swehack@gmail.com --- I am abandoning this as I don't have the time or understanding required to remedy all the issues with the packaging, and I haven't been involved in the package pipeline of the upstream project for over a year. There is NTS support in Chrony and I'm happy with that. At the time when this project was created we did not know when NTS support would make it into our distros.
package-review@lists.fedoraproject.org