Bug 519538
Summary: | Review Request: clutter-sharp - C#/.NET bindings to Clutter | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Christian Krause <chkr> |
Component: | Package Review | Assignee: | Michel Lind <michel> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, michel, notting, palango |
Target Milestone: | --- | Flags: | michel:
fedora-review+
j: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-10-25 23:24:32 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Christian Krause
2009-08-27 00:26:21 UTC
Taking this review. Could you put a comment above the source line, describing how exactly to reproduce the tarball you're using? With SVN and Hg, I'd recommend just using the revision number instead of revision date, but with Git's long revision number... say # DIRNAME=... ./clutter-sharp-make-git-snapshot.sh the-commit-id Source0: ... I have added the commit id (usually a part of it is sufficient) to the spec file, enhanced the snapshot script and documented its usage. - commit id is needed to allow reproducible creation of tarball - date is needed to create correct tag for snapshot releases Spec URL: http://chkr.fedorapeople.org/review/clutter-sharp.spec SRPM URL: http://chkr.fedorapeople.org/review/clutter-sharp-0-0.2.20090828.fc11.src.rpm Thanks for creating the package. I don't have the time to make a full review but I noticed some things. * mixed use of %{buildroot} and $RPM_BUILD_ROOT ($RPM_BUILD_ROOT only in %install) * latest changelog version is 0-0.3 but release field says 0.2 * shouldn't the %defattr filed be %defattr(-,root,root,-)? (In reply to comment #3) > Thanks for creating the package. I don't have the time to make a full review > but I noticed some things. > > * mixed use of %{buildroot} and $RPM_BUILD_ROOT ($RPM_BUILD_ROOT only in > %install) > * latest changelog version is 0-0.3 but release field says 0.2 > * shouldn't the %defattr filed be %defattr(-,root,root,-)? Thanks for the comment - all issues fixed: Spec URL: http://chkr.fedorapeople.org/review/clutter-sharp.spec SRPM URL: http://chkr.fedorapeople.org/review/clutter-sharp-0-0.3.20090828.fc11.src.rpm Almost ready to pass review; there is a trivial summary change to make, and a more serious case of some dependencies not being listed Koji build (F-12): http://koji.fedoraproject.org/koji/taskinfo?taskID=1646731 MUST • rpmlint: source -- clean clutter-sharp.x86_64: E: no-binary clutter-sharp.x86_64: W: only-non-binary-in-usr-lib These are harmless, rpmlint is not Mono-aware clutter-sharp-devel.x86_64: W: summary-not-capitalized pkgconfig file for clutter-sharp clutter-sharp-devel.x86_64: W: no-documentation The first should probably be fixed. The default spec file has Development files for %{name} which could be used. Esp. since we're packaging more than just the .pc file! OK package name OK spec file name OK package guideline-compliant OK license complies with guidelines OK license field accurate OK license file not deleted OK spec in US English OK spec legible OK source matches upstream OK builds under >= 1 archs, others excluded (Koji) OK build dependencies complete (Koji) FAIL on the other hand, runtime dependencies are not complete rpm -q --requires clutter-sharp rpm -e clutter 2>out.txt cat out.txt | grep sharp shows that the dependency on clutter is not picked up; this should be added manually. Also, once clutter-gtk can be enabled (prod the maintainer for an update?) you'd want to Requires: clutter-gtk, and meanwhile, now you want to %exclude %{_libdir}/pkgconfig/clutter-gtk-sharp.pc NA locales handled using %find_lang, no %{_datadir}/locale NA library -> ldconfig NA relocatable: give reason OK own all directories OK no dupes in %files OK permission OK %clean RPM_BUILD_ROOT OK macros used consistently OK Package contains code OK headers in -devel OK if contains *.pc, req pkgconfig OK devel requires versioned base package OK clean buildroot before install OK filenames UTF-8 SHOULD OK package build in mock on all architectures ? package functioned as described not tested OK scriplets are sane OK require package not files Ping? There are some small changes that need to be made, otherwise the package is OK. Thank you very much for the review. (In reply to comment #6) > Ping? There are some small changes that need to be made, otherwise the package > is OK. Sorry for the late response. I came just back from 2 weeks of vacation. ;-) The new packages can be found here: Spec URL: http://chkr.fedorapeople.org/review/clutter-sharp.spec SRPM URL: http://chkr.fedorapeople.org/review/clutter-sharp-0-0.4.20090828.fc11.src.rpm (In reply to comment #5) > clutter-sharp-devel.x86_64: W: summary-not-capitalized pkgconfig file for > clutter-sharp > clutter-sharp-devel.x86_64: W: no-documentation > > The first should probably be fixed. The default spec file has > > Development files for %{name} > > which could be used. Esp. since we're packaging more than just the .pc file! Fixed. > OK build dependencies complete (Koji) > > FAIL on the other hand, runtime dependencies are not complete > > rpm -q --requires clutter-sharp > > rpm -e clutter 2>out.txt > cat out.txt | grep sharp > > shows that the dependency on clutter is not picked up; this should be > added manually. Fixed. > Also, once clutter-gtk can be enabled (prod the maintainer for an update?) Unfortunately clutter-gtk is not yet released yet: http://www.clutter-project.org/sources/clutter-gtk/0.10/ . I'll try to keep this in mind and create a bug entry for clutter-gtk once 0.10.3 is released. > you'd want to Requires: clutter-gtk, and meanwhile, now you want to > %exclude %{_libdir}/pkgconfig/clutter-gtk-sharp.pc Fixed. (In reply to comment #7) > Thank you very much for the review. > > (In reply to comment #6) > Sorry for the late response. I came just back from 2 weeks of vacation. ;-) Ah, nice! > The new packages can be found here: > > Spec URL: http://chkr.fedorapeople.org/review/clutter-sharp.spec > SRPM URL: > http://chkr.fedorapeople.org/review/clutter-sharp-0-0.4.20090828.fc11.src.rpm Will review tomorrow; I had to dual-boot to Windows to do some .NET testing, and now am stuck waiting for a rather big download from a tediously slow server ([rant]seriously, upstreams should consider using BitTorrent with a private tracker -- [/rant]) > > Also, once clutter-gtk can be enabled (prod the maintainer for an update?) > > Unfortunately clutter-gtk is not yet released yet: > http://www.clutter-project.org/sources/clutter-gtk/0.10/ . I'll try to keep > this in mind and create a bug entry for clutter-gtk once 0.10.3 is released. > How about packaging a pre-release snapshot? As long as we know for certain -- or reasonably so -- that the final version will come out before we freeze for final, that's probably good enough. (In reply to comment #8) > > Unfortunately clutter-gtk is not yet released yet: > > http://www.clutter-project.org/sources/clutter-gtk/0.10/ . I'll try to keep > > this in mind and create a bug entry for clutter-gtk once 0.10.3 is released. > > > How about packaging a pre-release snapshot? As long as we know for certain -- > or reasonably so -- that the final version will come out before we freeze for > final, that's probably good enough. I've checked the clutter-gtk's git-repository and it looks like that there was not much activity in the last week: http://git.clutter-project.org/cgit.cgi?url=clutter-gtk/log/ Since the final freeze for F12 is on 9/29 I believe it would be too risky. :-( Changes look fine -- APPROVED. New Package CVS Request ======================= Package Name: clutter-sharp Short Description: C#/.NET bindings to Clutter Owners: chkr Branches: F-11 F-10 InitialCC: CVS done. |