Bug 2117046 - Review Request: pilotlog - A pilot logbook for logging flight time
Summary: Review Request: pilotlog - A pilot logbook for logging flight time
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL: http://pilotlog.stansoft.org
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-08-09 20:23 UTC by Stansoft
Modified: 2023-10-25 23:56 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Stansoft 2022-08-09 20:23:46 UTC
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.rpm/download
Description: Pilot Log is a pilot logbook for logging flight time and calculating aircraft weight & balance.
Fedora Account System Username: stansoft

Comment 1 Jonathan Wright 2022-08-11 18:24:45 UTC
Spec URL and SRPM URL need to be direct links so fedora-review can download them.

Comment 2 Jonathan Wright 2022-08-11 18:32:28 UTC
Are you a Fedora packager?  Your FAS name isn't in the packager group.

https://accounts.fedoraproject.org/user/stansoft/

Comment 4 Jonathan Wright 2022-08-11 21:20:48 UTC
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.

Comment 5 Stansoft 2022-08-11 21:40:12 UTC
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.

Comment 6 Jonathan Wright 2022-08-11 22:01:18 UTC
> # 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/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.

Comment 7 Stansoft 2022-08-11 23:03:00 UTC
(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]
> 
> 1. https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
> 2.
> 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

Comment 8 Jonathan Wright 2022-08-11 23:34:13 UTC
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++/#_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

Comment 9 Stansoft 2022-08-12 00:44:55 UTC
(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.

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

Comment 11 Package Review 2023-08-14 00:45:23 UTC
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.

Comment 14 Fedora Review Service 2023-10-25 23:56:40 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6565818
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2117046-pilotlog/fedora-rawhide-x86_64/06565818-pilotlog/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.


Note You need to log in before you can comment on or make changes to this bug.