Bug 2022991

This is the first package that I submit to Fedora review, so I need sponsorship to become a packager.
I've built i2pd successfully in my OBS (https://obs.infoserver.lv/project/monitor/i2p) and I use it on the regular basis on my computers.

1. You shouldn't use macroses with double underline (%{__install}).
2. systemd units and logic should be moved to the base package.
3. `%cmake3` should be used only on EPEL7. On Fedora and EPEL8+EPEL9 should be used %cmake.
4. `chrpath -d i2pd` can be dropped. Use `-DCMAKE_SKIP_INSTALL_RPATH:BOOL=ON` instead.
5. `%{_builddir}/%{name}-%{version}/contrib/i2pd.conf` - always use relative paths.
6. `Move config files from old version to proper place` - such scriplets are not allowed. You must install everything in %install.
7. Add %changelog section.
8. `Obsoletes:      %{name}-daemon` - can be removed. Fedora has no package with such name.
9. `cd build` should be dropped. %cmake macro will do everything automatically.
10. `%{?cmake3_build}%{?!cmake3_build:%{make_build}}` must be replaced with `%cmake_build`.
11. `%{?__cmake_builddir:cd %{__cmake_builddir}}` can be dropped.

(In reply to Vitaly Zaitsev from comment #2)
> 1. You shouldn't use macroses with double underline (%{__install}).

Done.

> 2. systemd units and logic should be moved to the base package.

The reason for making a separate systemd subpackage is to make it optional. Not everybody wants to run i2pd as a system service, some people may prefer to run it as a regular program.
I've just added "Recommends" for systemd subpackage instead.

> 3. `%cmake3` should be used only on EPEL7. On Fedora and EPEL8+EPEL9 should
> be used %cmake.

I'd like to have a single spec file for both CentOS 7 and Fedora and avoid conditional statements as much as possible. I'm going to keep using the same spec file to build CentOS and Fedora packages in my OBS for some time. Having separate spec files for building with my OBS and with Fedora infrastructure would be confusing for me and can lead to mistakes. Using %cmake3 macro works pretty well for CentOS and across a wide range of Fedora versions, and there are no indications that this macro is going to be dropped in foreseeable future.

> 4. `chrpath -d i2pd` can be dropped. Use
> `-DCMAKE_SKIP_INSTALL_RPATH:BOOL=ON` instead.

Done using CMAKE_SKIP_RPATH variable instead. There is no %cmake_install in %install section, all files are installed manually, no internal shared libraries used, so no need to build i2pd with rpath pointing to builddir.

> 5. `%{_builddir}/%{name}-%{version}/contrib/i2pd.conf` - always use relative
> paths.

Done. I've reworked %install section to not cd to %{__cmake_builddir). This was needed to install just one file anyway.

> 6. `Move config files from old version to proper place` - such scriplets are
> not allowed. You must install everything in %install.

This is needed for migration from older version of this package. Some of users may have older version installed from my OBS repo, and it would be a very unpleasant surprise for them if their configuration was not migrated to the new place automatically. Is there a better, more recommended way to migrate configuration files to a new (more standards conforming) location?

> 7. Add %changelog section.

There is a changelog, it's just stored in a separate file in OBS: https://obs.infoserver.lv/package/view_file/i2p/i2pd/i2pd.changes?expand=1
OBS combines spec file with changes file before building RPM package, so resulted packages have proper changelog, and the spec file included in the source RPM, contains proper changelog as well.
Sorry for forgetting to mention it in this ticket's description.

> 8. `Obsoletes:      %{name}-daemon` - can be removed. Fedora has no package
> with such name.

Again, this is needed for those who upgrade a package from my OBS (although, quite an old version).

> 9. `cd build` should be dropped. %cmake macro will do everything
> automatically.

It can't. The "build" directory is a special part of i2pd source, not a normal builddir: "CMakeLists.txt" file and cmake modules are located there. The builddir is created inside "build" directory for out of source build.

> 10. `%{?cmake3_build}%{?!cmake3_build:%{make_build}}` must be replaced with
> `%cmake_build`.

This will break build for older Fedora versions (<31) that have no %cmake_build macro.
If this is strictly necessary, I can consider dropping building for Fedora versions < 31 in my OBS and stopping supporting them, but I'd prefer to leave it as is because it has personal value for me, as an example of extremely portable spec file I've carefully crafted with as minimal changes as possible, that is still compatible with much older Fedora versions.

> 11. `%{?__cmake_builddir:cd %{__cmake_builddir}}` can be dropped.

I've dropped changing directory in %install section, but I still have to use another variant of this conditional macro for i2pd path, for spec file portability, as described above.

New release is available.
Spec URL: https://obs.infoserver.lv/package/view_file/i2p/i2pd/i2pd.spec?expand=1
Changelog URL: https://obs.infoserver.lv/package/view_file/i2p/i2pd/i2pd.changes?expand=1
SRPM URL: https://obs.infoserver.lv/repos/i2p/Fedora_35/src/i2pd-2.39.0-3.fc35.1.src.rpm

Please ignore separate spec and changelog files, this is how OBS works. It combines them when building packages, so resulting binary and source packages contain proper changelog, and spec file included in SRPM has changelog included.
Also please ignore that I didn't increase release number, OBS does this automatically, as you can see from SRPM file name and its version.

> The reason for making a separate systemd subpackage is to make it optional. Not everybody wants to run i2pd as a system service, some people may prefer to run it as a regular program.

It's completely useless without systemd modules. Fedora uses systemd as its unified init system. That's why I think, these units should be moved to the base package.

> I'd like to have a single spec file for both CentOS 7 and Fedora and avoid conditional statements as much as possible.

%cmake3 macro is obsolete and should not be used. Use %cmake for Fedora and %cmake3 for RHEL7.

Also EPEL7 will use its own git branch epel7. You can make such changes in it.

> Done using CMAKE_SKIP_RPATH variable instead. There is no %cmake_install in %install section, all files are installed manually, no internal shared libraries used, so no need to build i2pd with rpath pointing to builddir.

RPATH is strictly forbidden on Fedora. If it exists, it must be removed.

> This is needed for migration from older version of this package. Some of users may have older version installed from my OBS repo, and it would be a very unpleasant surprise for them if their configuration was not migrated to the new place automatically. Is there a better, more recommended way to migrate configuration files to a new (more standards conforming) location?

Such scriptlets are not allowed, because they are touching different files and directories not owned by your package. If you want to publish your package to Fedora, it must follow current Fedora packaging guidelines.

In general such scriplets must be guarded with %triggerrun:

%triggerun -- %{name} < 1.0.0-2
... do some hacks ...

It will only be executed by RPM if the installed version meets the condition.

> There is a changelog, it's just stored in a separate file in OBS

Fedora doesn't allow detached changelogs (only auto-generated from Git history).

> Changelog URL: https://obs.infoserver.lv/package/view_file/i2p/i2pd/i2pd.changes?expand=1

Incorrect. You must move contents of this file to %changelog section of the base SPEC.

> It can't. The "build" directory is a special part of i2pd source, not a normal builddir: "CMakeLists.txt" file and cmake modules are located there. The builddir is created inside "build" directory for out of source build.

Just use %cmake -S build.

> This will break build for older Fedora versions (<31) that have no %cmake_build macro.

We don't have to worry about the EOLed versions of Fedora. Currently supported are F34, F35 and Rawhide. You should do this.

> Also please ignore that I didn't increase release number, OBS does this automatically, as you can see from SRPM file name and its version.

Fedora uses Koji, not OBS.

> Not everybody wants to run i2pd as a system service, some people may prefer to run it as a regular program.

Fedora always installs systemd units in a disabled by default state. Users need to explicitly enable them with systemctl enable foo-bar.service. There is nothing to worry about.

> BuildRequires:  cmake3

BuildRequires: cmake should be used on Fedora.

> %setup -q

Can be replaced with %autosetup.

> %files

You must also include license: %license LICENSE.

So, I presume, you want me to make the spec file as readable and simple as possible.
I want the same spec file to retain compatibility with different versions of Fedora, as well as CentOS/RHEL, and not break any builds in my OBS.
Let's try to find middle ground.

I've localised all compatibility workarounds to 3 first lines of the spec file.

(In reply to Vitaly Zaitsev from comment #5)
> > The reason for making a separate systemd subpackage is to make it optional. Not everybody wants to run i2pd as a system service, some people may prefer to run it as a regular program.
> 
> It's completely useless without systemd modules. Fedora uses systemd as its
> unified init system. That's why I think, these units should be moved to the
> base package.

This is not only about systemd service file, it's also about creating a user and having systemwide configuration files.
But anyway, I've made a concession here and merged systemd subpackage into the main package.

Unfortunately, this caused systemd service to stop and become disabled during package upgrade because it removes obsolete i2pd-systemd package, and %systemd_preun macro used in %preun script of this removed package disables and stops i2pd service. The service file now belongs to i2pd package and has to be enabled and started again manually. I didn't find a sane way to prevent this from happening.

> > I'd like to have a single spec file for both CentOS 7 and Fedora and avoid conditional statements as much as possible.
> 
> %cmake3 macro is obsolete and should not be used.

So, to future-proof my spec file against removal of %cmake3 macro, I've added check in the beginning of the spec file, so %cmake3 will be defined as %cmake if this macro is not defined.

> Use %cmake for Fedora and %cmake3 for RHEL7.

And having different spec files for different distributions (or having different spec files in Fedora infrastructure and in my OBS) is what I strongly oppose against.

> > Done using CMAKE_SKIP_RPATH variable instead. There is no %cmake_install in %install section, all files are installed manually, no internal shared libraries used, so no need to build i2pd with rpath pointing to builddir.
> 
> RPATH is strictly forbidden on Fedora. If it exists, it must be removed.

This is why I use CMAKE_SKIP_RPATH. This results in RPATH not included in the binary during the build (instead of being removed during install).

> > This is needed for migration from older version of this package. Some of users may have older version installed from my OBS repo, and it would be a very unpleasant surprise for them if their configuration was not migrated to the new place automatically. Is there a better, more recommended way to migrate configuration files to a new (more standards conforming) location?
> 
> Such scriptlets are not allowed, because they are touching different files
> and directories not owned by your package. If you want to publish your
> package to Fedora, it must follow current Fedora packaging guidelines.
> 
> In general such scriplets must be guarded with %triggerrun:
> 
> %triggerun -- %{name} < 1.0.0-2
> ... do some hacks ...

OK, I've moved it to %triggerun section.

> > There is a changelog, it's just stored in a separate file in OBS
> 
> Fedora doesn't allow detached changelogs (only auto-generated from Git
> history).
> 
> > Changelog URL: https://obs.infoserver.lv/package/view_file/i2p/i2pd/i2pd.changes?expand=1
> 
> Incorrect. You must move contents of this file to %changelog section of the
> base SPEC.

Please don't worry about this. This is not detached changelog. It's just the way OBS stores spec file and its %changelog section. The spec file used for build (and included into SRPM as a result) has proper %changelog section. And of course, the spec file in the Fedora's Git repo will also have full %changelog once this repo is created.

> > It can't. The "build" directory is a special part of i2pd source, not a normal builddir: "CMakeLists.txt" file and cmake modules are located there. The builddir is created inside "build" directory for out of source build.
> 
> Just use %cmake -S build.

No, I can't. The %cmake macro in Fedora already uses -S option. Also, changing directory before building looks more readable to my taste.

> > This will break build for older Fedora versions (<31) that have no %cmake_build macro.
> 
> We don't have to worry about the EOLed versions of Fedora. Currently
> supported are F34, F35 and Rawhide. You should do this.

Now I use %cmake3_build. All compatibility workarounds are localised in one place.

> > Also please ignore that I didn't increase release number, OBS does this automatically, as you can see from SRPM file name and its version.
> 
> Fedora uses Koji, not OBS.

Yes, I know. But since i2pd is still built in my OBS, not in Fedora's Koji, I don't need to increase the release number. I'll definitely will be doing this for builds inside Koji.

(In reply to Vitaly Zaitsev from comment #6)
> > BuildRequires:  cmake3
> 
> BuildRequires: cmake should be used on Fedora.

It's exactly the same for Fedora because cmake package version 3 provides cmake3 capability.
But it doesn't work for CentOS 7 where cmake and cmake3 are different packages.
And I still insist on using the same spec file for Fedora's Koji and my OBS where I build for CentOS 7 as well.

> > %setup -q
> 
> Can be replaced with %autosetup.

As I don't use patches, there's no difference.
May be I'll consider using %autosetup later, if patches are needed.

> > %files
> 
> You must also include license: %license LICENSE.

Done.

New release is available.
Spec URL: https://obs.infoserver.lv/package/view_file/i2p/i2pd/i2pd.spec?expand=1
Changelog URL: https://obs.infoserver.lv/package/view_file/i2p/i2pd/i2pd.changes?expand=1
SRPM URL: https://obs.infoserver.lv/repos/i2p/Fedora_35/src/i2pd-2.39.0-4.fc35.1.src.rpm

Please note that separate URLs for spec file and changelog are just for convenience. It's not a detached changelog. The actual spec file contains proper %changelog section.

Hmm. Funny. I've just found out that cmake3 workaround is not needed even for CentOS 7: i2pd build succeeds with cmake 2. I was using this workaround because upstream uses it in its spec file. I have no idea why they decided to do it.

Anyway, this unexpected discovery allowed me to simplify spec file by removing cmake3 workarounds and reducing compatibility workarounds to just two simple lines in the beginning of the spec file.
Also, version 2.40.0 has been released, so I've packaged it.

As a result, my spec file became even more portable, so it can be used to build packages for all Fedora versions starting from 23 (didn't test earlier ones), for CentOS 7 with EPEL, and even for Mageia 8!

Spec URL: https://obs.infoserver.lv/package/view_file/i2p/i2pd/i2pd.spec?expand=1
Changelog URL: https://obs.infoserver.lv/package/view_file/i2p/i2pd/i2pd.changes?expand=1
SRPM URL: https://obs.infoserver.lv/repos/i2p/Fedora_35/src/i2pd-2.40.0-1.fc35.1.src.rpm

Please note that separate URLs for spec file and changelog are just for reading convenience. It's not a detached changelog. The actual spec file inside SRPM contains proper %changelog section, and I'll use it once I'm allowed to create a repo in Fedora infrastructure.

> BuildRequires:  systemd-units
There is not such package since a long long time, this virtual provides is now attached
to systemd. And in general you don't need to pull in systemd for the build. (It has a
lot of dependencies, so it's nicer to avoid it.)
If you only need %{_unitdir} and such, systemd-rpm-macros is enough.

> C++ implementation of I2P.
It'd be nice to expand this a bit. The implementation language usually isn't important
to users, and many people will not know who I2P is… 

> Obsoletes:      %{name}-daemon
> Obsoletes:      %{name}-systemd

Those should be versioned, with a fixed version, see https://docs.fedoraproject.org/en-US/packaging-guidelines/#renaming-or-replacing-existing-packages.

> Requires:       systemd
This should be removed. The general idea is that on a normal system you'll have systemd
installed anyway, so the dependency doesn't matter. But if someone is building e.g. a
container image w/o systemd or something custom, the dependency just gets in the way.

> Requires:       logrotate
Hmm, this is allowed, and there certainly are packages using custom logs, but since
you're introducing the package to Fedora, it is an easy moment to change user expectations.
If the program doesn't log too much, it'd be nicer to just let it log to the journal
because the UX is better.

Did you consider using %autorelease and %autochangelog? This would make the
packaging even a bit easier [https://docs.pagure.org/fedora-infra.rpmautospec/].

In general this spec file look OK to me.
We should wait for https://pagure.io/packaging-committee/issue/1158 to be resolved,
but at least my understanding (as a person who worked on the new macros…), that the
classical version is still OK.

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

Yes, I'm still interested.
Some time has passed. Since then, CMake requirement changed, so my idea to support RHEL 7 with the same spec file is not viable anymore.
Please give me a few days to prepare updated files for review. I'm not sure that I'll be able to do it before FOSDEM, though.

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.