Bug 472848
Summary: | Review Request: jeuclid - MathML rendering solution | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Brennan Ashton <bashton> |
Component: | Package Review | Assignee: | Dominik 'Rathann' Mierzejewski <dominik> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | alsadi, chitlesh, dominik, fedora-package-review, kwizart, mycae, notting |
Target Milestone: | --- | Flags: | dominik:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 3.1.3-9.fc10 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-02-24 20:58:16 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: | |||
Bug Depends On: | 453018, 473373, 473734 | ||
Bug Blocks: | 472639 |
Description
Brennan Ashton
2008-11-25 04:13:06 UTC
The koji scratch builds: http://koji.fedoraproject.org/koji/taskinfo?taskID=949406 Some quick notes: % rpmlint jeuclid-core-* jeuclid-core.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/jeuclid-core-3.1.3/NOTICE jeuclid-core.x86_64: W: incoherent-version-in-changelog 3.1.3 ['3.1.3-1.fc9', '3.1.3-1'] You need to put the package release in a changelog entry, too. jeuclid-core.x86_64: E: no-binary jeuclid-core-debuginfo.x86_64: E: empty-debuginfo-package 3 packages and 0 specfiles checked; 2 errors, 2 warnings. This should be a noarch package, because it contains no binaries (as rpmlint tells you). That'll also fix the empty debuginfo. Use BuildArch: noarch. BuildRequires: java-devel BuildRequires: java-1.6.0-openjdk-devel One of these is redundant. Please add comments to patches, i.e. # this patch fixes foo, Fedora specific Patch0: jeuclid-core-build.patch # this patch fixes bar, submitted upstream Patch1: jeuclid-core-FreeHep.patch I could sponsor you if you can demonstrate knowledge of our packaging guidelines, i.e. by submitting another package or two for review and doing a couple of package reviews. Thank you, I did not realize that I needed to run rpmlint on the rpm as well, I had run it on the SRPM and the spec. The changes are now made, and it compiles cleanly in koji. See: http://koji.fedoraproject.org/koji/taskinfo?taskID=951853 I have another ReviewRequest going here: https://bugzilla.redhat.com/show_bug.cgi?id=472724 that was off the wishlist. And I will start reviewing a few packages. Good, but you should post the link to the updated spec file and src.rpm after you've made the changes (also increase the Release: tag and describe your changes in %changelog). You still haven't addressed this: jeuclid-core.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/jeuclid-core-3.1.3/NOTICE It is admittedly minor, but please fix that by converting the file using tr, sed or perl in %prep. Remember to preserve the original file's timestamp using touch. I will try and complete the review tonight. (In reply to comment #4) > Good, but you should post the link to the updated spec file and src.rpm after > you've made the changes (also increase the Release: tag and describe your > changes in %changelog). Fixed. > > You still haven't addressed this: > jeuclid-core.noarch: W: wrong-file-end-of-line-encoding > /usr/share/doc/jeuclid-core-3.1.3/NOTICE > > It is admittedly minor, but please fix that by converting the file using tr, > sed or perl in %prep. Remember to preserve the original file's timestamp using > touch. Done. SRPM: http://bashton.fedorapeople.org/jeuclid-core-3.1.3-3.fc9.src.rpm SPEC: http://bashton.fedorapeople.org/jeuclid-core.spec #001 one line would do the job : sed -i 's/\r//' NOTICE #002: add a -verbose to ant: e.g ant -verbose #003: update the url of the source0 https://fedoraproject.org/wiki/Packaging/SourceURL #004: visual only: add more spaces between various sets of lines (In reply to comment #6) > #001 > one line would do the job : sed -i 's/\r//' NOTICE You can't preserve the timestamp that way, so no. > #002: > add a -verbose to ant: > e.g ant -verbose Produces copious output indeed. Won't hurt. > #003: update the url of the source0 > https://fedoraproject.org/wiki/Packaging/SourceURL Indeed. > #004: visual only: add more spaces between various sets of lines :) Full review, relevant items only (OK'd items omitted). MUST Items: - MUST: rpmlint must be run on every package. The output should be posted in the review. rpmlint output clean - MUST: The package must be named according to the Package Naming Guidelines . The project is named "jeuclid". Why is the package named jeuclid-core? The Java packaging guidelines (https://fedoraproject.org/wiki/Packaging/Java) say that you should follow the project's name and provide a symlink if the commonly used jar name is different. - MUST: The package must meet the Packaging Guidelines . Group: Applications/Text License: ASL 2.0 The above lines have trailing whitespace at the end. # this patch points the ant to the correct jars Patch0: jeuclid-core-build.patch You could use build-classpath or build-jar-repository instead of the above patch. # this patch removes FreeHep support as per the build README Patch1: jeuclid-core-FreeHep.patch You don't have to say "this patch", it's redundant. What you should say, instead, is whether or not the patches have been sent upstream (and if not, why). - MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines . The following files have no licensing information, please ask upstream to provide it (not a blocker). $ licensecheck.pl -r . | grep -v Apache ./jeuclid-core/src/test/java/net/sourceforge/jeuclid/test/LayoutTest.java: *No copyright* UNKNOWN ./jeuclid-core/src/test/java/net/sourceforge/jeuclid/test/ViewerTest.java: *No copyright* UNKNOWN ./jeuclid-core/src/test/java/net/sourceforge/jeuclid/test/ConverterTest.java: *No copyright* UNKNOWN ./jeuclid-core/src/test/java/net/sourceforge/jeuclid/test/DOMBuilderTest.java: *No copyright* UNKNOWN The rest seems to be under ASL 2.0, thus OK. - MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this. OK: ff3690e649bf0ead5fd2a03c732dc1ce jeuclid-parent-3.1.3-src.zip - MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. Refer to the Guidelines for examples. You should use %{_javadir}/%{name}-%{version}.jar instead of %{_javadir}/* in %files section. It'll let you detect stray files if they ever make their way into buildroot in some future release. (In reply to comment #7) > (In reply to comment #6) > > #001 > > one line would do the job : sed -i 's/\r//' NOTICE > > You can't preserve the timestamp that way, so no. That is why I did it that way > > > #002: > > add a -verbose to ant: > > e.g ant -verbose > > Produces copious output indeed. Won't hurt. Added > > > #003: update the url of the source0 > > https://fedoraproject.org/wiki/Packaging/SourceURL I looked at that, but it looks like when I tested it, it did a redirect on me. Fixed Now. > > Indeed. > > > #004: visual only: add more spaces between various sets of lines > > :) Done New files Spec URL: http://bashton.fedorapeople.org/jeuclid-core.spec SRPM URL: http://bashton.fedorapeople.org/jeuclid-core-3.1.3-4.fc9.src.rpm (In reply to comment #8) > - MUST: The package must be named according to the Package Naming Guidelines . > > The project is named "jeuclid". Why is the package named jeuclid-core? > > The Java packaging guidelines (https://fedoraproject.org/wiki/Packaging/Java) > say that you should follow the project's name and provide a symlink if the > commonly used jar name is different. > jeuclid is a larger project that contains more modules. This is the only core module. Should I package the project in one spec file and have the sub projects like core in it? http://jeuclid.sourceforge.net/jeuclid-core/index.html > - MUST: The package must meet the Packaging Guidelines . > > Group: Applications/Text > License: ASL 2.0 > > The above lines have trailing whitespace at the end. Fixed > # this patch points the ant to the correct jars > Patch0: jeuclid-core-build.patch > > You could use build-classpath or build-jar-repository instead of the above > patch. Yes and know, the main reason that I patch is that the build.xml file expects them to be symbolic linked to a lib folder in it that does not exist. It will not build if this is not there. I though while I was patching that, I might as well put the links to the jars in the file. I could do it with build-classpath, but the patch will have to stay for the lib dir issue. This is not really an upstream issue. > # this patch removes FreeHep support as per the build README > Patch1: jeuclid-core-FreeHep.patch > > You don't have to say "this patch", it's redundant. What you should say, > instead, is whether or not the patches have been sent upstream (and if not, > why). Took those extra words out. Added that FreeHep was an optional feature and would not be passed upstream. > > - MUST: The package must be licensed with a Fedora approved license and meet > the Licensing Guidelines . > > The following files have no licensing information, please ask upstream to > provide it (not a blocker). > $ licensecheck.pl -r . | grep -v Apache > ./jeuclid-core/src/test/java/net/sourceforge/jeuclid/test/LayoutTest.java: *No > copyright* UNKNOWN > ./jeuclid-core/src/test/java/net/sourceforge/jeuclid/test/ViewerTest.java: *No > copyright* UNKNOWN > ./jeuclid-core/src/test/java/net/sourceforge/jeuclid/test/ConverterTest.java: > *No copyright* UNKNOWN > ./jeuclid-core/src/test/java/net/sourceforge/jeuclid/test/DOMBuilderTest.java: > *No copyright* UNKNOWN > > The rest seems to be under ASL 2.0, thus OK. I will file an upstream bug and post. > > - MUST: The sources used to build the package must match the upstream source, > as provided in the spec URL. Reviewers should use md5sum for this task. If no > upstream URL can be specified for this package, please see the Source URL > Guidelines for how to deal with this. > > OK: > ff3690e649bf0ead5fd2a03c732dc1ce jeuclid-parent-3.1.3-src.zip > > - MUST: A package must own all directories that it creates. If it does not > create a directory that it uses, then it should require a package which does > create that directory. Refer to the Guidelines for examples. > > You should use > %{_javadir}/%{name}-%{version}.jar > instead of > %{_javadir}/* > in %files section. It'll let you detect stray files if they ever make their way > into buildroot in some future release. Fixed Sorry for the intermediate post, we had a mid-air bz collision. Spec URL: http://bashton.fedorapeople.org/jeuclid-core.spec SRPM URL: http://bashton.fedorapeople.org/jeuclid-core-3.1.3-5.fc9.src.rpm license bug filed: https://sourceforge.net/tracker/index.php?func=detail&aid=2352779&group_id=44862&atid=441104 I will package the other modules tonight as part of this so it will fit the naming correctly. I have changed it to the correct naming scheme. I am only going to package the core package as that is all that is needed right now, the others modules can be added later, but will require a lot of patching on the build and working with upstream to get around some font issues. Core uses ant while the rest uses mvn which adds another layer to the complexity as well. I was able to use build-jar-repository to work with there build.xml file so that patch has been removed. The other patch has to stay for FreeHep, the patch is right out of the readme for how to disable FreeHep. If someone wants to package FreeHep, then it can be added, but it is not really necessary. Hopefully this is the last bit: Spec URL: http://bashton.fedorapeople.org/jeuclid-core.spec SRPM URL: http://bashton.fedorapeople.org/jeuclid-core-3.1.3-6.fc9.src.rpm sorry the new spec is Spec URL: http://bashton.fedorapeople.org/jeuclid.spec per the naming scheme upstream license bug now fixed will be in 3.1.4. Thx for taking care of this package. Just one note: BuildRequires: java-devel >= 1:1.6.0 instead of BuildRequires: java-1.6.0-openjdk-devel There is a typo in the last changelog entry that weren't update to release 6. I'm trying to do runtime test now. Nice works This build contains more fixes, as well as including the jeuclid-fod and jeuclid-mathviewer subpackages. I cannot build the jeuclid-cli until commons-cli is updated bug 473373 . I have had to write a new build script based on the Debian one as the ones included do not work well with fedora package build structure. I still need to do the GUI stuff for jeclid-mathviewer. SRPM: http://bashton.fedorapeople.org/jeuclid-3.1.3-7.fc9.src.rpm SPEC: http://bashton.fedorapeople.org/jeuclid.spec I did a runtime test with jeuclid-mathviewer and it did not look all that great i got: http://bashton.fedorapeople.org/example5.png When it should look like http://jeuclid.sourceforge.net/images/example5.png Nicolas did your runtime test work? This problem could be due to fonts or the viewer. I have not written a test that goes directly to the jeuclid-core, and as I said the command line tools are dead until commons-cli is updated in fedora. Upstream also did not put the javadocs in the build script, while it is in the code, so I am adding that in as well. Actually, instead of runtime test, I've failed at scilab buildtime with a missing Symbol at configure check (whereas jeuclid-core.jar is already available in the classpath): checking jeuclid-core... no configure: error: Could not find or use the Java package/jar jeuclid-core used by MathML rendering solution (looking for package net.sourceforge.jeuclid.MathBase) (In reply to comment #19) > Actually, instead of runtime test, I've failed at scilab buildtime with a > missing Symbol at configure check (whereas jeuclid-core.jar is already > available in the classpath): > checking jeuclid-core... no > configure: error: Could not find or use the Java package/jar jeuclid-core used > by MathML rendering solution (looking for package > net.sourceforge.jeuclid.MathBase) Did you try the latest SRPM I posted? It has the system links for jeuclid-core.jar. It looks like the rendering error is due to an issue with OpenJDK. I am talking with upstream about it. (In reply to comment #17) > This build contains more fixes, as well as including the jeuclid-fod and > jeuclid-mathviewer subpackages. I cannot build the jeuclid-cli until > commons-cli is updated bug 473373 . I have had to write a new build script > based on the Debian one as the ones included do not work well with fedora > package build structure. I still need to do the GUI stuff for > jeclid-mathviewer. > > SRPM: http://bashton.fedorapeople.org/jeuclid-3.1.3-7.fc9.src.rpm > SPEC: http://bashton.fedorapeople.org/jeuclid.spec This specfile is older than the one in the src.rpm. Requires: fop = 0.95 Does it work only with this version? Will it have to be rebuilt when fop is upgraded to a newer version? (cd $RPM_BUILD_ROOT%{_javadir} && for jar in *-%{version}*; do ln -sf ${jar} `echo $jar| sed "s|-%{version}||g"`; done) This could be rewritten as: pushd $RPM_BUILD_ROOT%{_javadir} for jar in *-%{version}*; do ln -sf ${jar} `echo $jar| sed "s|-%{version}||g"` done popd Which is more readable IMHO, but I'm not insisting on it. Also, the usage of '|' in sed expression is a bit unusual. I suggest plain slash ('/') instead. It looks very nice otherwise. (In reply to comment #21) > (In reply to comment #17) > > This build contains more fixes, as well as including the jeuclid-fod and > > jeuclid-mathviewer subpackages. I cannot build the jeuclid-cli until > > commons-cli is updated bug 473373 . I have had to write a new build script > > based on the Debian one as the ones included do not work well with fedora > > package build structure. I still need to do the GUI stuff for > > jeclid-mathviewer. > > > > SRPM: http://bashton.fedorapeople.org/jeuclid-3.1.3-7.fc9.src.rpm > > SPEC: http://bashton.fedorapeople.org/jeuclid.spec > > This specfile is older than the one in the src.rpm. looks like I forgot to dump the spec file over sorry for that. > > Requires: fop = 0.95 > > Does it work only with this version? Will it have to be rebuilt when fop is > upgraded to a newer version? > Yes, upstream highly recommends this as this fop plugin development is so closely tied to fop development. > (cd $RPM_BUILD_ROOT%{_javadir} && for jar in *-%{version}*; do ln -sf ${jar} > `echo $jar| sed "s|-%{version}||g"`; done) > > This could be rewritten as: > pushd $RPM_BUILD_ROOT%{_javadir} > for jar in *-%{version}*; do > ln -sf ${jar} `echo $jar| sed "s|-%{version}||g"` > done > popd > > Which is more readable IMHO, but I'm not insisting on it. Also, the usage of > '|' in sed expression is a bit unusual. I suggest plain slash ('/') instead. > > It looks very nice otherwise. It is going to be a busy week for me, but I will try to get the latest out with the GUI and Javadocs stuff done. Ok the rendering issue is fixed in F10, so it looks like soon as commons-cli is updated I will be able to roll out a srpm that has the .desktop javadocs and wrappers done. Ok I think I am done, I have left out the javadocs for now, they are only partially done upstream, and are going to take a while to format in the build script. I will add them in as an update when they are ready as they are not a requirement for packaging. This SRPM will not compile until Bug 453018 is fixed, you can use the SRPM I posted there and it will work. So far that maintainer has not responded to emails or the bug. He is in Europe so it is hard to find him on IRC during my time. This will run on F9, but render correctly only F10, there was no response to a question asking if the openjdk fix will be applied to F9 in the future. And the files: SRPM: http://bashton.fedorapeople.org/jeuclid-3.1.3-8.fc9.src.rpm SPEC: http://bashton.fedorapeople.org/jeuclid.spec I just got an email back from the jakarta-commons-cli maintainer, and he will be updating to 1.1 on Monday. (In reply to comment #25) > I just got an email back from the jakarta-commons-cli maintainer, and he will > be updating to 1.1 on Monday. Shouldn't BuildRequires: jakarta-commons-cli be amended to require >= 1.1, too? Or is it OK to build with 1.0 as long as 1.1 is present at runtime? $ rpmlint /var/lib/mock//fedora-rawhide-i386/result jeuclid-cli.noarch: W: no-documentation jeuclid-mathviewer.noarch: W: no-documentation jeuclid-fop.noarch: W: no-documentation 5 packages and 0 specfiles checked; 0 errors, 3 warnings. jeuclid-mathviewer.desktop ... Icon=/usr/share/icons/hicolor/48x48/apps/jeuclid_48x48.png Icon tag with just plain filename without extension is preferred, but the above is perfectly acceptable as well. ... Categories=Graphics;Math;Science Categories should be semicolon-terminated (;). License: ASL 2.0 and SPL Where did "SPL" come from? %package mathviewer Summary: Viewer for MathML files Group: Applications/Publishing Requires: %{name} = %{version}-%{release} This is missing Requires: hicolor-icon-theme, because you're putting the icon in %{_datadir}/icons/hicolor/* . cp src/icons/jeuclid_48x48.png $RPM_BUILD_ROOT/%{_datadir}/icons/hicolor/48x48/apps/ Please use cp -p to preserve the timestamp. %attr(0755,root,root) %{_bindir}/jeuclid-mathviewer and %attr(0755,root,root) %{_bindir}/jeuclid-cli %attr is redundant, you're already installing these scripts with install -pm755. You are missing %post/%postun scripts for updating GTK icon cache, see: https://fedoraproject.org/wiki/PackagingDrafts/ScriptletSnippets#GTK.2B_icon_cache I've just sponsored Brennan, lifting NEEDSPONSOR block. (In reply to comment #27) > $ rpmlint /var/lib/mock//fedora-rawhide-i386/result > jeuclid-cli.noarch: W: no-documentation > jeuclid-mathviewer.noarch: W: no-documentation > jeuclid-fop.noarch: W: no-documentation > 5 packages and 0 specfiles checked; 0 errors, 3 warnings. > > jeuclid-mathviewer.desktop > ... > Icon=/usr/share/icons/hicolor/48x48/apps/jeuclid_48x48.png > > Icon tag with just plain filename without extension is preferred, but the above > is perfectly acceptable as well. That is what I thought, but I could not seem to get it to find it, might have been related to the cache that you were talking about > ... > Categories=Graphics;Math;Science > > Categories should be semicolon-terminated (;). > > > License: ASL 2.0 and SPL > > Where did "SPL" come from? There is code in Patch 1 that fixes some Apple requirements that is SPL licensed > > %package mathviewer > Summary: Viewer for MathML files > Group: Applications/Publishing > Requires: %{name} = %{version}-%{release} > > This is missing Requires: hicolor-icon-theme, because you're putting the icon > in %{_datadir}/icons/hicolor/* . Added > > cp src/icons/jeuclid_48x48.png > $RPM_BUILD_ROOT/%{_datadir}/icons/hicolor/48x48/apps/ > > Please use cp -p to preserve the timestamp. > Fixed > %attr(0755,root,root) %{_bindir}/jeuclid-mathviewer > and > %attr(0755,root,root) %{_bindir}/jeuclid-cli > > %attr is redundant, you're already installing these scripts with install > -pm755. Fixed > > You are missing %post/%postun scripts for updating GTK icon cache, see: > https://fedoraproject.org/wiki/PackagingDrafts/ScriptletSnippets#GTK.2B_icon_cache Added Also added the => 1.1 on jakara-commons-cli in build. This will only build for F10 there are two other packages that are not fixing bugs in F9 that this depends on. SRPM: http://bashton.fedorapeople.org/jeuclid-3.1.3-9.fc10.src.rpm SPEC: http://bashton.fedorapeople.org/jeuclid.spec Anything else? No, looks fine now. APPROVED. Also, removing FE-NEEDSPONSOR as you are sponsored already. New Package CVS Request ======================= Package Name: jeuclid Short Description: MathML rendering solution Owners: bashton Branches: F-10 cvs done. This package has been imported only in F-10 but failed. Any Reason ? A scratch build succeed targeted as dist-f11 http://koji.fedoraproject.org/koji/taskinfo?taskID=1074452 The holdup has been jakarta-commons-cli 1.1 needs to be pushed to stable, it has been in test for a few weeks now (before that the maintainer forgot to push it for a few weeks into that). If you can give jakarta-commons-cli karma love we can get this on on its way. Ok, done. But can we have the Rawhide build at least ? done. http://koji.fedoraproject.org/koji/taskinfo?taskID=1078404 One more karma for jakarta-commons-cli to go live in F10. Ping? jeuclid-3.1.3-9.fc10 has been pushed to the Fedora 10 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update jeuclid'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-1249 jeuclid-3.1.3-9.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report. |