Spec URL: http://icedrobot.de/~roman/mockito.spec SRPM URL: http://icedrobot.de/~roman/mockito-1.9.0-1.fc16.src.rpm Description: Mockito is a mocking framework that tastes really good. It lets you write beautiful tests with clean & simple API. Mockito doesn't give you hangover because the tests are very readable and they produce clean verification errors. More information can be found: http://code.google.com/p/mockito/wiki/FeaturesAndMotivations
Not a sponsor so just a few quick things: * Instead of having a patch adding the pom, add it as additional Source1 (direct link to maven repo or wherever) - you can refer to it later as %{SOURCE1} * You are not installing license. Both main package and javadoc has to have %doc LICENSE (and in this case also NOTICE) * Instead of: cp -p target/mockito-core-1.9.0.jar $RPM_BUILD_ROOT%{_javadir}/%{name}.jar do: cp -p target/mockito-core-%{version}.jar $RPM_BUILD_ROOT%{_javadir}/%{name}.jar It will simplify updates * Some upstreams have complicated build systems that add certain files in special places and then generate something else out of them. It means that if you build "your way" you can inadvertently introduce changes into final binary jars. That's why we usually (not always!) prefer to build from SCM over using source jars. Best way to solve this long term: ask upstream to generate -project.zip files with complete contents of SCM needed to build. I wouldn't block a review on this, but I would urge you to be careful. Ugly things have happened when building like this :-)
> * Instead of having a patch adding the pom, add it as additional Source1 > (direct link to maven repo or wherever) - you can refer to it later as > %{SOURCE1} The problem is that the pom that they upload into the Maven repo lacks 2 dependencies: to ant and to junit (which in this case is not only needed for test scope). That's why I made my own quick pom out of the Maven repository one. Mockito upstream doesn't seem to use Maven themselves. They use ant, and I suppose this would have complicated the build process even more. > * You are not installing license. Both main package and javadoc has to have > %doc LICENSE (and in this case also NOTICE) Fixed. > * Instead of: > cp -p target/mockito-core-1.9.0.jar $RPM_BUILD_ROOT%{_javadir}/%{name}.jar > do: > cp -p target/mockito-core-%{version}.jar $RPM_BUILD_ROOT%{_javadir}/%{name}.jar > It will simplify updates Fixed this and a couple of other places where the version is hardcoded. > * Some upstreams have complicated build systems that add certain files in > special places and then generate something else out of them. It means that if > you build "your way" you can inadvertently introduce changes into final binary > jars. That's why we usually (not always!) prefer to build from SCM over using > source jars. Best way to solve this long term: ask upstream to generate > -project.zip files with complete contents of SCM needed to build. I wouldn't > block a review on this, but I would urge you to be careful. Ugly things have > happened when building like this :-) Yeah true. Maybe I should look into building with upstream's ant instead. Or get them to provide a correct pom somewhere. I updated the above linked files to reflect the changes I made. Thanks, Roman
(In reply to comment #2) > > * Instead of having a patch adding the pom, add it as additional Source1 > > (direct link to maven repo or wherever) - you can refer to it later as > > %{SOURCE1} > > The problem is that the pom that they upload into the Maven repo lacks 2 > dependencies: to ant and to junit (which in this case is not only needed for > test scope). That's why I made my own quick pom out of the Maven repository > one. Mockito upstream doesn't seem to use Maven themselves. They use ant, and I > suppose this would have complicated the build process even more. > Funny, most people still prefer ant over maven and think the process is more complicated with mvn :-) Anyway, as far as pom is concerned. You should have Source1 as original pom from maven central and then patch on top of this pom that would only introduce changes needed in fedora and why they are needed. > > * You are not installing license. Both main package and javadoc has to have > > %doc LICENSE (and in this case also NOTICE) > > Fixed. Good, note that you can include both on the same line (i.e. %doc LICENSE NOTICE). Nothing wrong with separate lines, just making sure you are aware of both options > > * Instead of: > > cp -p target/mockito-core-1.9.0.jar $RPM_BUILD_ROOT%{_javadir}/%{name}.jar > > do: > > cp -p target/mockito-core-%{version}.jar $RPM_BUILD_ROOT%{_javadir}/%{name}.jar > > It will simplify updates > > Fixed this and a couple of other places where the version is hardcoded. OK > > * Some upstreams have complicated build systems that add certain files in > > special places and then generate something else out of them. It means that if > > you build "your way" you can inadvertently introduce changes into final binary > > jars. That's why we usually (not always!) prefer to build from SCM over using > > source jars. Best way to solve this long term: ask upstream to generate > > -project.zip files with complete contents of SCM needed to build. I wouldn't > > block a review on this, but I would urge you to be careful. Ugly things have > > happened when building like this :-) > > Yeah true. Maybe I should look into building with upstream's ant instead. Or > get them to provide a correct pom somewhere. It is not a *hard* requirement, but we usually suggest using upstream's build method with exception of gradle or similar ugly things :-) If you have a good reason not to use upstream build method just state it in the comment somewhere in the spec. As far as I am concerned: you are maintainer and it's up to you to decide best way to handle your package. You just have to be aware of ups/downs of different decisions. > I updated the above linked files to reflect the changes I made. Normally during review you should raise release number and add changelog just as you would during normal package maintenance. It shows additional skills + it's much easier to track changes through the review process. I.e. the spec url can stay the same, but at least srpms should be always different. However from your comment on IRC I looked at the sources and as far as I see they bundle asm and cglib in org.mockito.[asm|cglib] tree. This means you have to remove those subtrees, change classes that use "shaded" classpaths to use original asm classpaths and finally add proper dependencies in pom.xml. Might get complicated if APIs are different (to the point where you'd have to package different version of cglib & asm as additional rpms)
Ok, I now did the additional changes: - Use the upstream build system, with some changes to skip tests which would pull in additional dependencies that can currently not be fullfilled, and modification of build classpath to use the libraries under /usr/share/java instead of bundled ones. - Created source tarball myself by checking out the upstream sourcecode, updating to release tag 1.9.0, stripping out bundled dependency JARs and a bunch of questionable fluff, converting line endings to Unix - Fixed spec to work with the above changes Files in same location as above. /Roman
Just a few quick comments for now: (In reply to comment #4) > Ok, I now did the additional changes: Please always bump the release and add a changelog entry what you did. This way you also get a new SRPM (with other relase), so anyone can run a diff, who wants to. > - Created source tarball myself by checking out the upstream sourcecode, > updating to release tag 1.9.0, stripping out bundled dependency JARs and a > bunch of questionable fluff, converting line endings to Unix How? :) There needs to be a full URL where you download the source or describe in a comment in the spec file, how you did it (not like a comment like here, bash what you did). > Files in same location as above. It would be best to always putt the URL again here, so nobody needs to search again, furthermore the link will change slightly, when you bump the release ;)
(In reply to comment #5) > Just a few quick comments for now: > (In reply to comment #4) > > Ok, I now did the additional changes: > > Please always bump the release and add a changelog entry what you did. This way > you also get a new SRPM (with other relase), so anyone can run a diff, who > wants to. Ok thanks for the hint. Will do so for any future changes. > > - Created source tarball myself by checking out the upstream sourcecode, > > updating to release tag 1.9.0, stripping out bundled dependency JARs and a > > bunch of questionable fluff, converting line endings to Unix > > How? :) There needs to be a full URL where you download the source or describe > in a comment in the spec file, how you did it (not like a comment like here, > bash what you did). I made a small shell script. Should I include it in the package or what is the preferred way of distributing it? > > Files in same location as above. > > It would be best to always putt the URL again here, so nobody needs to search > again, furthermore the link will change slightly, when you bump the release ;) Ok, will do so for any future changes (I actually might setup a VCS just for the specfile, patches, and the script). Spec URL: http://icedrobot.de/~roman/mockito.spec SRPM URL: http://icedrobot.de/~roman/mockito-1.9.0-1.fc16.src.rpm Thanks, Roman
I'll do this one.
Package Review ============== Key: - = N/A x = Pass ! = Fail ? = Not evaluated ==== Generic ==== [x]: MUST Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: MUST Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: MUST All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: MUST Buildroot is not present Note: Unless packager wants to package for EPEL5 this is fine [x]: MUST Package contains no bundled libraries. [x]: MUST Changelog in prescribed format. [x]: MUST Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) Note: Clean would be needed if support for EPEL is required [x]: MUST Sources contain only permissible code or content. [x]: MUST Each %files section contains %defattr if rpm < 4.4 Note: Note: defattr macros not found. They would be needed for EPEL5 [x]: MUST Macros in Summary, %description expandable at SRPM build time. [x]: MUST Package requires other packages for directories it uses. [x]: MUST Package uses nothing in %doc for runtime. [x]: MUST Package is not known to require ExcludeArch. [x]: MUST Permissions on files are set properly. [x]: MUST Package does not contain duplicates in %files. [x]: MUST Spec file lacks Packager, Vendor, PreReq tags. [x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. Note: rm -rf would be needed if support for EPEL5 is required [x]: MUST If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [x]: MUST License field in the package spec file matches the actual license. [x]: MUST License file installed when any subpackage combination is installed. [x]: MUST Package consistently uses macros (instead of hard-coded directory names). [x]: MUST Package meets the Packaging Guidelines. [x]: MUST Package is named according to the Package Naming Guidelines. [x]: MUST Package does not generates any conflict. [x]: MUST Package obeys FHS, except libexecdir and /usr/target. [x]: MUST Package must own all directories that it creates. [x]: MUST Package does not own files or directories owned by other packages. [x]: MUST Package installs properly. [x]: MUST Requires correct, justified where necessary. [x]: MUST Rpmlint output is silent. rpmlint mockito-javadoc-1.9.0-1.fc18.noarch.rpm mockito-javadoc.noarch: W: spelling-error Summary(en_US) Javadocs -> Java docs, Java-docs, Avocados 1 packages and 0 specfiles checked; 0 errors, 1 warnings. rpmlint mockito-1.9.0-1.fc18.src.rpm mockito.src: W: invalid-url Source0: mockito-1.9.0.tar.xz 1 packages and 0 specfiles checked; 0 errors, 1 warnings. rpmlint mockito-1.9.0-1.fc18.noarch.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. [x]: MUST Sources used to build the package match the upstream source, as provided in the spec URL. [x]: MUST Spec file is legible and written in American English. [x]: MUST Spec file name must match the spec package %{name}, in the format %{name}.spec. [-]: MUST Package contains a SysV-style init script if in need of one. [x]: MUST File names are valid UTF-8. [x]: SHOULD Reviewer should test that the package builds in mock. [x]: SHOULD If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: SHOULD Dist tag is present. [x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [x]: SHOULD Package functions as described. [x]: SHOULD Package does not include license text files separate from upstream. [x]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: SHOULD SourceX / PatchY prefixed with %{name}. Note: Source0: mockito-%{version}.tar.xz (mockito-%{version}.tar.xz) Patch0: fixup-ant-script.patch (fixup-ant-script.patch) Patch1: fix- cglib-refs.patch (fix-cglib-refs.patch) [x]: SHOULD SourceX is a working URL. [-]: SHOULD Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: SHOULD Package should compile and build into binary rpms on all supported architectures. [-]: SHOULD %check is present and all tests pass. [-]: SHOULD Packages should try to preserve timestamps of original installed files. [x]: SHOULD Spec use %global instead of %define. ==== Java ==== [-]: MUST If source tarball includes bundled jar/class files these need to be removed prior to building [x]: MUST Packages have proper BuildRequires/Requires on jpackage-utils [x]: MUST Fully versioned dependency in subpackages, if present. [x]: MUST Javadoc documentation files are generated and included in -javadoc subpackage [x]: MUST Javadoc subpackages have Requires: jpackage-utils [x]: MUST Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlink) [x]: SHOULD Package has BuildArch: noarch (if possible) [x]: SHOULD Package uses upstream build method (ant/maven/etc.) ==== Maven ==== [x]: MUST Old add_to_maven_depmap macro is not being used [!]: MUST If package contains pom.xml files install it (including depmaps) even when building with ant Issues: [!] Pom file is available in the maven directory - it would be good to install it(mockito-core.pom) I assume together with a depmap. This will simplify a number of other packages where we build with maven and patch to strip out mockito because it was not packaged. [!] Please include the shell script used for generating the tarball as Source1 in the spec [!] I suspect that there are missing Requires because there are too many BuildRequires but not Requires. Are they really build time only dependencies and not runtime? The mockito-core.pom lists hamcrest and objenesis as runtime dependencies, true?
Thanks Alexander for the review! I fixed the Issues you mentioned and uploaded a new package: SPEC: http://icedrobot.de/~roman/mockito/2/mockito.spec SRPM: http://icedrobot.de/~roman/mockito/2/mockito-1.9.0-2.fc16.src.rpm I also have a number of additional questions: - The included Maven pom is lacking a bunch of dependencies: - cglib (and maybe asm) because upstream repackages and includes them in their own JAR. - JUnit4 because users of Mockito certainly must include that otherwise Mockito doesn't make sense. I suppose I need to patch-in at least the former two. Question: How do we deal with Maven dependency versions? In Maven I need to put an explicit in the dependency, which version should I pick? Do we also need to patch the versions of the existing dependencies (hamcrest, objenesis) to match what is in Fedora? - rpmlint complains about Requires: cglib, should I leave it out? Does rpm really figure out that dependency by itself as it claims? And why doesn't it complain about the other libs that I depend on? Cheers, Roman > Issues: > [!] Pom file is available in the maven directory - it would be good to install > it(mockito-core.pom) I assume together with a depmap. This will simplify a > number of other packages where we build with maven and patch to strip out > mockito because it was not packaged. > [!] Please include the shell script used for generating the tarball as Source1 > in the spec > [!] I suspect that there are missing Requires because there are too many > BuildRequires but not Requires. Are they really build time only dependencies > and not runtime? The mockito-core.pom lists hamcrest and objenesis as runtime > dependencies, true?
(In reply to comment #9) > Thanks Alexander for the review! I fixed the Issues you mentioned and uploaded > a new package: > > SPEC: http://icedrobot.de/~roman/mockito/2/mockito.spec > SRPM: http://icedrobot.de/~roman/mockito/2/mockito-1.9.0-2.fc16.src.rpm > > I also have a number of additional questions: > - The included Maven pom is lacking a bunch of dependencies: > - cglib (and maybe asm) because upstream repackages and includes them in > their own JAR. > - JUnit4 because users of Mockito certainly must include that otherwise > Mockito doesn't make sense. > > I suppose I need to patch-in at least the former two. Question: How do we deal > with Maven dependency versions? In Maven I need to put an explicit in the > dependency, which version should I pick? Do we also need to patch the versions > of the existing dependencies (hamcrest, objenesis) to match what is in Fedora? Yes, adding them to the pom after unbundling is the way. Versions don't matter for Fedora per se because our maven ignores them anyway, so I guess it's up to you and how you want to handle it. The only change would be if you wanted to upstream this patch, which might be tricky. > - rpmlint complains about Requires: cglib, should I leave it out? Does rpm > really figure out that dependency by itself as it claims? And why doesn't it > complain about the other libs that I depend on? Nope, rpmlint just sees "*lib" and thinks it knows, but it doesn't :-) Leave it in or we'd have missing deps. I'll let Alex continue with the rest :-)
> > Thanks Alexander for the review! I fixed the Issues you mentioned and uploaded > > a new package: > > > > SPEC: http://icedrobot.de/~roman/mockito/2/mockito.spec > > SRPM: http://icedrobot.de/~roman/mockito/2/mockito-1.9.0-2.fc16.src.rpm > > > > I also have a number of additional questions: > > - The included Maven pom is lacking a bunch of dependencies: > > - cglib (and maybe asm) because upstream repackages and includes them in > > their own JAR. > > - JUnit4 because users of Mockito certainly must include that otherwise > > Mockito doesn't make sense. > > > > I suppose I need to patch-in at least the former two. Question: How do we deal > > with Maven dependency versions? In Maven I need to put an explicit in the > > dependency, which version should I pick? Do we also need to patch the versions > > of the existing dependencies (hamcrest, objenesis) to match what is in Fedora? > > Yes, adding them to the pom after unbundling is the way. Versions don't matter > for Fedora per se because our maven ignores them anyway, so I guess it's up to > you and how you want to handle it. The only change would be if you wanted to > upstream this patch, which might be tricky. Well for upstream it'd actually be easy since I can just put any version that is in Maven central and works with Mockito. I found another problem: it seems like cglib in Fedora doesn't install a pom, even though there seems to be one in Maven central. What should I do with it? Asm is not used directly by Mockito, only cglib requires and depends on it (transitively), therefore I don't need to put it into mockito's pom. > > - rpmlint complains about Requires: cglib, should I leave it out? Does rpm > > really figure out that dependency by itself as it claims? And why doesn't it > > complain about the other libs that I depend on? > > Nope, rpmlint just sees "*lib" and thinks it knows, but it doesn't :-) Leave it > in or we'd have missing deps. Ok alright :-) > I'll let Alex continue with the rest :-) Yep, thanks alot! Roman
(In reply to comment #9) > - JUnit4 because users of Mockito certainly must include that otherwise > Mockito doesn't make sense. I would not agree with that. Mockito can be easily used with TestNG as well as probably with any other testing framework/library for Java. JUnit is only internally used to run Mockito's own tests during a build process.
(In reply to comment #12) > (In reply to comment #9) > > - JUnit4 because users of Mockito certainly must include that otherwise > > Mockito doesn't make sense. > > I would not agree with that. Mockito can be easily used with TestNG as well as > probably with any other testing framework/library for Java. > JUnit is only internally used to run Mockito's own tests during a build > process. This is not exactly true. While I don't agree with the above logic as well (I would include junit4 as runtime dependency), it is true that Mockito provides a bunch of classes that subclass org.junit.runner.Runner (look into org.mockito.runners). This means that junit is needed at compile time, and if a user uses one of those classes, even at runtime. The point is that if a user depends on mockito and uses one of those classes, he will also need to depend on junit as well, that is probably why junit4 is not in Mockito's own dependency list. The question here is: should we fix that? should we try to get upstream to fix that? /Roman
I've been using Mockito for years and generally it's not a problem. If you would like to use MockitoJUnitRunner in your code then you have to add JUnit dependency to your project anyway to use JUnit itself (e.g. in tests that doesn't use Mockito). With TestNG it doesn't make sense to use JUnit runner and you don't reference to those classes in your code (mocks can be also initialized using MockitoAnnotations.initMocks()). I would treat JUnit as an optional dependency (a little bit like a plugin which could be enabled if a library/jar is provided). If the end user (some application) needs JUnit then it has to add it manually as a dependency. If uses TestNG or something else there is no side effects. I would leave BuildRequires - it's required to build Mockito itself. It is from a Java programmer point of view. I've never packaged Java based application for Fedora. You can ask Szczepan and others on the mailing list, but I doubt that JUnit would be added as a required dependency. I would force non-junit users to excluded it.
(In reply to comment #9) > Thanks Alexander for the review! I fixed the Issues you mentioned and uploaded > a new package: > > SPEC: http://icedrobot.de/~roman/mockito/2/mockito.spec > SRPM: http://icedrobot.de/~roman/mockito/2/mockito-1.9.0-2.fc16.src.rpm > Thanks for fixing the issues. It looks good now. > I also have a number of additional questions: > - The included Maven pom is lacking a bunch of dependencies: > - cglib (and maybe asm) because upstream repackages and includes them in > their own JAR. You definetely have to add cglib but I think that asm should not be there as the asm package is version 1.x while cglib is built against package objectweb-asm (version 3.x). If you think that cglib must Require objectweb-asm please open a bug against cglib for that. About cglib not shipping pom I see /usr/share/maven-poms/JPP-cglib.pom is shipped with it. > - JUnit4 because users of Mockito certainly must include that otherwise > Mockito doesn't make sense. From the rest of the comments it looks like this is optional dependency. As RPM is not supporting soft dependencies I think that the package maintainer (you Roman) is the one to decide whether to add this require or not. Once the cglib dependency is added to the pom I'll approve.
> Thanks for fixing the issues. It looks good now. > > > I also have a number of additional questions: > > - The included Maven pom is lacking a bunch of dependencies: > > - cglib (and maybe asm) because upstream repackages and includes them in > > their own JAR. > > You definetely have to add cglib but I think that asm should not be there as > the asm package is version 1.x while cglib is built against package > objectweb-asm (version 3.x). If you think that cglib must Require objectweb-asm > please open a bug against cglib for that. About cglib not shipping pom I see > /usr/share/maven-poms/JPP-cglib.pom is shipped with it. Ah I see.. I was looking at F16 :-) > > - JUnit4 because users of Mockito certainly must include that otherwise > > Mockito doesn't make sense. > > From the rest of the comments it looks like this is optional dependency. As RPM > is not supporting soft dependencies I think that the package maintainer (you > Roman) is the one to decide whether to add this require or not. > > Once the cglib dependency is added to the pom I'll approve. I added the cglib dependency and uploaded new spec/srpm: http://icedrobot.de/~roman/mockito/3/mockito.spec http://icedrobot.de/~roman/mockito/3/mockito-1.9.0-3.fc16.src.rpm /Roman
Looks good. APPROVED
Sponsored.
New Package SCM Request ======================= Package Name: mockito Short Description: A Java mocking framework Owners: rkennke Branches: InitialCC: akurtakov
New Package SCM Request ======================= Package Name: mockito Short Description: A Java mocking framework Owners: rkennke Branches: InitialCC: akurtakov, java-sig
Git done (by process-git-requests).
Roman, Once you have your build in koji it's good to close this bug.
build in rawhide: http://koji.fedoraproject.org/koji/buildinfo?buildID=300942
Package Change Request ====================== Package Name: mockito New Branches: f17 Owners: rkennke InitialCC: java-sig
Package Change Request ====================== Package Name: mockito New Branches: epel7 Owners: mcpierce InitialCC: java-sig
Btw, the will be a new stable (and backward compatible) Mockito version release "very soon" (after almost 2 years), so maybe it would be good to wait for 1.10.0 to not freeze 1.9.5 in EPEL7.
How long is the wait? I ask because I'm totally blocked from pushing qpid-proton-java in EPEL7 until all of its dependencies (including mockito) are available in EPEL7. If 1.10.0 is backwarks compatible with 1.9.5 can't we just release 1.9.5 and then upgrade later? Let 1.10.0 only go to stable with karma votes?
The version is already "feature complete", but there are some continuous delivery related things in progress. I think the first part of September is pretty possible. 1.10.0 is the next version in the 1.x line and is planned to be backward compatible, so if this is not a problem for you to later upgrade 1.9.5 to 1.10.0 in EPEL7 that would be IMHO the best option.
(In reply to Marcin Zajaczkowski from comment #30) > The version is already "feature complete", but there are some continuous > delivery related things in progress. I think the first part of September is > pretty possible. > > 1.10.0 is the next version in the 1.x line and is planned to be backward > compatible, so if this is not a problem for you to later upgrade 1.9.5 to > 1.10.0 in EPEL7 that would be IMHO the best option. I'm fine with the option that gets mockito into EPEL sooner.
Package Change Request ====================== Package Name: mockito New Branches: epel7 Owners: rfenkhuber InitialCC: java-sig