https://bugzilla.redhat.com/show_bug.cgi?id=2117046
Bug ID: 2117046 Summary: Review Request: pilotlog - A pilot logbook for logging flight time Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: rpm@stansoft.org QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://sourceforge.net/projects/pilotlog/files/SRPMS/pilotlog.spec/download SRPM URL: https://sourceforge.net/projects/pilotlog/files/SRPMS/pilotlog-9-1.fc36.src.... Description: Pilot Log is a pilot logbook for logging flight time and calculating aircraft weight & balance. Fedora Account System Username: rpm@stansoft.org
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
Stansoft rpm@stansoft.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://sourceforge.net/projects/pilotlog/files/SRPMS/pilotlog.spec/download SRPM URL: https://sourceforge.net/projects/pilotlog/files/SRPMS/pilotlog-9-1.fc36.src.... Description: Pilot Log is a pilot logbook for logging flight time and calculating aircraft weight & balance. Fedora Account System Username: stansoft
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
Jonathan Wright jonathan@almalinux.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value CC| |jonathan@almalinux.org
--- Comment #1 from Jonathan Wright jonathan@almalinux.org --- Spec URL and SRPM URL need to be direct links so fedora-review can download them.
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
Jonathan Wright jonathan@almalinux.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
--- Comment #2 from Jonathan Wright jonathan@almalinux.org --- Are you a Fedora packager? Your FAS name isn't in the packager group.
https://accounts.fedoraproject.org/user/stansoft/
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=2117046
--- Comment #3 from Stansoft rpm@stansoft.org --- Spec URL: https://downloads.sourceforge.net/project/pilotlog/SRPMS/pilotlog.spec?ts=gA... SRPM URL: https://downloads.sourceforge.net/project/pilotlog/SRPMS/pilotlog-9-1.fc36.s...
I am not a Fedora packager so I will need a sponsor.
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
--- Comment #4 from Jonathan Wright jonathan@almalinux.org --- Those URLs still aren't going to work. The URLs need to be a direct link to the files without a landing page, etc.
I've flagged your review as needing a sponsor but we can still get the review knocked out.
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
--- Comment #5 from Stansoft rpm@stansoft.org --- Spec URL: https://download.stansoft.org/files/pilotlog.spec SRPM URL: https://download.stansoft.org/files/pilotlog-9-1.fc36.src.rpm
Those were suppose to be direct links on sourceforge, but I see they do not work either so use the above links instead.
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
--- Comment #6 from Jonathan Wright jonathan@almalinux.org ---
# Omit /lib/.build-id to avoid conflict with included libs %global _build_id_links none # Filter internal libraries %global _privatelibs ^%{_libdir}/%{name}/.*\.so$ %global __provides_exclude ^(%{_privatelibs})$ %global __requires_exclude ^(%{_privatelibs})$ %global __requires_exclude_from ^(%{_privatelibs})$ %global __filter_GLIBC_PRIVATE 1
Since I can't get the source code I'm not sure what all of this is for but are you packaging libraries and trying to avoid using system libs? You should almost always use system libs. [2][3]
Source0: %{name}-v%{version}-src.tar.bz2
This needs to be a URL. [1]
./configure make
You should use macros %configure and %make_build. [4]
cp installpl updatepl pl changelog COPYING README \ %{buildroot}%{_libdir}/%{name}
%pre %post %preun %postun
These should be removed.
BuildRequires: systemd-rpm-macros
Is this needed?
You probably don't need changelog, COPYING, and README here. README and changelog should fall under %doc and COPYING under %license. This takes care of copying them into the correct places for you. [5]
1. https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ 2. https://docs.fedoraproject.org/en-US/packaging-guidelines/AutoProvidesAndReq... 3. https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling 4. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_parallel_make 5. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation
There are some other parts of this I'm not sure about at all and we'll have to seek guidance from others.
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
--- Comment #7 from Stansoft rpm@stansoft.org --- (In reply to Jonathan Wright from comment #6)
# Omit /lib/.build-id to avoid conflict with included libs %global _build_id_links none # Filter internal libraries %global _privatelibs ^%{_libdir}/%{name}/.*\.so$ %global __provides_exclude ^(%{_privatelibs})$ %global __requires_exclude ^(%{_privatelibs})$ %global __requires_exclude_from ^(%{_privatelibs})$ %global __filter_GLIBC_PRIVATE 1
Since I can't get the source code I'm not sure what all of this is for but are you packaging libraries and trying to avoid using system libs? You should almost always use system libs. [2][3]
I am packaging Postgresql for version control since the pilotlog program is compiled against it. Is it a requirement that I use the Fedora postgresql package and libs?
Source0: %{name}-v%{version}-src.tar.bz2
This needs to be a URL. [1]
./configure make
%configure works but it does not compile when using %make_build
You should use macros %configure and %make_build. [4]
cp installpl updatepl pl changelog COPYING README \ %{buildroot}%{_libdir}/%{name}
%pre %post %preun %postun
These should be removed.
BuildRequires: systemd-rpm-macros
Is this needed?
You probably don't need changelog, COPYING, and README here. README and changelog should fall under %doc and COPYING under %license. This takes care of copying them into the correct places for you. [5]
https://docs.fedoraproject.org/en-US/packaging-guidelines/ AutoProvidesAndRequiresFiltering/#_summary 3. https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling 4. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_parallel_make 5. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation
There are some other parts of this I'm not sure about at all and we'll have to seek guidance from others.
Here are the updated spec and source rpm
Spec URL: https://download.stansoft.org/files/pilotlog.spec SRPM URL: https://download.stansoft.org/files/pilotlog-9-2.fc36.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
--- Comment #8 from Jonathan Wright jonathan@almalinux.org --- You need a `BuildRequires: gcc`. [1]
Requires(pre): shadow-utils
Is this needed? I removed it and everything compiled fine it seems.
I am packaging Postgresql for version control since the pilotlog program is compiled against it. Is it a requirement that I use the Fedora postgresql package and libs?
Unless there's a really good reason not to, yes. The fact that you're trying to populate a database here is partially what I'm unsure about. I'm not totally sure what the packaging policy is surrounding situations like this since Postgres could be used for other things already running, etc.
%configure works but it does not compile when using %make_build
It doesn't seem to like compiling with multiple threads. You can use `%make_build -j1` to force it to 1 thread but ideally you can figure out why it won't compile using multiple threads. I believe you're upstream/the developer of this aren't you?
How are you test building the RPM? You need to be using mock (or something like `fedpkg --release rawhide mockbuild`).
Why not also use `%make_install` instead of copying everything into place by hand in the spec? I see the Makefile has some installation components in it already.
Requires: ncurses%{?_isa} >= 6.2
This should instead be `BuildRequires: ncurses-devel`. RPM will then automatically generate the proper `Requires` since it builds against it. [2]
I think a bunch of your psql related stuff is failing. I see a ton of this in the build logs:
2022-08-11 18:29:25.788 CDT [570] ERROR: relation "syscolatt" does not exist at character 81 2022-08-11 18:29:25.788 CDT [570] STATEMENT: DECLARE chkscatt_0 CURSOR WITH HOLD FOR select syscolatt.def_format,seqno from syscolatt WHERE tabname='log_entry' AND colname='entry_date' ORDER BY seqno
---
On further looking at the source tarball I notice that Postgres you're providing is already compiled which is definitely forbidden. [3]
---
1. https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/#_buildr... 2. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_package_dependen... 3. https://docs.fedoraproject.org/en-US/packaging-guidelines/what-can-be-packag...
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
--- Comment #9 from Stansoft rpm@stansoft.org --- (In reply to Jonathan Wright from comment #8)
You need a `BuildRequires: gcc`. [1]
Added
Requires(pre): shadow-utils
Is this needed? I removed it and everything compiled fine it seems.
It is not needed, I have removed it.
I am packaging Postgresql for version control since the pilotlog program is compiled against it. Is it a requirement that I use the Fedora postgresql package and libs?
Unless there's a really good reason not to, yes. The fact that you're trying to populate a database here is partially what I'm unsure about. I'm not totally sure what the packaging policy is surrounding situations like this since Postgres could be used for other things already running, etc.
Yes a database needs to be created and populated prior to compiling since the programs need the database schema to compile.
%configure works but it does not compile when using %make_build
It doesn't seem to like compiling with multiple threads. You can use `%make_build -j1` to force it to 1 thread but ideally you can figure out why it won't compile using multiple threads. I believe you're upstream/the developer of this aren't you?
Yes I am the developer. I put `%make_build -j1` in for now, but I will figure out why it doesn't work with multiple threads.
How are you test building the RPM? You need to be using mock (or something like `fedpkg --release rawhide mockbuild`).
rpmbuild --target x86_64 -ba pilotlog.spec
Why not also use `%make_install` instead of copying everything into place by hand in the spec? I see the Makefile has some installation components in it already.
The Makefile copies into /usr/local/pilotlog and I want to use the same Makefile for Fedora packaging that I am using for anyone that downloads the source tarball and compiles.
Requires: ncurses%{?_isa} >= 6.2
This should instead be `BuildRequires: ncurses-devel`. RPM will then automatically generate the proper `Requires` since it builds against it. [2]
I made this change.
I think a bunch of your psql related stuff is failing. I see a ton of this in the build logs:
2022-08-11 18:29:25.788 CDT [570] ERROR: relation "syscolatt" does not exist at character 81 2022-08-11 18:29:25.788 CDT [570] STATEMENT: DECLARE chkscatt_0 CURSOR WITH HOLD FOR select syscolatt.def_format,seqno from syscolatt WHERE tabname='log_entry' AND colname='entry_date' ORDER BY seqno
On further looking at the source tarball I notice that Postgres you're providing is already compiled which is definitely forbidden. [3]
I will make the change to use the Fedora postgresql.
https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ #_buildrequires_and_requires 2. https://docs.fedoraproject.org/en-US/packaging-guidelines/ #_package_dependencies 3. https://docs.fedoraproject.org/en-US/packaging-guidelines/what-can-be- packaged/#prebuilt-binaries-or-libraries
Spec URL: https://download.stansoft.org/files/pilotlog.spec SRPM URL: https://download.stansoft.org/files/pilotlog-9-2.fc36.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
--- Comment #10 from Stansoft rpm@stansoft.org --- Spec URL: https://download.stansoft.org/files/pilotlog.spec SRPM URL: https://download.stansoft.org/files/pilotlog-9-2.fc36.src.rpm
Added %make_install
Product: Fedora Version: rawhide Component: Package Review
Stansoft rpm@stansoft.org has denied Package Review package-review@lists.fedoraproject.org's request for Stansoft rpm@stansoft.org's needinfo: Bug 2117046: Review Request: pilotlog - A pilot logbook for logging flight time https://bugzilla.redhat.com/show_bug.cgi?id=2117046
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
Stansoft rpm@stansoft.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(rpm@stansoft.org) |needinfo- needinfo-
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
Stansoft rpm@stansoft.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo- needinfo- |
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
--- Comment #12 from Stansoft rpm@stansoft.org --- Spec URL: https://download.copr.fedorainfracloud.org/results/stansoft/pilotlog/fedora-... SRPM URL: https://download.copr.fedorainfracloud.org/results/stansoft/pilotlog/fedora-...
unbundled aubit4gl, now using the Fedora repo package
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
Stansoft rpm@stansoft.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841 (FE-NEEDSPONSOR) |
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=2117046
--- Comment #13 from Stansoft rpm@stansoft.org --- Spec URL: https://download.copr.fedorainfracloud.org/results/stansoft/pilotlog/fedora-... SRPM URL: https://download.copr.fedorainfracloud.org/results/stansoft/pilotlog/fedora-...
https://bugzilla.redhat.com/show_bug.cgi?id=2117046
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |http://pilotlog.stansoft.or | |g
--- Comment #14 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6565818 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com