Spec URL: http://resources.ovirt.org/repos/staging/ebay-cors-filter.spec SRPM URL: http://resources.ovirt.org/repos/staging/ebay-cors-filter-1.0.1-1.fc22.src.rpm Description: CORS (Cross Origin Resource Sharing) is a mechanism supported by W3C to enable cross origin requests in web-browsers. CORS requires support from both browser and server to work. This is a Java servlet filter implementation of server-side CORS for web containers such as Apache Tomcat. Fedora Account System Username: sbonazzo
I will review the package.
Please change in %files the line: %{_javadir}/%{name} to have a %dir prefix. It is usually a good idea to also add a '/' suffix to make it easier to spot it is a directory, but with %dir that is redundant. I do not see much of a reason to also add the LICENSE file to the javadoc package. Sure there may be cases where one installs only the javadoc packages, but those should be quite uncommon. But removing it is optional. The LICENSE and README.md file have DOS style line endings, please correct it, for example: for doc in README.md LICENSE; do sed "s|\r||g" $doc > $doc.new && \ touch -r $doc $doc.new && \ mv $doc.new $doc done I think cors-flowchart.png is more useful than README.md in %doc. Now the bad news :) For the first package it is asking a lot to have the package approved, without running any tests: # Tests don't compile with servlet 3.x, as the mock objects miss some of the # required methods: Please explain why it cannot be adjusted to run tests. Note that tests is the best way to know the package is at least partially functional.
tomcat-servlet-3.0-api no more exist in rawhide, the new tomcat-servlet-3.1-api don't provides anymore this alias. you must use: BuildRequires: mvn(javax.servlet:servlet-api) the proper name (groupId:artifactId) of the BR is the pom file or the proper servlet-api version BuildRequires: mvn(org.jboss.spec.javax.servlet:jboss-servlet-api_2.5_spec) using %pom_remove_dep javax.servlet:servlet-api %pom_add_dep org.jboss.spec.javax.servlet:jboss-servlet-api_2.5_spec:1.0.1.Final:provided seem you have omit also this BR BuildRequires: mvn(org.sonatype.oss:oss-parent:pom:) sorry for the noise
(In reply to Paulo Andrade from comment #2) > Please change in %files the line: > %{_javadir}/%{name} > to have a %dir prefix. It is usually a good idea to > also add a '/' suffix to make it easier to spot it > is a directory, but with %dir that is redundant. with the newer javapackages-tools/maven-local %dir %{_javadir}/%{name} is no more required (for F22, and rawhide is redundant) > I do not see much of a reason to also add the LICENSE > file to the javadoc package. Sure there may be cases > where one installs only the javadoc packages, but those > should be quite uncommon. But removing it is optional. "License file installed when any subpackage combination is installed." https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#Subpackage_Licensing a javadoc sub package should be consider as "independent of any base package" and then include the license files other issues NOTICE file is part of the license and must be installe in both packages with macro %license e.g. %license LICENSE NOTICE java guideline is available here: https://fedoraproject.org/wiki/Packaging:Java https://fedorahosted.org/released/javapackages/doc/
Created attachment 1010804 [details] servlet 3.1 apis support
Created attachment 1010805 [details] servlet 3.1 apis support
(In reply to gil cattaneo from comment #6) > Created attachment 1010805 [details] > servlet 3.1 apis support Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=9413865
Created attachment 1010806 [details] spec file
Created attachment 1010807 [details] spec file
Thanks Gil for the fixes! So if I've understood correctly, the only pending changes are: - add cors-flowchart.png to %doc - change new lines in README.md, LICENSE and NOTICE Everything else has been already addressed by Gil right?
(In reply to gil cattaneo from comment #4) > (In reply to Paulo Andrade from comment #2) > with the newer javapackages-tools/maven-local > %dir %{_javadir}/%{name} > is no more required (for F22, and rawhide is redundant) for F22 have some doubt... ask to mizdebsk, Mikolaj Izdebski, @ fedora-java irc channel
I suggest you run fedora-review yourself also, so you can test before asking for a new formal review. For rawhide it may be a bit trick right now, and may need to manually install an older openjdk until it is fixed in rawhide. For other fedora releases, it depends on what releases you want the package, and in that case may need to have a different set of patches, depending on what servlet is available.
Addressed pending comments. On Fedora 22 %dir was required in order to own all created directories. Built with mock on Fedora 22. Tested with rpmlint and fedora-review on Fedora 22. spec: http://resources.ovirt.org/repos/staging/ebay-cors-filter.spec srpm: http://resources.ovirt.org/repos/staging/ebay-cors-filter-1.0.1-3.fc22.src.rpm
For formal information, this is fedora-review output with some redundancies removed. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - 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. Note: Cannot find LICENSE in rpm(s) See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text - This seems like a Java package, please install fedora-review-plugin-java to get additional checks ===== MUST items ===== Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Apache (v2.0)". Detailed output of licensecheck in /home/pcpa/1208816 -ebay-cors-filter/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [x]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [x]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. [x]: Packages must not store files under /srv, /opt or /usr/local [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package do not use a name that already exist [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. Java: [x]: Bundled jar/class files should be removed before build ===== SHOULD items ===== Generic: [x]: 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]: Final provides and requires are sane (see attachments). [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in ebay-cors- filter-javadoc [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Sources can be downloaded from URI in Source: tag [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: Dist tag is present (not strictly required in GL). [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
The package is approved!
Sandro, be welcome as a fedora packager! This time using my hatter account not by mistake :) If you need any further assistance feel free to ping me, I am pcpa in freenode, and in rhat irc. Or by email, short aliases are pcpa (for gmail acccount) and pandrade or pcpa for my hatter account. I will do my duty as sponsor watching you in bugzilla, but I am sure, actually I have more to learn from you! Your required acls should be available now, or shortly, if you did not already, please run fedora-packager-setup, and the starting point instructions to import and build the package are at http://fedoraproject.org/wiki/Join_the_package_collection_maintainers
Thanks Paulo!
New Package SCM Request ======================= Package Name: ebay-cors-filter Short Description: eBay CORS filter Upstream URL: https://github.com/eBay/cors-filter Owners: sbonazzo Branches: fc22 InitialCC: jhernand
Git done (by process-git-requests). The disttag is fc22 but the branch is f22. Yeah, I know. . .
*** Bug 1186751 has been marked as a duplicate of this bug. ***
ebay-cors-filter-1.0.1-3.fc22 has been submitted as an update for Fedora 22. https://admin.fedoraproject.org/updates/ebay-cors-filter-1.0.1-3.fc22
ebay-cors-filter-1.0.1-3.fc22 has been pushed to the Fedora 22 stable repository.