Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=772608 --- Comment #14 from Jorge A Gallegos <k...@blegh.net> 2012-02-07 01:27:30 EST --- (In reply to comment #12) See my responses inline > (In reply to comment #11) > > > First, I can see a glaring issue with your package, you are shipping GDM's > > .src.rpm as part of your own source. That is probably not going to fly, even > > though I can't really find anything in the packaging guidelines that > > prevents > > you from doing it (closest would be > > http://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries > > but there's no mention about .src.rpm). I see some problems with your > > approach: > > > > - You are shipping a specific version of source that is already present in > > fedora > > - You will have to ship newer, static .src.rpm whenever GDM gets a newer > > version > > - You may end up fighting duplicated libraries > > > > The approach, in my opinion, would be to get in touch with the GDM > > packagers > > and ask for your stuff to be included if it's not already. It is not > > entirely > > apparent why requiring gdm-devel wouldn't work right now. > > The reason I include the gdm src.rpm file is because I didn't find another way > to compile a gdm plugin outside the gdm's source tree. It is used only during > compilation and linkage time and it is not shipped with the plugin itself. > That sounds weird, and actually sounds like a shortcoming of the GDM plugin framework and/or the gdm-devel package. I'd still say this is not permisible, but I'll leave the decision to a more seasoned packager (Steven?) > > [BLOCKER] The License field in the package spec file must match the actual > > license - the COPYING file in the source tarball has GPLv3, you have GPLv2+ > > in > > the spec file > > Fixed. > Cool! > > [BLOCKER] The sources used to build the package must match the upstream > > source, > > as provided in the spec URL - You should take a look at > > http://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control and > > not use a self-hosted source tarball (Source0: > > http://ghammer.fedorapeople.org/%{name}-%{version}.tar.bz2) > > As far as I understood this is allowed > (http://fedoraproject.org/wiki/Packaging/SourceURL). > From that wiki page: "There are several cases where upstream is not providing the source to you in an upstream tarball. In these cases you must document how to generate the tarball used in the rpm either through a spec file comment or a script included as a separate SourceX" Check that URL I pasted above again, you should actually describe how to create that tarball from git, not point to the tarball you created > > [BLOCKER] The package MUST successfully compile and build into binary rpms > > on > > at least one primary architecture - I got this error when trying a mock > > build: > > xorg-x11-server-Xorg is needed by gdm-1:3.2.1.1-6.fc16.x86_64. > > Probably a bug after fixes done after a review. I've updated a new version > (http://ghammer.fedorapeople.org/ovirt-guest-agent-1.0.1-1.fc16.src.rpm) which > compile on both 32 and 64 bits. > Try following Steven's advice and bump the rpm changelog every time you make changes (hint: rpmdev-bumpspec foo.spec). Also try submitting a koji scratch build (https://fedoraproject.org/wiki/Using_the_Koji_build_system) and paste the URL each time. In this case I just submitted it and the results are here: http://koji.fedoraproject.org/koji/taskinfo?taskID=3767959 As you can see, there are still some unmet build dependencies: error: Failed build dependencies: xorg-x11-server-Xorg is needed by gdm-1:3.2.1.1-6.fc17.x86_64 If you don't want to use koji you can use mock in your machine like this: mock -r fedora-16-x86_64 rebuild package-1.2-3.src.rpm. This should do a scratch build in a sanboxed chroot so you can see if all dependencies are met. > > [BLOCKER] Each package must consistently use macros - I see a mix of $MACRO > > and > > %{macro}, use one style and stick with. Also, if you don't plan on > > supporting > > EPEL/RH, you should do away with the whole RPM_BUILD_ROOT [citation needed, > > maybe?] > > I'm not sure I understand this. I checked with the gdm spec file (as an > example) and it look pretty much like mine (using %macro expect for > $RPM_BUILD_ROOT). > Fair enough, I suppose if an existing accepted package follows the same tone is fair to assume this is OK. > > Additionally, it seems you include some python code in there, so, using > > http://fedoraproject.org/wiki/Packaging:Python as guide: > > > > [BLOCKER] To build a package containing python2 files, you need to have > > BuildRequires: python2-devel > > Why do I need to include it? My build machine doesn't have the python2-devel > package installed. > I was just pointing out the python packaging guidelines. > > Some additional suggestions: > > - defattr is not needed anymore, if I recall > > "The %files section normally begins with a %defattr line which sets the > default > file permissions." > (http://fedoraproject.org/wiki/How_to_create_an_RPM_package). > > Maybe it is time to review/update the manual... :-) > Yeah, the defattr thing I got from another review somewhere... let me see, here it is https://bugzilla.redhat.com/show_bug.cgi?id=725292#c1 and a couple of other reviews I've seen it (and perhaps you are correct, packaging docs *could* be outdated) > > - you should name your different targets appropriately like > > "ovirt-guest-agent-gdm-plugin" instead of just "gdm-plugin" > > The rpmbuild does that for me. The rpm file name is called: > ovirt-guest-agent-gdm-plugin-1.0.1-1.fc16.x86_64. > Cool > > - in your %pre step, you add a user rhevagent if it doesn't exist, but you > > are > > also hardcoding the UID. You should leave that to the system to figure out > > and > > just use -r for system users > > This is an assigned uid. I don't think I quite understand this last one... what does "assigned" mean? If I do in my system (before installing this package) `/usr/sbin/useradd -u 175 -o -r haproxy -c "High Availability Proxy Daemon User" -d / -s /sbin/nologin` and then try installing this package, the %prep step would likely fail, I think? -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review