Bug 225751 - Merge Review: file-roller
Merge Review: file-roller
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Clasen
Fedora Package Reviews List
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:36 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-10 21:28:10 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bdpepple: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:36:34 EST
Fedora Merge Review: file-roller

http://cvs.fedora.redhat.com/viewcvs/devel/file-roller/
Initial Owner: caillon@redhat.com
Comment 1 Brian Pepple 2007-02-03 13:07:39 EST
Good:
* Package name conforms to the Fedora Naming Guidelines
* Group Tag is from the official list
* Buildroot has all required elements
* All paths begin with macros
* All necessary BuildRequires listed.
* Package builds in Mock.

Must Fix:
* Source URL in not canonical
* Remove unnecessary Requires:
 Requires(post): desktop-file-utils
 Requires(postun): desktop-file-utils
Refer to:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef


Minor:
* Duplicate BuildRequires: glib2-devel (by pango-devel), pango-devel (by
gtk2-devel), gtk2-devel (by libgnomeui-devel), libgnomeprint22-devel (by
libgnomeprintui22-devel), autoconf (by libtool)
* Could use -disable-static and not bother building static libs.
* It looks like the Requires on GConf is unnecessary.
* Is the conflicts on nautilus still necessary?
Comment 2 Matthias Clasen 2007-02-03 13:54:18 EST
FWIW, I disagree with the purging of "Duplicate BuildRequires" that seems to
be proposed. If the configure script explicitly checks for glib2, it is a good
idea to have an explicit BR for glib2-devel, even if pango-devel happens to pull
it in already.
Comment 3 Christopher Aillon 2007-02-03 14:25:05 EST
I also disagree with canonicalizing the Source URL.  It will need to be
regularly updated to stay in sync, at least for GNOME URLs.

Sources are typically "automatically" updated with the expansion of the
%{version} macro, for example.  So, %{name}-%{version}.tar.bz2 for example. 
With GNOME, you see things such as:
http://ftp.gnome.org/pub/gnome/sources/file-roller/ and you need to update the
sourceball's parent directory as well.

So, 2.16.0 would have a URL of
http://ftp.gnome.org/pub/gnome/sources/file-roller/2.16/%{name}-%{version}.tar.bz2

and 2.17.0 would have one of
http://ftp.gnome.org/pub/gnome/sources/file-roller/2.17/%{name}-%{version}.tar.bz2

It's my impression we don't want to force people to do that.
Comment 4 Kevin Fenzi 2007-02-03 14:39:28 EST
Note that you could do something like: 

http://ftp.gnome.org/pub/gnome/sources/file-roller/%{version}/%{name}-%{version}.tar.bz2

If that format always holds true. 
Comment 5 Matthias Clasen 2007-02-03 20:15:14 EST
nope, %{version} expsnds to 2.17.2, but the directory does not include the mico
version number.
Comment 6 Brian Pepple 2007-02-04 09:52:56 EST
(In reply to comment #2)
> FWIW, I disagree with the purging of "Duplicate BuildRequires" that seems to
> be proposed. If the configure script explicitly checks for glib2, it is a good
> idea to have an explicit BR for glib2-devel, even if pango-devel happens to pull
> it in already.

Agreed, the comments in the minor section of comment #1 aren't considered
blockers for the packages approval.  They are merely suggestion of things to
look at.
Comment 7 Christopher Aillon 2007-02-05 15:14:41 EST
I fixed the desktop-file-utils issue, but did not canonicalize the source
because of the reasons I outlined.

--disable-static doesn't exactly disable building of all static objects, which I
filed upstream: http://bugzilla.gnome.org/show_bug.cgi?id=404717

Left the rest alone.
Comment 8 Toshio Kuratomi 2007-02-08 00:18:20 EST
Canonicalizing the sources is a requirement:
- MUST: The sources used to build the package must match the upstream source, as
provided in the spec URL. Reviewers should use md5sum for this task.

The requirement predates the packaging committee but I belive I recall its
purpose being to aid reviewers and possible automated QA scripts in verifying
sources.  Let me know if you consider this important enough for me to bring up
at the next packaging committee meeting.
Comment 9 Christopher Aillon 2007-02-08 12:25:42 EST
Then please bring it up.  The source URL can be derived by going to the website
given in the URL field, and in some cases, notably such as with the mozilla
sources, I get them many times before they are posted publicly (for security
releases) so cannot say with 100% certainty what the actual Source URL will be
at build time for my packages.
Comment 10 Toshio Kuratomi 2007-02-08 21:07:35 EST
I see two issues being expressed here:

1) Convenience.  file-roller will need a one-character change in the spec
approximately four times per year.
2) Factuality.  When we are heavily involved with upstream (ex: Mozilla), there
will be times when we have access to tarballs that aren't yet available in
publicly available trees.

These are the reasons not to include the full URL in the source line.  The
reasons for having the full source URL are to help reviewers, qa people, and
automated scripts find and verify the source.

I asked about this on fedora-packaging and got a limited response where #1 was
seen as not having significant benefits for a change.  #2 was seen as something
that we need to make an exception for but needed more information.  So if you're
okay with it, I'd like to drop trying to change #1 and concentrate on #2.

For #2, we'd like to know if the source tarball eventually lands at a specific
URL or if it's often not publically available at all.  Is it a snapshot or
something more formal?  If the tarball eventually lands someplace, just that it
wouldn't necessarily be present at the time we build, then we probably want to
include the full URL where the tarball will eventually land with a note that the
source may not be present there at this exact moment.  If the source will never
land one person suggested using a patch against the last released tarball. 
Another suggestion was to allow the mozilla source to go through with a spec
file comment explaining why there is no upstream URL.  A third suggestion was to
treat it like a Red Hat created application which is only distributed in the
srpm.  We don't have a policy for that yet so what the proceedure will be for
that is unknown.  If that's the route you think we need to go, I'll start
drafting something with Jeremy and let you know what it looks like.
Comment 11 Christopher Aillon 2007-02-08 21:26:37 EST
#1. There are other packages where this is more frequent and might be more than
just a 1 character change, such as some sort of checksum in the path.  The
bottom line is that while there are patterns, they are impossible to script in
every case and is an IMO unnecessary requirement.

#2. When I get it from upstream which is about 75% of the time, it will
eventually land somewhere, but I don't always know what the URL will be. 
Sometimes I am forced to pull directly from CVS and this tarball will never be
posted publicly.  In the mozilla case, patching against the latest tarball is
wrong.  There are hundreds of patches between releases and to sort them out is
silly.  Shipping a megadiff is also wrong because it makes it harder to
maintain, and I'd like to avoid becoming Debian.

#3 Additionally, forcing an http URI which the current guidelines mandate is not
going to fly for packages which don't distribute tarballs, but are maintained in
a git repository and have people check them out by tag.

I would like to see all these items addressed and furthermore, I'd like to see
this changed from a required item to a recommended item.  There is no real
one-size-fits all here that I can tell.  Additionally, It has no impact on the
quality of the RPM or the testability.  Aiding reviewers is nice, but does not
impact the RPM itself, and they can always ask.  As far as doing automated
md5sum matching, it is also nice, but I personally have no issue verifying it
myself, as I do for all source tarballs before I do anything with them (even
locally).
Comment 12 Toshio Kuratomi 2007-02-08 21:55:45 EST
#1 Please give me some arguments as to why it's an unnecessary requirement. 
Without arguments, it's going to fail to pass the committee.  With arguments it
may or may not pass but at least it'll be considered.

#2 Given your relation with upstream on Mozilla we could use the snapshot method
from #3 or the unwritten sources-only-in-srpm policy.  Let me know which one
would be preferable.

#3 I thought was taken care of in the rules for snapshots.  However, I'm unable
to find any snapshot guidelines on the wiki.  Looks like it's been an
undocumented guideline passed down on the mailing lists.  Basically, you have to
give the commands to generate the source tarball from the repo in a comment or
provide a script which can generate the tarball.  Snapshots also use a date in
the release tag which you may or may not like.

I'll write up the snapshot guideline and propose it since this is something that
we've been doing for a while.  Let me know if I need to put my head together
with jeremy and expedite the "sources only in srpm" policy as well.
Comment 13 Matthias Clasen 2007-02-10 18:14:53 EST
Regardless wether it passes committee or not, there are multiple scenarios where
there simply is not suitable source URL that can be put there. 

I have no problem putting the full source url for gnome packages in the spec
files, since I do so many package updates from gnome ftp that I can type the
full source url for a gnome tarball without looking. 

But what about packages which do not publish upstream releases in tarball form
at all, or packages where the fedora source rpm is the preferred form of
distribution because they are Red Hat inhouse projects ?
Comment 14 Toshio Kuratomi 2007-02-10 19:05:39 EST
As explained in #13, there are de facto rules for pulling from revision control
which need to be written down and voted on.  And Jeremy Katz is working on the
inhouse Red Hat "source rpm is canonical" policy.  We need to write those two
policies in such a way that Mozilla and similar are covered.

The way things have worked until now, the sources have been assumed to be
available somehow (otherwise it's not open source.)  If it's a tarball, provide
a URL.  If it's a snapshot from revision control, either a comment on how to
pull that revision or a script to pull it and construct the tarball is necessary.

I'm not sure what Jeremy's plans are WRT srpm's being the canonical source but
if the only public source is the rpm itself then the rules will have to reflect
that.

The overarching reason is that sources need to be checked against upstream.  One
of RPM's design goals is to cleanly separate the upstream code (in the form of a
tarball) from the vendor changes (in the form of patches).  Including the
information necessary to check this in the spec file helps reviewers to check
that the tarball is actually based on upstream.  In cases where we're upstream
we should theoretically be able to apply other, better tests to show this: like
tapping the developer on the shoulder and asking if he really released 0.2
yesterday with the following md5sum.
Comment 15 Patrice Dumas 2007-02-11 05:46:25 EST
In some rare case there it isn't possible to have an url, when 
there was an upstream project, but it has disappeared and one
happens to find a tarball somewhere.
Comment 16 Matthias Clasen 2007-02-11 08:19:32 EST
There is also the case of upstream projects which are hosted e.g. at
sourceforge, where it is very hard to come up with a working url. The
sourceforge download urls I have come up with had the filename as a parameter
behind a '?', and rpmbuild does not grok that, it seems to assume that the
filename part of the source url is always delimited by a '/'
Comment 17 Ville Skyttä 2007-02-11 08:38:51 EST
http://downloads.sourceforge.net/$project/$tarball-$version.tar.gz works in
those cases.
Comment 18 Toshio Kuratomi 2007-02-12 12:22:55 EST
(In reply to comment #15)
> In some rare case there it isn't possible to have an url, when 
> there was an upstream project, but it has disappeared and one
> happens to find a tarball somewhere.

In those cases, I generally consider that we have become upstream per:
http://www.fedoraproject.org/wiki/Extras/OrphanedPackages?highlight=%28upstream%29%7C%28orphan%29#head-f7fccda587fb4a4401f9f232639659d0c5832481

As such it will fall under the "We're upstream and source rpm is canonical" when
it is created.  Or we could use Debian or some other distro as the upstream and
have a comment that explains how to extract the source tarball from their package.
Comment 19 Toshio Kuratomi 2007-02-14 16:13:29 EST
I have a draft of the new guideline here::
  http://www.fedoraproject.org/wiki/PackagingDrafts/SourceUrl

I'm sending it out to the fedora-packaging list for suggestions and comments today.

caillon: If you could talk a bit about the Mozilla case and pose any suggestions
you might have about getting your Mozilla workflow to fit comfortably in one of
those sections that would be beneficial.

Since it seems no one here is demanding that file-roller fits under this rule,
I'm unblocking this review from FE-GUIDELINES with the expectation that the
Soure0: line would contain the canonical URL.  If that is in error, please feel
free to let me know along with some suggestions for enhancing the draft.
Comment 20 Christopher Aillon 2007-02-14 16:47:48 EST
Looks good from the brief glance I took, but I strongly feel this whole thing
should be a "good practises" recommendation and not a requirement.  If you're
trying to prevent against "bad" RPMs, well you're not going to do that if there
are exceptions... Even for a good SRPM, someone could simply fork an open source
project, not have a repo other than the SRPM, and distribute whatever code they
want that way in extras, theoretically.  This has no bearing on the actual
packaging or quality of RPMs.  It's only redeeming quality is that it might
potentially help with automated verification of upstream sources, but that does
not exist right now and that potential benefit should be enough to convince most
packagers to do this.  There's simply no reason to make it a hard requirement
IMO other than because it's always been that way (which is no real reason).
Comment 21 Matěj Cepl 2007-02-27 05:56:51 EST
Brian, so what is your conclusion about this bug (theoretical discussions about
Source0 URLs standard aside)?
Comment 22 Matthias Clasen 2007-06-17 00:16:55 EDT
Stalled review ?
Comment 23 Brian Pepple 2007-06-17 10:11:58 EDT
Going over the spec file again, all my initial blockers have been fixed. 
Setting review to '+'.
Comment 24 Matthias Clasen 2007-08-10 21:28:10 EDT
Review done.

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