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
*** Bug 1678632 has been marked as a duplicate of this bug. ***
*** Bug 1763931 has been marked as a duplicate of this bug. ***
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
(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...
(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 !
(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
- 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
(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. :)
Err, I mean I don't want apt configured with Debian repos out of the gate.
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!
(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.
(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
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
(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
I see. Thanks for the response.
This is now done and pushed into Rawhide: * Git: https://src.fedoraproject.org/rpms/apt/c/2d596d650c7ffb3dc1d38c2ab61763f0c946513b * Koji: https://koji.fedoraproject.org/koji/buildinfo?buildID=1423017