Bug 2309760

Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=122947740

Copr build:
https://copr.fedorainfracloud.org/coprs/build/7979835
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2309760-accel-ppp/fedora-rawhide-x86_64/07979835-accel-ppp/fedora-review/review.txt

Found issues:

- pcre-devel is deprecated, you must not depend on it.
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/deprecating-packages/
- Not a valid SPDX expression 'GPL-2.0'.
  Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1
- Systemd service file(s) in accel-ppp
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Spec URL: https://neelc.org/fedora/accel-ppp/r2/accel-ppp.spec
SRPM URL: https://neelc.org/fedora/accel-ppp/r2/accel-ppp-1.13.0-1.fc42.src.rpm

https://koji.fedoraproject.org/koji/taskinfo?taskID=122949062

Old spec broke on i686 so use this instead.

Created attachment 2045415 [details]
The .spec file difference from Copr build 7979835 to 7979938

Copr build:
https://copr.fedorainfracloud.org/coprs/build/7979938
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2309760-accel-ppp/fedora-rawhide-x86_64/07979938-accel-ppp/fedora-review/review.txt

Found issues:

- pcre is deprecated, you must not depend on it.
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/deprecating-packages/
- Not a valid SPDX expression 'GPL-2.0'.
  Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1
- Systemd service file(s) in accel-ppp
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Notes/Questions:

1) license SPDX appears to be (GPL-2.0-Only OR GPL-2.0-Or-Later OR MIT)
2) pcre package is deprecated, will this build with pcre2? may need to pull in https://github.com/accel-ppp/accel-ppp/pull/185
3) when a package has a system unit associated, we need to handle this in the pre/post sections of the rpm install; see https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets
4) unversioned shared libraries must be handled -- see https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shared_libraries -- if upstream cannot version them, we can maintain versioning on it downstream. the unversioned .so files can be in a devel subpackage, if you want
5) you can use globs for non-library files, so instead of listing all the entries in `%{_datadir}/accel-ppp/radius/`, you could just add `%{_datadir}/accel-ppp/radius/dictionary*`. as long as it's not a shared directory or is shared object libraries, globbing is OK.
6) need to own directories --      Note: Directories without known owners: /usr/share/accel-ppp, /usr/lib64/accel-ppp, /usr/share/accel-ppp/l2tp, /usr/share/accel-ppp/radius
7) can any tests be run in %check?

lastly, regarding

># We need Centos9 for modern Fedora versions alongside EPEL 9
> %cmake -DCMAKE_BUILD_TYPE=Release -DCPACK_TYPE=Centos9

This is probably OK, but we should probably ask upstream if adding a Fedora config would be possible, as that config will diverge from either Fedora or CentOS/EPEL at some point in the future.

I am having trouble adding versioning to my accel-ppp package. CMake has a SOVERSION but that for some reason isn't adding versioning.

I'm trying both the accel-pppd itself, didn't work. try each module individually, fails on build:

CMake Error at CMakeLists.txt:105 (set_target_properties):
  set_target_properties Can not find target to add properties to: TARGET

Any pointers?

I doubt that the .so file are development libraries but dlopened() code - that's why they are installed into %{_libdir}/accel-pppd instead of %{_libdir}.  They do not need versioning.

The build system is rather wonky for this one.
- cpack wise, we don't use it at all, so you can ignore tgat variable
- minimum cmake version is waaay too old and it *should* be bumped, otherwise it would be skipping some default settings that are assumed when writing a cmake project
- check of you want to set any of the `BUILD_PPTP_DRIVER` options
- upstream abuses `CMAKE_C_FLAGS` incredibly. Make sure Fedora's flags are being picked up appropriately and not overridden
- how did you try to add `set_target_properties`? You can't add it to `accel-pppd` because it's and executable. Something like `crypto` (`internal-crypto` target) can set the versions
- we don't want to use the RPATH that they are setting, instead `/etc/ld.so.conf.d/` would work, assuming there are no SONAME/SOVERSION collisions
- the sbin install directory should be changed to bin to prepare for thr merger
- I believe pcre2 should be preferred instead of pcre. Not sure how compatible the APIs are
- there is sooo much more to review in this package, I don't know if rewriting their build system is faster than pointing out each issues

(In reply to Orion Poplawski from comment #8)
> I doubt that the .so file are development libraries but dlopened() code -
> that's why they are installed into %{_libdir}/accel-pppd instead of
> %{_libdir}.  They do not need versioning.

They seem to be configured by accel-ppp.conf so should make sure a good default isstructure. Haven't checked if it has some inheritance structure. This does conflict with the RPATH they are setting

I feel versioning libraries doesn't make sense here, as the build system won't allow it. Samba doesn't version on Fedora either:

$ ls -l /usr/lib64/samba
-rwxr-xr-x. 1 root root   61712 Aug 16 20:00 libaddns-private-samba.so
-rwxr-xr-x. 1 root root  187152 Aug 16 20:00 libads-private-samba.so
...
-rwxr-xr-x. 1 root root    7672 Aug 16 20:00 libutil-setid-private-samba.so
-rwxr-xr-x. 1 root root   12696 Aug 16 20:00 libutil-tdb-private-samba.so

And Samba is a **major** FOSS project. However, I am not sure if the rules are different and whatnot.

But regardless, accel-ppp is a hard project to package. There's a reason why there's why only Alpine packaged accel-ppp afaict.

About my updated specs:

Spec URL: https://neelc.org/fedora/accel-ppp/r4/accel-ppp.spec
SRPM URL: https://neelc.org/fedora/accel-ppp/r4/accel-ppp-1.13.0-1.fc42.src.rpm
Patch0: https://neelc.org/fedora/accel-ppp/r4/0001-Use-PCRE2-instead-of-PCRE.patch
Patch1: https://neelc.org/fedora/accel-ppp/r4/0002-Add-Fedora-CPack-option.patch

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=123906657

Created attachment 2048501 [details]
The .spec file difference from Copr build 7979938 to 8066452

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8066452
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2309760-accel-ppp/fedora-rawhide-x86_64/08066452-accel-ppp/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Agreed that this is a difficult one, and probably better to err on the side of not versioning it. The guidelines[1] state that we "MUST" try and convince upstream to version the SO, but I believe that may be done in parallel, as this won't affect anything meaningfully in Fedora if this occurs. Just be mindful in the future if packages depend on this, and there is an ABI change, branch packages may require rebuilds.

Thank you, Orion and Cristian for your input!

Notes:

1) In Fedora we can just use `ExcludeArch: %{ix86}` as we i686 artifacts are not required. For EPEL, instead of:

```
%ifnarch i686
# Do this because we don't have an i686 kernel
BuildRequires:  kernel-modules-extra
%endif
```

For EPEL branches, you can conditionalize on `0%{?rhel}`



---

APPROVED with the above change to drop ix86.

The Pagure repository was created at https://src.fedoraproject.org/rpms/accel-ppp

FEDORA-2024-12cd734e6f (accel-ppp-1.13.0-3.fc42) has been submitted as an update to Fedora 42.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-12cd734e6f

FEDORA-2024-12cd734e6f (accel-ppp-1.13.0-3.fc42) has been pushed to the Fedora 42 stable repository.
If problem still persists, please make note of it in this bug report.

FYI I have a ticket for versioned libraries: https://github.com/accel-ppp/accel-ppp/issues/200