https://bugzilla.redhat.com/show_bug.cgi?id=2022991

--- Comment #1 from Oleg Girko ol+redhat@infoserver.lv --- 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.

https://bugzilla.redhat.com/show_bug.cgi?id=2022991

--- Comment #3 from Oleg Girko ol+redhat@infoserver.lv --- (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.

https://bugzilla.redhat.com/show_bug.cgi?id=2022991

--- Comment #5 from Vitaly Zaitsev vitaly@easycoding.org ---

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.

https://bugzilla.redhat.com/show_bug.cgi?id=2022991

--- Comment #7 from Oleg Girko ol+redhat@infoserver.lv --- 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.

https://bugzilla.redhat.com/show_bug.cgi?id=2022991

--- Comment #9 from Oleg Girko ol+redhat@infoserver.lv --- 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.r...

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.

https://bugzilla.redhat.com/show_bug.cgi?id=2022991

Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:

What |Removed |Added ---------------------------------------------------------------------------- CC| |zbyszek@in.waw.pl

--- Comment #11 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl ---

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-repla....

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.

Product: Fedora Version: rawhide Component: Package Review

Oleg Girko ol+redhat@infoserver.lv has canceled Package Review package-review@lists.fedoraproject.org's request for Oleg Girko ol+redhat@infoserver.lv's needinfo: Bug 2022991: Review Request: i2pd - I2P router written in C++ https://bugzilla.redhat.com/show_bug.cgi?id=2022991

--- Comment #13 from Oleg Girko ol+redhat@infoserver.lv --- 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.