Spec URL: http://www.bagu.org/eclipse/junit4.spec SRPM URL: http://www.bagu.org/eclipse/junit4-4.3.1-1jpp.1.fc7.src.rpm Description: JUnit is a regression testing framework written by Erich Gamma and Kent Beck. It is used by the developer who implements unit tests in Java. JUnit is Open Source Software, released under the IBM Public License and hosted on SourceForge. Additional Info: The reason I am proposing a new junit4 package rather than updating the junit3 package is because eclipse 3.3 requires junit 3 and 4.
I will start on the review...
Created attachment 160393 [details] Detailed review Overview of things that need attention: X * skim the summary and description for typos, etc. # The summary is outdated - references EPL which may confuse people. See patch # for fix from http://www.junit.org/index.htm X * correct buildroot - should be: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) # Needs to be fixed X * license text included in package and marked with %doc # cpl-v10.html should be included in %doc X * rpmlint on <this package>.srpm gives no output # tab and space mixing issues: see patch X * run rpmlint on the binary RPMs XNote: group warnings can be ignored. # Results from binary rpm rpmlint-ing #$ rpmlint -v /var/lib/mock/fedora-7-x86_64/root/builddir/build/RPMS/junit4-* #I: junit4 checking #W: junit4 wrong-file-end-of-line-encoding /usr/share/doc/junit4-4.3.1/README.html #I: junit4-debuginfo checking #I: junit4-demo checking #W: junit4-demo no-documentation #I: junit4-javadoc checking #I: junit4-manual checking #W: junit4-manual wrong-file-end-of-line-encoding /usr/share/doc/junit4-manual-4.3.1/faq/faq.htm #W: junit4-manual wrong-file-end-of-line-encoding /usr/share/doc/junit4-manual-4.3.1/testinfected/testing.htm #W: junit4-manual wrong-file-end-of-line-encoding /usr/share/doc/junit4-manual-4.3.1/index.htm #W: junit4-manual wrong-file-end-of-line-encoding /usr/share/doc/junit4-manual-4.3.1/cookbook/cookbook.htm #W: junit4-manual wrong-file-end-of-line-encoding /usr/share/doc/junit4-manual-4.3.1/cookstour/cookstour.htm SHOULD: X * package should include license text in the package and mark it with %doc # See above, might need fixing
Created attachment 160394 [details] Suggested fixes for some of the issues NEEDSWORK: See above summary
New files: http://bagu.org/eclipse/junit4.spec http://bagu.org/eclipse/junit4-4.3.1-1jpp.2.fc8.src.rpm (In reply to comment #2) > Created an attachment (id=160393) [edit] > Detailed review > > Overview of things that need attention: > X * skim the summary and description for typos, etc. > # The summary is outdated - references EPL which may confuse people. See > patch > # for fix from http://www.junit.org/index.htm Fixed > X * correct buildroot > - should be: > %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) > # Needs to be fixed Fixed > X * license text included in package and marked with %doc > # cpl-v10.html should be included in %doc This license file is not installed so it's not needed. > X * rpmlint on <this package>.srpm gives no output > # tab and space mixing issues: see patch Fixed > X * run rpmlint on the binary RPMs Fix most issues. Running rpmlint on the binary rpm now gives these messages: %% rpmlint RPMS/x86_64/junit4-* E: junit4 no-binary This is ok because that package currently doesn't have binary files since aot-compile-rpm is temporarily disabled due to gcj bugs. E: junit4-debuginfo empty-debuginfo-package Again, this package is empty because aot-compile-rpm is temporarily disabled due to gcj bugs. W: junit4-demo no-documentation There is no documentation for the demo packages.
(In reply to comment #4) > New files: > > http://bagu.org/eclipse/junit4.spec > http://bagu.org/eclipse/junit4-4.3.1-1jpp.2.fc8.src.rpm > Thanks and sorry for the delay. I dont think you need to bump the release for this, but whatever :) This is probably really nit-picky but I recommend using the %define _with_gcj_support 0 up top to disable aot compilation for the moment if you want to. So you wont have to comment out ownership under the %{_libdir}/gcj etc. and once the bugs in gcj are fixed simply switching the gcj_support thing would do. Also, it seems that the version of spec-gcj-convert used to build the spec has bugs: %postun %if %{gcj_support} ... %endif while it really should be: %if %{gcj_support} %postun ... %endif This leaves empty script sections when gcj support is disabled by default - which is the case in jpackage and currently, due to the said gcj bug for Fedora as well. Adding fnasser to CC since he is going to handle the push of the package to jpackage. NEEDSWORK: See above
New Files: http://bagu.org/eclipse/junit4-4.3.1-1jpp.1.fc8.src.rpm http://bagu.org/eclipse/junit4.spec (In reply to comment #5) > Thanks and sorry for the delay. I dont think you need to bump the release for > this, but whatever :) Ok, I put it back to 1. > This is probably really nit-picky but I recommend using the > %define _with_gcj_support 0 up top to disable aot compilation for the moment if > you want to. So you wont have to comment out ownership under the %{_libdir}/gcj > etc. and once the bugs in gcj are fixed simply switching the gcj_support thing > would do. The macro wasn't working so I just set gcj_support to 0 manually. > Also, it seems that the version of spec-gcj-convert used to build the > spec has bugs: > %postun > %if %{gcj_support} > ... > %endif > > while it really should be: > %if %{gcj_support} > %postun > ... > %endif > > This leaves empty script sections when gcj support is disabled by default - > which is the case in jpackage and currently, due to the said gcj bug for Fedora > as well. Fixed.
Starting import procedure
Confirmed that the changes requested by the reviewer were implemented. Requesting 'junit4' cvs branch for import.
Can the submitter add a CVS template so we know what branches are needed, etc? See: http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure Set the fedora-cvs flag back to ? when you are ready.
Sorry, missed the template: Package Change Request ====================== Package Name: junit4 New Branches: devel Fedora Owners: fnasser, bkonrath
I'm a little confused here... Vivek was reviewing this package, but Fernando approved it and is going to maintain it as well? It's not typically allowed to approve a package that you are going to maintain. (With the idea a reviewer should be a disinterested party). Could we get a ack from Vivek that he approves this review and sees no further issues?
(In reply to comment #6) > Fixed. Thanks Ben, Everything looks good. APPROVED.
Sorry to be a pain about that...just want to make sure everything is aboveboard and correct. :) cvs done.
(In reply to comment #13) > Sorry to be a pain about that...just want to make sure everything is aboveboard > and correct. :) > > cvs done. Thanks Kevin! Your concern was valid. I mentioned to Fernando elsewhere that everything was OK with the package but he might have been a bit too eager to get this out on rawhide :)
Built into Rawhide