Bug 1754964 - Review Request: breezy - Friendly distributed version control system
Summary: Review Request: breezy - Friendly distributed version control system
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1753195 1753633
Blocks: 1737937
TreeView+ depends on / blocked
 
Reported: 2019-09-24 12:52 UTC by Miro Hrončok
Modified: 2019-10-26 17:24 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2019-10-26 17:24:15 UTC
Type: ---
Embargoed:
ngompa13: fedora-review+


Attachments (Terms of Use)

Description Miro Hrončok 2019-09-24 12:52:19 UTC
Spec URL: https://churchyard.fedorapeople.org/SRPMS/breezy.spec
SRPM URL: https://churchyard.fedorapeople.org/SRPMS/breezy-3.0.1-1.fc30.src.rpm

Description:
Breezy (brz) is a decentralized revision control system, designed to be easy
for developers and end users alike.

By default, Breezy provides support for both the Bazaar and Git file formats.

Fedora Account System Username: churchyard

Comment 1 Miro Hrončok 2019-09-24 12:54:59 UTC
WARNING:

Fetching revisions doesn't work with Python 3.8, reported upstream: https://bugs.launchpad.net/brz/+bug/1844684


I do not plan to import this into Fedora before this is fixed, as it renders the package rather useless.
This package obsoletes bzr, doing that from a broken package is rather unfair.

However, this can be reviewed for all the other aspects of packaging.

Comment 2 Neal Gompa 2019-09-26 21:27:26 UTC
Taking this review.

Comment 3 Neal Gompa 2019-09-26 21:30:21 UTC
Adding external bug reference blocking review acceptance.

Comment 4 Neal Gompa 2019-09-26 21:37:21 UTC
Notes through initial pass of review:

> %global _pkgdocdir %{_docdir}/%{name}-%{version}

Why are we overriding pkgdocdir this way?

> Suggests:       %{name} = %{version}-%{release}

This should be using "Enhances" instead of "Suggests", as this is a reverse relationship.

> ln -s brz.1 %{buildroot}%{_mandir}/man1/bzr.1

This doesn't do *exactly* the thing you expect. Do something like this:

> # Link bzr(1) to brz(1)
> echo ".so man1/brz.1" > %{buildroot}%{_mandir}/man1/bzr.1

This ensures man-db does the correct thing for references.

> %doc %{_pkgdocdir}/COPYING.txt

Please relocate the license properly and install it via "%license"

Comment 5 Miro Hrončok 2019-09-26 21:45:15 UTC
(In reply to Neal Gompa from comment #4)
> Notes through initial pass of review:
> 
> > %global _pkgdocdir %{_docdir}/%{name}-%{version}
> 
> Why are we overriding pkgdocdir this way?

Copypasta from bzr, will drop it.


> > Suggests:       %{name} = %{version}-%{release}
> 
> This should be using "Enhances" instead of "Suggests", as this is a reverse
> relationship.

IIRC we don't do reverse dependencies. If I wanted breezy to suggest the docs, I'd do that directly.
This was adapted from bzr, where the docs required bzr (and I found that too strong). In fact, it can go away totally.


> > ln -s brz.1 %{buildroot}%{_mandir}/man1/bzr.1
> 
> This doesn't do *exactly* the thing you expect. Do something like this:
> 
> > # Link bzr(1) to brz(1)
> > echo ".so man1/brz.1" > %{buildroot}%{_mandir}/man1/bzr.1
> 
> This ensures man-db does the correct thing for references.

Could you please describe the difference? It sound interesting, I've always just linked the manual page (I remember it was broken before due to the way the Builrtoot Policy handled compression, but that works fine now).



> > %doc %{_pkgdocdir}/COPYING.txt
> 
> Please relocate the license properly and install it via "%license"

Sure thing. Again, a (wrong) copypasta from bzr.

Comment 6 Neal Gompa 2019-09-26 22:10:03 UTC
(In reply to Miro Hrončok from comment #5)
> (In reply to Neal Gompa from comment #4)
> > Notes through initial pass of review:
> > 
> > > %global _pkgdocdir %{_docdir}/%{name}-%{version}
> > 
> > Why are we overriding pkgdocdir this way?
> 
> Copypasta from bzr, will drop it.
> 
> 

Okay, sounds good!

> > > Suggests:       %{name} = %{version}-%{release}
> > 
> > This should be using "Enhances" instead of "Suggests", as this is a reverse
> > relationship.
> 
> IIRC we don't do reverse dependencies. If I wanted breezy to suggest the
> docs, I'd do that directly.
> This was adapted from bzr, where the docs required bzr (and I found that too
> strong). In fact, it can go away totally.
> 

That's fine with me as well. But we do in fact do reverse dependencies all the time. Supplements and Enhances have been allowed and used for a couple of years now. In fact, we used them first before forward weak dependencies, because YUM ignores them rather than breaking on them.

Enhances is the correct relationship for this, and for things that use Suggests and Enhances for helping users discover complementary packages, it's quite helpful as long as the relationships are correct.

> 
> > > ln -s brz.1 %{buildroot}%{_mandir}/man1/bzr.1
> > 
> > This doesn't do *exactly* the thing you expect. Do something like this:
> > 
> > > # Link bzr(1) to brz(1)
> > > echo ".so man1/brz.1" > %{buildroot}%{_mandir}/man1/bzr.1
> > 
> > This ensures man-db does the correct thing for references.
> 
> Could you please describe the difference? It sound interesting, I've always
> just linked the manual page (I remember it was broken before due to the way
> the Builrtoot Policy handled compression, but that works fine now).
> 
> 

If you do a "man 1 bzr" with this, man's cached references will pull brz(1). It also allows manpage indexers to Do The Right Thing(TM) and re-point references.

For example: https://www.mankier.com/1/pkg-config will actually redirect to https://www.mankier.com/1/pkgconf instead of just being a copy.

There's a few other benefits, mostly related to the web of man page dependencies and how they're handled by man page parsers.

I don't know why this isn't documented or made into a macro, but it probably should be. It's a very useful trick!

> 
> > > %doc %{_pkgdocdir}/COPYING.txt
> > 
> > Please relocate the license properly and install it via "%license"
> 
> Sure thing. Again, a (wrong) copypasta from bzr.

Okay, sounds good!

Comment 7 Neal Gompa 2019-09-26 22:16:29 UTC
(In reply to Neal Gompa from comment #6)
> (In reply to Miro Hrončok from comment #5)
> > (In reply to Neal Gompa from comment #4)
> > > > ln -s brz.1 %{buildroot}%{_mandir}/man1/bzr.1
> > > 
> > > This doesn't do *exactly* the thing you expect. Do something like this:
> > > 
> > > > # Link bzr(1) to brz(1)
> > > > echo ".so man1/brz.1" > %{buildroot}%{_mandir}/man1/bzr.1
> > > 
> > > This ensures man-db does the correct thing for references.
> > 
> > Could you please describe the difference? It sound interesting, I've always
> > just linked the manual page (I remember it was broken before due to the way
> > the Builrtoot Policy handled compression, but that works fine now).
> > 
> > 
> 
> If you do a "man 1 bzr" with this, man's cached references will pull brz(1).
> It also allows manpage indexers to Do The Right Thing(TM) and re-point
> references.
> 
> For example: https://www.mankier.com/1/pkg-config will actually redirect to
> https://www.mankier.com/1/pkgconf instead of just being a copy.
> 
> There's a few other benefits, mostly related to the web of man page
> dependencies and how they're handled by man page parsers.
> 
> I don't know why this isn't documented or made into a macro, but it probably
> should be. It's a very useful trick!
> 

To note, I discovered this trick a while ago and used it in the pkgconf package: https://src.fedoraproject.org/rpms/pkgconf/c/d7c5865721f8c1fa8e8a67fa663416ac1ec0f215

Comment 8 Miro Hrončok 2019-09-26 22:21:38 UTC
(In reply to Neal Gompa from comment #6)
> > > > ln -s brz.1 %{buildroot}%{_mandir}/man1/bzr.1
> > > 
> > > This doesn't do *exactly* the thing you expect. Do something like this:
> > > 
> > > > # Link bzr(1) to brz(1)
> > > > echo ".so man1/brz.1" > %{buildroot}%{_mandir}/man1/bzr.1
> > > 
> > > This ensures man-db does the correct thing for references.
> > 
> > Could you please describe the difference? It sound interesting, I've always
> > just linked the manual page (I remember it was broken before due to the way
> > the Builrtoot Policy handled compression, but that works fine now).
> > 
> > 
> 
> If you do a "man 1 bzr" with this, man's cached references will pull brz(1).

I don't understands the difference here, sorry.

When I link it via "ln -s", what will "man 1 bzr" do?
When I link it via ".so man1/brz.1", what will "man 1 bzr" do?


> It also allows manpage indexers to Do The Right Thing(TM) and re-point
> references.
> 
> For example: https://www.mankier.com/1/pkg-config will actually redirect to
> https://www.mankier.com/1/pkgconf instead of just being a copy.

python3.spec has:

ln -s ./python3.1 %{buildroot}%{_mandir}/man1/python.1

But https://www.mankier.com/1/python redirects to https://www.mankier.com/1/python3


> There's a few other benefits, mostly related to the web of man page
> dependencies and how they're handled by man page parsers.
> 
> I don't know why this isn't documented or made into a macro, but it probably
> should be. It's a very useful trick!

This should be part of the guidelines, if it really matters (although I am not quite understand that yet).

Comment 9 Miro Hrončok 2019-09-27 08:54:09 UTC
--- breezy.spec.orig	2019-09-27 10:28:16.923615896 +0200
+++ breezy.spec	2019-09-27 10:48:05.020867674 +0200
@@ -9,8 +9,6 @@
 #global brzrc b6
 %global baserelease 1
 
-%global _pkgdocdir %{_docdir}/%{name}-%{version}
-
 Name:           breezy
 Version:        %{brzmajor}%{?brzminor}
 Release:        %{baserelease}%{?brzrc:.}%{?brzrc}%{?dist}
@@ -47,6 +45,9 @@
 # This is needed for launchpad support
 Recommends:     python3-launchpadlib
 
+# Docs are not needed, but some might want them
+Suggests:       %{name}-doc = %{version}-%{release}
+
 %description
 Breezy (brz) is a decentralized revision control system, designed to be easy
 for developers and end users alike.
@@ -57,7 +58,6 @@
 %package doc
 Summary:        Documentation for Breezy
 BuildArch:      noarch
-Suggests:       %{name} = %{version}-%{release}
 
 %description doc
 This package contains the documentation for the Breezy version control system.
@@ -85,6 +85,13 @@
 
 # Build documents
 make docs-sphinx PYTHON=%{__python3}
+rm doc/*/_build/html/.buildinfo
+rm -f doc/*/_build/html/_static/*/Makefile
+pushd doc
+for dir in *; do
+  test -d $dir/_build/html && mv $dir/_build/html ../$dir
+done
+popd
 
 
 %install
@@ -99,24 +106,6 @@
 install -Dpm 0644 contrib/bash/brz %{buildroot}$bashcompdir/brz
 rm contrib/bash/brz
 
-# Install documents
-install -d %{buildroot}/%{_pkgdocdir}/pdf
-cp -pr NEWS README.rst TODO COPYING.txt contrib/ %{buildroot}/%{_pkgdocdir}/
-cd doc
-for dir in *; do
-    if [ -d $dir/_build/html ]; then
-        cp -R $dir/_build/html %{buildroot}%{_pkgdocdir}/$dir
-        rm -f %{buildroot}%{_pkgdocdir}/$dir/.buildinfo 
-        rm -f %{buildroot}%{_pkgdocdir}/$dir/_static/$dir/Makefile
-        pushd %{buildroot}
-        find .%{_pkgdocdir}/$dir -name '*.pdf' | while read pdf; do
-            ln -s $pdf %{buildroot}%{_pkgdocdir}/pdf/
-        done
-        popd
-    fi
-done
-cd ..
-
 install -d %{buildroot}%{_datadir}/pixmaps
 install -m 0644 %{SOURCE2} %{buildroot}%{_datadir}/pixmaps/brz.png
 
@@ -124,8 +113,8 @@
 mv %{buildroot}%{_prefix}/man %{buildroot}%{_datadir}
 
 # backwards compatible symbolic links
-ln -s brz.1 %{buildroot}%{_mandir}/man1/bzr.1
 ln -s brz %{buildroot}%{_bindir}/bzr
+echo ".so man1/brz.1" > %{buildroot}%{_mandir}/man1/bzr.1
 
 # locales: all the .po files have empty msgstrs, so this doesn't do anything
 #mv %%{name}/locale %%{buildroot}%%{_datadir}
@@ -134,12 +123,8 @@
 
 %files
 # ... -f %%{name}.lang
-%dir %{_pkgdocdir}
-%doc %{_pkgdocdir}/NEWS
-%doc %{_pkgdocdir}/README.rst
-%doc %{_pkgdocdir}/TODO
-%doc %{_pkgdocdir}/COPYING.txt
-%doc %{_pkgdocdir}/contrib/
+%license COPYING.txt
+%doc NEWS README.rst TODO contrib/
 %{_bindir}/brz
 %{_bindir}/bzr
 %{_bindir}/bzr-*-pack
@@ -152,13 +137,8 @@
 
 
 %files doc
-%dir %{_pkgdocdir}
-%doc %{_pkgdocdir}/*
-%exclude %{_pkgdocdir}/NEWS
-%exclude %{_pkgdocdir}/README.rst
-%exclude %{_pkgdocdir}/TODO
-%exclude %{_pkgdocdir}/COPYING.txt
-%exclude %{_pkgdocdir}/contrib/
+%license COPYING.txt
+%doc en developers
 
 
 %changelog


Spec URL: https://churchyard.fedorapeople.org/SRPMS/breezy.spec
SRPM URL: https://churchyard.fedorapeople.org/SRPMS/breezy-3.0.1-1.fc30.src.rpm

Comment 10 Neal Gompa 2019-10-07 00:29:52 UTC
(In reply to Miro Hrončok from comment #8)
> (In reply to Neal Gompa from comment #6)
> > > > > ln -s brz.1 %{buildroot}%{_mandir}/man1/bzr.1
> > > > 
> > > > This doesn't do *exactly* the thing you expect. Do something like this:
> > > > 
> > > > > # Link bzr(1) to brz(1)
> > > > > echo ".so man1/brz.1" > %{buildroot}%{_mandir}/man1/bzr.1
> > > > 
> > > > This ensures man-db does the correct thing for references.
> > > 
> > > Could you please describe the difference? It sound interesting, I've always
> > > just linked the manual page (I remember it was broken before due to the way
> > > the Builrtoot Policy handled compression, but that works fine now).
> > > 
> > > 
> > 
> > If you do a "man 1 bzr" with this, man's cached references will pull brz(1).
> 
> I don't understands the difference here, sorry.
> 
> When I link it via "ln -s", what will "man 1 bzr" do?
> When I link it via ".so man1/brz.1", what will "man 1 bzr" do?
> 
> 
> > It also allows manpage indexers to Do The Right Thing(TM) and re-point
> > references.
> > 
> > For example: https://www.mankier.com/1/pkg-config will actually redirect to
> > https://www.mankier.com/1/pkgconf instead of just being a copy.
> 
> python3.spec has:
> 
> ln -s ./python3.1 %{buildroot}%{_mandir}/man1/python.1
> 
> But https://www.mankier.com/1/python redirects to
> https://www.mankier.com/1/python3
> 
> 
> > There's a few other benefits, mostly related to the web of man page
> > dependencies and how they're handled by man page parsers.
> > 
> > I don't know why this isn't documented or made into a macro, but it probably
> > should be. It's a very useful trick!
> 
> This should be part of the guidelines, if it really matters (although I am
> not quite understand that yet).

The symlink trick used to not work, I guess presumably because of the auto-compression thing freaking out on symlinks.

Comment 11 Miro Hrončok 2019-10-07 09:23:15 UTC
(In reply to Neal Gompa from comment #10)
> The symlink trick used to not work, I guess presumably because of the
> auto-compression thing freaking out on symlinks.

Yes. Yes.


Anyway, bzr cannot be installed any longer, so approving this doesn't make it worse.

Comment 12 Neal Gompa 2019-10-07 11:29:02 UTC
Yeah. The packaging looks good from my perspective.

PACKAGE APPROVED.

Comment 13 Gwyn Ciesla 2019-10-08 13:26:51 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/breezy

Comment 15 Fedora Update System 2019-10-09 16:17:21 UTC
FEDORA-2019-7fb253a20a has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2019-7fb253a20a

Comment 16 Fedora Update System 2019-10-09 23:05:51 UTC
breezy-3.0.1-1.fc31, python-launchpadlib-1.10.7-1.fc31, python-lazr-restfulclient-0.14.2-1.fc31, python-lazr-uri-1.0.3-1.fc31, python-sphinx-epytext-0.0.4-1.fc31, python-wadllib-1.3.3-1.fc31 has been pushed to the Fedora 31 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-7fb253a20a

Comment 17 Miro Hrončok 2019-10-10 09:37:47 UTC
Followup in bz1760260.

Comment 18 Fedora Update System 2019-10-26 17:24:15 UTC
breezy-3.0.1-1.fc31, python-launchpadlib-1.10.7-1.fc31, python-lazr-restfulclient-0.14.2-1.fc31, python-lazr-uri-1.0.3-1.fc31, python-sphinx-epytext-0.0.4-1.fc31, python-wadllib-1.3.3-1.fc31 has been pushed to the Fedora 31 stable repository. If problems still persist, please make note of it in this bug report.


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