Bug 1764813 (apt, apt-dpkg) - Review Request: apt - Command-line package manager for Debian packages
Summary: Review Request: apt - Command-line package manager for Debian packages
Keywords:
Status: CLOSED RAWHIDE
Alias: apt, apt-dpkg
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Sergio Basto
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1678632 1763931 (view as bug list)
Depends On: 1762976 1764799
Blocks: sbuild 1782555
TreeView+ depends on / blocked
 
Reported: 2019-10-23 19:26 UTC by Neal Gompa
Modified: 2019-12-17 03:41 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-12-17 03:26:55 UTC
Type: ---
Embargoed:
sergio: fedora-review+


Attachments (Terms of Use)

Description Neal Gompa 2019-10-23 19:26:33 UTC
Spec URL: https://ngompa.fedorapeople.org/for-review/apt.spec
SRPM URL: https://ngompa.fedorapeople.org/for-review/apt-1.9.4-1.fc30.src.rpm

Description:
This package provides commandline tools for searching and
managing as well as querying information about packages
as a low-level access to all features of the libapt-pkg library.

These include:
  * apt-get for retrieval of packages and information about them
    from authenticated sources and for installation, upgrade and
    removal of packages together with their dependencies
  * apt-cache for querying available information about installed
    as well as installable packages
  * apt-cdrom to use removable media as a source for packages
  * apt-config as an interface to the configuration settings
  * apt-key as an interface to manage authentication keys


Fedora Account System Username: ngompa

Comment 1 Neal Gompa 2019-10-23 19:28:02 UTC
*** Bug 1678632 has been marked as a duplicate of this bug. ***

Comment 2 Neal Gompa 2019-10-28 13:56:50 UTC
*** Bug 1763931 has been marked as a duplicate of this bug. ***

Comment 3 Sergio Basto 2019-11-14 05:45:48 UTC
Hello , 
I take this review 

 - BuildRequires:  gcc , is not needed we already got gcc-c++ 

 - no need to Obsolete apt-transport-curl-https and apt-transport-curl-https, we never had these packages. Provides can eventually be useful but I'd put it in spec. 

 - non-standard-uid and non-standard-gid _apt , I suggest use apt 

 - IMHO I'd remove devel-doc and put all in doc .

please add in %install 

install -d %{buildroot}%{_sysconfdir}/apt/{apt.conf,preferences,sources.list,trusted.gpg}.d
install -m644 doc/examples/apt.conf %{buildroot}%{_sysconfdir}/apt/
install -m644 %{_target_platform}/vendor/sources.list %{buildroot}%{_sysconfdir}/apt/sources.list
install -d %{buildroot}%{_sysconfdir}/logrotate.d
cat > %{buildroot}%{_sysconfdir}/logrotate.d/apt <<EOF
/var/log/apt/term.log {
  rotate 12
  monthly
  compress
  missingok
  notifempty
}
/var/log/apt/history.log {
  rotate 12
  monthly
  compress
  missingok
  notifempty
}
EOF

in %files 
%dir %{_sysconfdir}/apt/apt.conf.d
%dir %{_sysconfdir}/apt/preferences.d
%dir %{_sysconfdir}/apt/sources.list.d
%dir %{_sysconfdir}/apt/trusted.gpg.d
%config(noreplace) %{_sysconfdir}/apt/apt.conf
%config(noreplace) %{_sysconfdir}/apt/sources.list
%config(noreplace) %{_sysconfdir}/logrotate.d/apt



 - Note: warning: File listed twice: /usr/share/doc/apt , I notice that we have "%doc %{_docdir}/%{name}" no need use %doc tag , %doc is to copy from BUILD to BUILDROOT or maybe you want use %docdir ... 


The following I need to recheck tomorrow :

 [ ]: Package requires other packages for directories it uses.
      Note: No known owner of /usr/libexec/dpkg
 [ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/libexec/dpkg/methods,
     /usr/libexec/dpkg



These are all minor details , most important is user _apt.

Thanks

Comment 4 Neal Gompa 2019-11-15 00:37:18 UTC
(In reply to Sergio Monteiro Basto from comment #3)
> Hello , 
> I take this review 
> 
>  - BuildRequires:  gcc , is not needed we already got gcc-c++ 
> 

I prefer to be explicit here.

>  - no need to Obsolete apt-transport-curl-https and
> apt-transport-curl-https, we never had these packages. Provides can
> eventually be useful but I'd put it in spec. 
> 

I had them before, but I guess I can drop the Obsoletes...

>  - non-standard-uid and non-standard-gid _apt , I suggest use apt 
> 

This name is hard-coded in the apt codebase. I _have_ to use that.

>  - IMHO I'd remove devel-doc and put all in doc .
> 

I'm keeping them separate because they are different types of documentation.

> please add in %install 
> 
> install -d
> %{buildroot}%{_sysconfdir}/apt/{apt.conf,preferences,sources.list,trusted.
> gpg}.d
> install -m644 doc/examples/apt.conf %{buildroot}%{_sysconfdir}/apt/
> install -m644 %{_target_platform}/vendor/sources.list
> %{buildroot}%{_sysconfdir}/apt/sources.list
> install -d %{buildroot}%{_sysconfdir}/logrotate.d
> cat > %{buildroot}%{_sysconfdir}/logrotate.d/apt <<EOF
> /var/log/apt/term.log {
>   rotate 12
>   monthly
>   compress
>   missingok
>   notifempty
> }
> /var/log/apt/history.log {
>   rotate 12
>   monthly
>   compress
>   missingok
>   notifempty
> }
> EOF
> 
> in %files 
> %dir %{_sysconfdir}/apt/apt.conf.d
> %dir %{_sysconfdir}/apt/preferences.d
> %dir %{_sysconfdir}/apt/sources.list.d
> %dir %{_sysconfdir}/apt/trusted.gpg.d
> %config(noreplace) %{_sysconfdir}/apt/apt.conf
> %config(noreplace) %{_sysconfdir}/apt/sources.list
> %config(noreplace) %{_sysconfdir}/logrotate.d/apt
> 
> 

Will do, working on that...


> 
>  - Note: warning: File listed twice: /usr/share/doc/apt , I notice that we
> have "%doc %{_docdir}/%{name}" no need use %doc tag , %doc is to copy from
> BUILD to BUILDROOT or maybe you want use %docdir ... 
> 

I was doing that so that I don't need to list all the things inside there...

Comment 5 Sergio Basto 2019-11-15 00:57:55 UTC
(In reply to Neal Gompa from comment #4)

(...)
OK

> >  - IMHO I'd remove devel-doc and put all in doc .
> > 
> 
> I'm keeping them separate because they are different types of documentation.

In my point of view we should avoid have many packages, if user want install docs they have one package with all docs, samples and examples etc , doesn't make sense IMHO have several doc / samples / example packages ... 
 
> > please add in %install 
> > 
> > install -d
> > %{buildroot}%{_sysconfdir}/apt/{apt.conf,preferences,sources.list,trusted.
> > gpg}.d
> > install -m644 doc/examples/apt.conf %{buildroot}%{_sysconfdir}/apt/
> > install -m644 %{_target_platform}/vendor/sources.list
> > %{buildroot}%{_sysconfdir}/apt/sources.list
> > install -d %{buildroot}%{_sysconfdir}/logrotate.d
> > cat > %{buildroot}%{_sysconfdir}/logrotate.d/apt <<EOF
> > /var/log/apt/term.log {
> >   rotate 12
> >   monthly
> >   compress
> >   missingok
> >   notifempty
> > }
> > /var/log/apt/history.log {
> >   rotate 12
> >   monthly
> >   compress
> >   missingok
> >   notifempty
> > }
> > EOF
> > 
> > in %files 
> > %dir %{_sysconfdir}/apt/apt.conf.d
> > %dir %{_sysconfdir}/apt/preferences.d
> > %dir %{_sysconfdir}/apt/sources.list.d
> > %dir %{_sysconfdir}/apt/trusted.gpg.d
> > %config(noreplace) %{_sysconfdir}/apt/apt.conf
> > %config(noreplace) %{_sysconfdir}/apt/sources.list
> > %config(noreplace) %{_sysconfdir}/logrotate.d/apt
> > 
> > 
> 
> Will do, working on that...

Cool thanks

The package is almost ready to be approved !

Comment 6 Neal Gompa 2019-11-17 20:08:31 UTC
(In reply to Sergio Monteiro Basto from comment #5)
> (In reply to Neal Gompa from comment #4)
> 
> (...)
> OK
> 
> > >  - IMHO I'd remove devel-doc and put all in doc .
> > > 
> > 
> > I'm keeping them separate because they are different types of documentation.
> 
> In my point of view we should avoid have many packages, if user want install
> docs they have one package with all docs, samples and examples etc , doesn't
> make sense IMHO have several doc / samples / example packages ... 
>  
> > > please add in %install 
> > > 
> > > install -d
> > > %{buildroot}%{_sysconfdir}/apt/{apt.conf,preferences,sources.list,trusted.
> > > gpg}.d
> > > install -m644 doc/examples/apt.conf %{buildroot}%{_sysconfdir}/apt/
> > > install -m644 %{_target_platform}/vendor/sources.list
> > > %{buildroot}%{_sysconfdir}/apt/sources.list
> > > install -d %{buildroot}%{_sysconfdir}/logrotate.d
> > > cat > %{buildroot}%{_sysconfdir}/logrotate.d/apt <<EOF
> > > /var/log/apt/term.log {
> > >   rotate 12
> > >   monthly
> > >   compress
> > >   missingok
> > >   notifempty
> > > }
> > > /var/log/apt/history.log {
> > >   rotate 12
> > >   monthly
> > >   compress
> > >   missingok
> > >   notifempty
> > > }
> > > EOF
> > > 
> > > in %files 
> > > %dir %{_sysconfdir}/apt/apt.conf.d
> > > %dir %{_sysconfdir}/apt/preferences.d
> > > %dir %{_sysconfdir}/apt/sources.list.d
> > > %dir %{_sysconfdir}/apt/trusted.gpg.d
> > > %config(noreplace) %{_sysconfdir}/apt/apt.conf
> > > %config(noreplace) %{_sysconfdir}/apt/sources.list
> > > %config(noreplace) %{_sysconfdir}/logrotate.d/apt
> > > 
> > > 
> > 
> > Will do, working on that...
> 
> Cool thanks
> 
> The package is almost ready to be approved !

I've done the appropriate changes...

Spec URL: https://ngompa.fedorapeople.org/for-review/apt.spec
SRPM URL: https://ngompa.fedorapeople.org/for-review/apt-1.9.4-1.fc30.src.rpm

Comment 7 Sergio Basto 2019-11-19 04:34:58 UTC
- warning: File listed twice: /usr/share/doc/apt

- you preferred 
touch %{buildroot}%{_sysconfdir}/apt/sources.list 
instead  
install -m644 %{_target_platform}/vendor/sources.list %{buildroot}%{_sysconfdir}/apt/sources.list

- also we still have 2 doc packages .

All minor details 

Package APPROVED

Comment 8 Neal Gompa 2019-11-19 11:28:40 UTC
(In reply to Sergio Monteiro Basto from comment #7)
> - warning: File listed twice: /usr/share/doc/apt
> 
> - you preferred 
> touch %{buildroot}%{_sysconfdir}/apt/sources.list 
> instead  
> install -m644 %{_target_platform}/vendor/sources.list
> %{buildroot}%{_sysconfdir}/apt/sources.list
> 
> - also we still have 2 doc packages .
> 
> All minor details 
> 
> Package APPROVED

Yeah, I elected to not install any repo configuration because I don't have apt configured with Debian repos out of the gate. If someone wants to accidentally break their computer, I'm not going to provide the necessary footgun. They'll have to work for it. :)

Comment 9 Neal Gompa 2019-11-19 11:29:06 UTC
Err, I mean I don't want apt configured with Debian repos out of the gate.

Comment 10 Dridi Boukelmoune 2019-11-24 15:18:22 UTC
Apologies for being late to the game but I have some remarks.

We should avoid listing %{_libdir}/*.so.* as this might lead to unattended sonames bumps.

Also as per the guidelines I think we should follow upstream (sub)packaging as prior art:

- apt-libs: split into libapt-pkg and libapt-inst
- libapt-pkg: make it provide libapt-pkg%{soname}
- libapt-inst: make it provide libapt-inst%{soname}
- apt-devel: rename to libapt-pkg-devel
- apt-devel-doc: rename to libapt-pkg-doc

I'm very much in favor of not having the dummy apt-transport-https package at all, and only provide it in the main apt package.

See the upstream source package for reference: https://packages.debian.org/source/buster/apt

Other than that, thanks for taking care of the re-review process!

Comment 11 Neal Gompa 2019-11-24 21:35:16 UTC
(In reply to Dridi Boukelmoune from comment #10)
> Apologies for being late to the game but I have some remarks.
> 
> We should avoid listing %{_libdir}/*.so.* as this might lead to unattended
> sonames bumps.
> 

The library interface isn't stable, but sure, I can change this to track the soversion...

> Also as per the guidelines I think we should follow upstream (sub)packaging
> as prior art:
> 
> - apt-libs: split into libapt-pkg and libapt-inst

The splitting is unnecessary. Moreover, libapt-inst is gone now.

> - libapt-pkg: make it provide libapt-pkg%{soname}

What would be the point? I can do it, but the generated library Provides are what we usually rely on for this...

> - libapt-inst: make it provide libapt-inst%{soname}

This library is gone.

> - apt-devel: rename to libapt-pkg-devel
> - apt-devel-doc: rename to libapt-pkg-doc
> 

I'm not going to do this. People should not rely on package names like this when building against it. Instead, they should rely on pkgconfig() / cmake() Provides that RPM generates. That said, I already include those names as Provides for the subpackages in question.

> I'm very much in favor of not having the dummy apt-transport-https package
> at all, and only provide it in the main apt package.
> 

I'm currently adding the virtual Provides for it in the main package. Keep in mind I've been tracking changes in upstream for three years now. My spec file went through all the transitions related to that transport plugin.

> See the upstream source package for reference:
> https://packages.debian.org/source/buster/apt
> 

I've been following what's been going on in salsa.d.o, so I'm aware of what has been changing there.

Comment 12 Sergio Basto 2019-11-25 02:46:17 UTC
(In reply to Neal Gompa from comment #11)
> (In reply to Dridi Boukelmoune from comment #10)
> >
> > Also as per the guidelines I think we should follow upstream (sub)packaging
> > as prior art:
> > 
> > - apt-libs: split into libapt-pkg and libapt-inst

We don't need follow Debian packing way in fedora we don't use libfoo we use foo-libs , specially if main program is a executable not a library.


> The splitting is unnecessary. Moreover, libapt-inst is gone now.
> 
> > - libapt-pkg: make it provide libapt-pkg%{soname}
> 
> What would be the point? I can do it, but the generated library Provides are
> what we usually rely on for this...
> 
> > - libapt-inst: make it provide libapt-inst%{soname}
> 
> This library is gone.
> 
> > - apt-devel: rename to libapt-pkg-devel
> > - apt-devel-doc: rename to libapt-pkg-doc
> > 
> 
> I'm not going to do this. People should not rely on package names like this
> when building against it. Instead, they should rely on pkgconfig() / cmake()
> Provides that RPM generates. That said, I already include those names as
> Provides for the subpackages in question.

libapt-pkg-devel, libapt-pkg-doc is debian names , apt-devel and apt-devel-doc are the correct name 

Also URL https://tracker.debian.org/pkg/apt is must better than https://packages.debian.org/source/buster/apt 


By packing guidelines the old apt should be retired and should be already retired a long time ago and package should be orphan. 
After we should do package review as we did here and request the package again a simple releng ticket. 

The package review was approve and is ready to be imported.

Also fedora-package-config-apt should be retired and not be a zombie on stable releases.

In my opinion , we should avoid splitting packages in many sub-packages without a good reason

Comment 13 Jun Aruga 2019-11-29 23:04:33 UTC
Hi. Just from my curiosity. There is already "rpms/apt" here. Why are you reviewing new (?) "apt" rather than updating current "apt"?
https://src.fedoraproject.org/rpms/apt

Comment 14 Neal Gompa 2019-11-29 23:23:14 UTC
(In reply to Jun Aruga from comment #13)
> Hi. Just from my curiosity. There is already "rpms/apt" here. Why are you
> reviewing new (?) "apt" rather than updating current "apt"?
> https://src.fedoraproject.org/rpms/apt

Dridi insisted on the re-review. I'll be updating the `rpms/apt` package after FESCo approves the Change: https://fedoraproject.org/wiki/Changes/Move_apt_package_from_RPM_to_DPKG_backend

Comment 15 Jun Aruga 2019-11-29 23:49:48 UTC
I see. Thanks for the response.


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