Bug 165833 - Review Request: git
Review Request: git
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
David Lawrence
http://www.kernel.org/pub/linux/kerne...
:
Depends On: 166308
Blocks: FE-ACCEPT 165836
  Show dependency treegraph
 
Reported: 2005-08-12 14:24 EDT by Chris Wright
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-09-16 20:49:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
wtogami: fedora‑cvs+


Attachments (Terms of Use)
spec file fixes (1.44 KB, patch)
2005-08-17 23:00 EDT, Tom "spot" Callaway
no flags Details | Diff
use $(RPM_OPT_FLAGS) (269 bytes, patch)
2005-08-17 23:01 EDT, Tom "spot" Callaway
no flags Details | Diff
spec update (1.49 KB, patch)
2005-08-18 20:09 EDT, Chris Wright
no flags Details | Diff

  None (edit)
Description Chris Wright 2005-08-12 14:24:49 EDT
Spec Name or Url: http://www.kernel.org/pub/linux/kernel/people/chrisw/git/review/git-core.spec
SRPM Name or Url: http://www.kernel.org/pub/linux/kernel/people/chrisw/git/review/git-core-0.99.4-1.src.rpm
Description: git-core provides core functionality for git content tracker used
as SCM for linux kernel and other projects.
Comment 1 Josh Boyer 2005-08-12 22:16:47 EDT
Forbidden

You don't have permission to access
/pub/linux/kernel/people/chrisw/git/review/git-core.spec on this server.

403 here too.

Comment 2 Chris Wright 2005-08-13 03:10:09 EDT
Sorry, fixed that one too.  Thanks for reviewing.
Comment 3 Josh Boyer 2005-08-13 14:38:28 EDT
Nits:

1) Buildroot should be:

%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

to match up with the Packaging guidelines for FE.

2) Normally FE packages don't carry the Vendor: tag

Problems:

1) This doesn't build on FC4 or rawhide when using mock due to the lack of an
asciidoc package.  We could either change the without_docs macro to with_docs
instead, find a different utility that works to generate the docs, or get
asciidoc imported into Extras.

2) curl and openssl are missing from Requires

I'll do some more reviewing a bit later.
Comment 4 Chris Wright 2005-08-15 13:21:58 EDT
- Fix Buildroot to:

%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

- Remove Vendor tag

- Add curl and openssl to Requires

- Open, asciidoc issue...

Upstream perfers to default doc building to on.  It is nice to get man pages
instead of txt files in /usr/share/doc/git-core.  Downside, asciidoc is
non-standard, and extremely slow.  I'm open to suggestions.

Thanks for the review.

New spec file and src.rpm are uploading to:

http://kernel.org/pub/linux/kernel/people/chrisw/git/review/git-core-0.99.4-2.src.rpm
http://kernel.org/pub/linux/kernel/people/chrisw/git/review/git-core.spec
Comment 5 Josh Boyer 2005-08-17 20:04:44 EDT
Thanks for doing the updates.  They look good.

I agree that building with docs is probably important.  We'll have to figure
something out.  I'll be checking off and on, but my kid decided to be born on
Monday, so I'm not sure how much time I'll have.  I'll bug some people about
getting you sponsored and finishing this review.
Comment 6 Chris Wright 2005-08-17 20:09:39 EDT
Wow, congratulations! ;-)  Thanks for your help.  I'm open to suggestion re: the
docs.
Comment 7 Tom "spot" Callaway 2005-08-17 22:57:23 EDT
I'm not seeing asciidoc packaged in Core or Extras... am I missing something
here? That will be needed before it will build with docs (and since thats the
default...)

Also, there are a couple of items in the spec:

- It's not respecting RPM_OPT_FLAGS, which is bad, but trivial to fix.
- The package wasn't owning any of the directories in %{_datadir}/git-core/
- You're not using a %{?dist} tag, which is ok, but it does make cross-branch
versioning trickier.

I patched in support for these items. Watch this space for attachments. :)

But first... we need to see an asciidoc package request for review. 
Comment 8 Tom "spot" Callaway 2005-08-17 23:00:10 EDT
Created attachment 117860 [details]
spec file fixes

fixup spec file
Comment 9 Tom "spot" Callaway 2005-08-17 23:01:13 EDT
Created attachment 117861 [details]
use $(RPM_OPT_FLAGS)

use $(RPM_OPT_FLAGS)
Comment 10 Warren Togami 2005-08-18 03:21:17 EDT
Requires: 	sh-utils, diffutils, rsync, rcs, mktemp >= 1.5, curl, openssl

sh-utils, diffutils, and mktemp are unnecessary to list here because you can't
have an installed system without these packages already pulled in by core
dependencies.  The others are fine to list, except wouldn't openssl be pulled in
as a linked library dependency automatically?

Also please answer the question about asciidoc.  If the package requires
asciidoc by default to build, then you must also submit that for Extras review.
 Otherwise disable it by default in the spec.
Comment 11 Chris Wright 2005-08-18 14:59:06 EDT
Thank you for the updates.  I had added openssl as suggested by Josh, however
the library dependency is certainly auto discovered.  So I've removed it, as
well as sh-utils, diffutils and mktemp.  One question regarding RPM_OPT_FLAGS,
is it reasonable to simply do:

%build
COPTS=$RPM_OPT_FLAGS make all...

as opposed to maintaining a patch that will never go upstream?  That'd be my
preference unless it's an issue of some standardization for FE that I'm unaware
of.  Regarding asciidoc, I'll just submit that package for review this afternoon. 
Comment 12 Tom "spot" Callaway 2005-08-18 15:08:30 EDT
No, thats actually a much better idea. I tried doing CFLAGS=$RPM_OPT_FLAGS, but
that didn't do the trick, so I wrote the patch.
Comment 13 Chris Wright 2005-08-18 20:06:11 EDT
OK, updated and uploading to:

http://www.kernel.org/pub/linux/kernel/people/chrisw/git/review/

I submitted asciidoc as well.
Comment 14 Chris Wright 2005-08-18 20:09:32 EDT
Created attachment 117893 [details]
spec update
Comment 15 Josh Boyer 2005-08-29 20:36:23 EDT
Chris, after talking with Warren, I've imported your latest package into CVS to
keep things moving until you get your account created.
Comment 16 Josh Boyer 2005-09-13 13:11:39 EDT
Any progress being made here?
Comment 17 Jacob Kroon 2005-09-15 04:22:39 EDT
I noticed git-core has made it to extras-development. Since FC5 is still far away, 
are there any plans on adding it to FC4 extras ? Or perhaps this is the normal
path a package takes before landing up in one of the "stable" extra-trees ?
Comment 18 Josh Boyer 2005-09-15 12:51:56 EDT
I brought the package into CVS (along with asciidoc) and built it for
development while Chris was waiting for CVS access.

I was hoping Chris would request the branches for FC-3 and FC-4, but there
hasn't been much activity here in a while.  I can request them if needs be.
Comment 19 Ed Hill 2005-09-15 13:20:13 EDT
Hi Josh, if its OK, would you please request branches and builds for FC4?  It
would be nice to have an easy way to install and try out cogito/git.  And *many*
thanks to everyone who worked to get them into FE!
Comment 20 Josh Boyer 2005-09-16 06:34:46 EDT
Ok, branches requested and built.  Should be available with the next repo push.

I consider this approved.  Tom, care to finish the bug off?

Chris, you'll need to update git to the latest release and add this to the
owners.list file when you can.
Comment 21 Chris Wright 2005-09-16 14:27:00 EDT
Updated to latest git, although import doesn't like tagging the branches. 
Assume I need to manually tag when updating a branch?  Added to owners.list.
Comment 22 Josh Boyer 2005-09-16 15:01:10 EDT
Typically you only use import for the initial import into CVS.  After that, you
use 'make new-sources FILES="foobar.tar.gz"' to upload a new tarball.

You need to manually tag before building each branch.  So once you get all the
source and specfiles committed to CVS, you do this:

make tag

and then this to build:

make build

See http://fedoraproject.org/wiki/Extras/UsingCvsFaq and
http://fedoraproject.org/wiki/Extras/BuildRequests for more info.

Now that this has been added to owners.list, you can close the bug as NEXTRELEASE.
Comment 23 Chris Wright 2005-09-16 15:09:50 EDT
Oh, I see.  The FAQ mentions import -b as part of update a new .src.rpm under
Update Existing Package in CVS, which is what I used.  So if I'm updating to new
version, I commit a new spec file, and just follow the make new-sources route?
I did manually tag, so I believe the tags are OK.  Please holler at me if I got
that wrong (it's a bit of a maze in here ;-)
Comment 24 Christian Iseli 2006-04-08 16:45:12 EDT
Please rename the git-core entry in owners.list to git
Comment 25 Christian Iseli 2006-10-18 05:21:02 EDT
Normalize summary field for easy parsing
Comment 26 James Bowes 2007-06-08 15:46:26 EDT
Package Change Request
======================
Package Name: git
New Branches: EL-4 EL-5

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