Requesting sponsor for this. Have introduced myself on the devel mailing list and this is my first package submitted for review.

(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.fedoraproject.org&q=Stefan+Midjich

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.

 - 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)

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)

Created attachment 1645283 [details]
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.

 - 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

 - 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)

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

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

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)