Bug 1438673

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=18776746

Updated to only build for x86 and x86_64, since the build will fail on other platforms. Would be nice to work with upstream to test on other platforms.

Spec URL: https://jonny.fedorapeople.org/openjfx.spec
SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-4.b00.fc25.src.rpm

Can you put an explanation in the SPEC FILE why you remove these source codes

rm -rf modules/web/src/*
rm -rf modules/builders/src/main/java/javafx/scene/web
rm -rf modules/media/src/*
rm -rf modules/builders/src/main/java/javafx/scene/media
rm -f  modules/jmx/src/main/java/com/oracle/javafx/jmx/SGMXBeanImpl.java
rm -f  modules/jmx/src/main/java/com/oracle/javafx/jmx/MXExtensionImpl.java

libjpeg's bundle?
rm -rf modules/graphics/src/main/native-iio/libjpeg*

Some patches for other arches are available here:
https://anonscm.debian.org/cgit/pkg-java/openjfx.git/tree/debian/patches/fix-arm32-build.patch
https://anonscm.debian.org/cgit/pkg-java/openjfx.git/tree/debian/patches/fix-arm64-build.patch

Are not applicable?

Is available a new release: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/abcaf2cb0939
Please, considering upgrade

Please, add in %setup section:
find -name "*.class" -delete
find -name "*.jar" -delete

Suggestion. Maybe you could use, instead of SOURCE2:

cat > gradle.properties << EOF
 CONTENTS ...
EOF

I tried running try-fedora-review (see https://pagure.io/FedoraReview) and then installed the resulting rpm.

I then tried building Kawa (https://www.gnu.org/software/kawa/) using /usr/bin/javac - and that failed to find the jfx classes.

As a work-around I created in /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.121-11.b14.fc27.x86_64/jre/lib/ext a link to /usr/lib/jvm/openjfx/rt/lib/ext/jfxrt.jar . That allowed the compile to succeed.  However, the resulting executable failed to run.  I didn't continue, but it's plausible that java couldn't find the right shared libraries.

Since openjdk is installed in a versioned directory (with a nest of alternatives links), we have to install openjfx using a compatible layout.

The created openjfx-javadoc-8.0.152-4.b00.fc27.x86_64.rpm only creates files in /usr/share/javadoc/openjfx.  Which means the package should be noarch:

!]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
     Note: Arch-ed rpms have a total of 58439680 bytes in /usr/share
     openjfx-javadoc-8.0.152-4.b00.fc27.x86_64.rpm:58378240
     See:
     http://fedoraproject.org/wiki/Packaging:ReviewGuidelines#Package_Review_Guidelines

(In reply to Per Bothner from comment #5)
> The created openjfx-javadoc-8.0.152-4.b00.fc27.x86_64.rpm only creates files
> in /usr/share/javadoc/openjfx.  Which means the package should be noarch:
> 
> !]: Large data in /usr/share should live in a noarch subpackage if package
>      is arched.
>      Note: Arch-ed rpms have a total of 58439680 bytes in /usr/share
>      openjfx-javadoc-8.0.152-4.b00.fc27.x86_64.rpm:58378240
>      See:
>     
> http://fedoraproject.org/wiki/Packaging:
> ReviewGuidelines#Package_Review_Guidelines

Other referencies are available here:
https://fedora-java.github.io/howto/latest/
https://fedoraproject.org/wiki/Packaging:Java
https://fedoraproject.org/wiki/Packaging:Java#Javadoc_installation

Thanks for the feedback, will fix those later the comming week

(In reply to Per Bothner from comment #4)
> I tried running try-fedora-review (see https://pagure.io/FedoraReview) and
> then installed the resulting rpm.
> 
> I then tried building Kawa (https://www.gnu.org/software/kawa/) using
> /usr/bin/javac - and that failed to find the jfx classes.
> 
> As a work-around I created in
> /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.121-11.b14.fc27.x86_64/jre/lib/ext a
> link to /usr/lib/jvm/openjfx/rt/lib/ext/jfxrt.jar . That allowed the compile
> to succeed.  However, the resulting executable failed to run.  I didn't
> continue, but it's plausible that java couldn't find the right shared
> libraries.
> 
> Since openjdk is installed in a versioned directory (with a nest of
> alternatives links), we have to install openjfx using a compatible layout.

There have been a long discussion at bug 1145303, where the plan is to have two packages, this one with the binaries and another with the symbolic links that is part of an openjdk sub-package.

It would be great if you can test Kawa by following the steps described in /usr/share/doc/openjfx/README.fedora

(In reply to Jonny Heggheim from comment #8)

When I install the openjfx package, I expect it to work.  I do not expect to have to look for an obscure README, and have to install some other mysterious package.

Whatever happens behind the scenes with magic links and alternates is one thing, but I as a user should not have to do anything more complicated than "sudo dnf install openjfx".

(In reply to Per Bothner from comment #9)
> When I install the openjfx package, I expect it to work.  I do not expect to
> have to look for an obscure README, and have to install some other
> mysterious package.

This README is intended for Fedora packagers and not end users. 

> Whatever happens behind the scenes with magic links and alternates is one
> thing, but I as a user should not have to do anything more complicated than
> "sudo dnf install openjfx".

The openjdk sub-package will provide this functionality, but that package have not been made.

Please take part in bug 1145303 if you have found a better way to package openjfx.

(In reply to Jonny Heggheim from comment #8)
> It would be great if you can test Kawa by following the steps described in
> /usr/share/doc/openjfx/README.fedora

I did that.  Results:

I was able to build Kawa from source, with the --with-javafx configure flag (which requires some javafx packages).

I was able to run some simple Kawa-Scheme JavaFX GUI demo scripts.

I was not able to use the -wjavafx option.  That should open a REPL console (using DomTerm http://domterm.org/) in a fresh WebView:

$ bin/kawa -wjavafx
java.lang.NoClassDefFoundError: javafx/scene/web/WebView
	at org.domterm.javafx.WebTerminalApp.createScene(WebTerminalApp.java:68)
	at org.domterm.javafx.WebTerminalApp.start(WebTerminalApp.java:154)
	at com.sun.javafx.application.LauncherImpl.lambda$launchApplication1$8(LauncherImpl.java:863)
	at com.sun.javafx.application.PlatformImpl.lambda$runAndWait$7(PlatformImpl.java:326)
	at com.sun.javafx.application.PlatformImpl.lambda$null$5(PlatformImpl.java:295)
	at java.security.AccessController.doPrivileged(Native Method)
	at com.sun.javafx.application.PlatformImpl.lambda$runLater$6(PlatformImpl.java:294)
	at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
	at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
	at com.sun.glass.ui.gtk.GtkApplication.lambda$null$5(GtkApplication.java:139)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.ClassNotFoundException: javafx.scene.web.WebView
	at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	... 11 more
Exception in Application start method
Application stop called

The same bin/kawa -wjavafx command, using the same Kawa build, worked fine using with Oracle's JDK in the PATH.

(In reply to Per Bothner from comment #11)
> I did that.  Results:
> 
> I was able to build Kawa from source, with the --with-javafx configure flag
> (which requires some javafx packages).
> 
> I was able to run some simple Kawa-Scheme JavaFX GUI demo scripts.
> 
> I was not able to use the -wjavafx option.  That should open a REPL console
> (using DomTerm http://domterm.org/) in a fresh WebView:
> 
> $ bin/kawa -wjavafx
> java.lang.NoClassDefFoundError: javafx/scene/web/WebView
> 	at org.domterm.javafx.WebTerminalApp.createScene(WebTerminalApp.java:68)
> 	at org.domterm.javafx.WebTerminalApp.start(WebTerminalApp.java:154)

The web (and media) module is not included in the package because of missing dependencies. This needs to be documented.

is the https://jonny.fedorapeople.org/openjfx-8.0.152-4.b00.fc25.src.rpm semi stable? Can I add symliks to openjdk8 in rawhide?

Btw - the .spec of yours - javafxpackager and javapackager are not on path. Is it intentional?

If  not, I would recommend to link them to %{_bindir}/

(In reply to jiri vanek from comment #13)
> is the https://jonny.fedorapeople.org/openjfx-8.0.152-4.b00.fc25.src.rpm
> semi stable? Can I add symliks to openjdk8 in rawhide?

Technically they should be semi stable, but there might be political reasons to change location. Lack of time have stopped me from doing all the changes based on the feedback from Per and Gil

(In reply to jiri vanek from comment #14)
> Btw - the .spec of yours - javafxpackager and javapackager are not on path.
> Is it intentional?
> 
> If  not, I would recommend to link them to %{_bindir}/

Not intentional, but I know that one of them are deprecated.

(In reply to Jonny Heggheim from comment #15)
> (In reply to jiri vanek from comment #13)
> > is the https://jonny.fedorapeople.org/openjfx-8.0.152-4.b00.fc25.src.rpm
> > semi stable? Can I add symliks to openjdk8 in rawhide?
> 
> Technically they should be semi stable, but there might be political reasons
> to change location. Lack of time have stopped me from doing all the changes

I see:(
> based on the feedback from Per and Gil

(In reply to Jonny Heggheim from comment #16)
> (In reply to jiri vanek from comment #14)
> > Btw - the .spec of yours - javafxpackager and javapackager are not on path.
> > Is it intentional?
> > 
> > If  not, I would recommend to link them to %{_bindir}/
> 
> Not intentional, but I know that one of them are deprecated.

So at least second one?

(In reply to jiri vanek from comment #13)
> is the https://jonny.fedorapeople.org/openjfx-8.0.152-4.b00.fc25.src.rpm
> semi stable? Can I add symliks to openjdk8 in rawhide?

Newer version (still WIP) is located here: https://jonny.fedorapeople.org/openjfx-8.0.152-5.b02.fc25.src.rpm

Complete!
Finish: build setup for openjfx-8.0.152-5.b02.fc25.src.rpm
Start: rpmbuild openjfx-8.0.152-5.b02.fc25.src.rpm
Building target platforms: x86_64
Building for target x86_64
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.ChQeqH
+ umask 022
+ cd /builddir/build/BUILD
+ cd /builddir/build/BUILD
+ rm -rf rt-8u152-b02
+ /usr/bin/bzip2 -dc /builddir/build/SOURCES/8u152-b02.tar.bz2
+ /usr/bin/tar -xof -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd rt-8u152-b02
+ /usr/bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ /usr/bin/git init -q
/var/tmp/rpm-tmp.ChQeqH: line 42: /usr/bin/git: No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.ChQeqH (%prep)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.ChQeqH (%prep)
ERROR: Exception(openjfx-8.0.152-5.b02.fc25.src.rpm) Config(fedora-rawhide-x86_64) 2 minutes 19 seconds
INFO: Results and/or logs in: /var/lib/mock/fedora-rawhide-x86_64/result
ERROR: Command failed. See logs for output.

I guess it is due to new autosetup macro at %autosetup -S git -n rt-8u152-b02

So buildrequires git is still mandatory.

Also the link(s) to bindir keeps missing.

ExclusiveArch:  x86 x86_64

 intels only  is also upstream verdict?

(In reply to jiri vanek from comment #19)
> RPM build errors:
>     Bad exit status from /var/tmp/rpm-tmp.ChQeqH (%prep)
> ERROR: Exception(openjfx-8.0.152-5.b02.fc25.src.rpm)
> Config(fedora-rawhide-x86_64) 2 minutes 19 seconds
> INFO: Results and/or logs in: /var/lib/mock/fedora-rawhide-x86_64/result
> ERROR: Command failed. See logs for output.
> 
> I guess it is due to new autosetup macro at %autosetup -S git -n rt-8u152-b02
> 
> So buildrequires git is still mandatory.

Thanks, I did not know that

(In reply to jiri vanek from comment #20)
> Also the link(s) to bindir keeps missing.

Yes, on my TODO

(In reply to jiri vanek from comment #21)
> ExclusiveArch:  x86 x86_64
> 
>  intels only  is also upstream verdict?

Yes, upstream checks during build:


> } else if (IS_LINUX && OS_ARCH != "i386" && OS_ARCH != "amd64") {
>    throw new Exception("Unknown and unsupported build architecture: $OS_ARCH")
> }

Debian have patches for fixing #IFDEF's in the source code. I will not have time to maintain a large set of patches, but I welcome any co-maintainers to do that.

(In reply to Jonny Heggheim from comment #24)
> (In reply to jiri vanek from comment #21)
> > ExclusiveArch:  x86 x86_64
> > 
> >  intels only  is also upstream verdict?
> 
> Yes, upstream checks during build:
> 
> 
> > } else if (IS_LINUX && OS_ARCH != "i386" && OS_ARCH != "amd64") {
> >    throw new Exception("Unknown and unsupported build architecture: $OS_ARCH")
> > }
> 
> Debian have patches for fixing #IFDEF's in the source code. I will not have
> time to maintain a large set of patches, but I welcome any co-maintainers to
> do that.

Sure thanx!

I hope me an Mario (especially Mario, for some reason he likes javafx:)  will be able to help.

I have updated the SPEC to include all comments, ready for another review:

Spec URL: https://jonny.fedorapeople.org/openjfx.spec
SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-6.b03.fc25.src.rpm

(In reply to Jonny Heggheim from comment #26)
> I have updated the SPEC to include all comments, ready for another review:
> 
> Spec URL: https://jonny.fedorapeople.org/openjfx.spec
> SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-6.b03.fc25.src.rpm

Jsut quick check, and looks good. Tahnx!

(In reply to jiri vanek from comment #27)
> Jsut quick check, and looks good. Tahnx!

Great, just let me know when you have a symlink/subpackage that I can test building/installing.

(In reply to Jonny Heggheim from comment #28)
> (In reply to jiri vanek from comment #27)
> > Jsut quick check, and looks good. Tahnx!
> 
> Great, just let me know when you have a symlink/subpackage that I can test
> building/installing.

Here you go:
scratch build for rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=19487720
srpm+patch+f24binaries: https://jvanek.fedorapeople.org/jfxsupport/v1/

The fx subpackages currently dont have nay provides/requires on yours, to simplify   testing.
I expect to make them depend on yours package in future. Nothing else (unless agreed)

I had split the links to runtime and devel, don't you think it have sense to split also yours package?

Thoughts welcomed, thanx!

(In reply to jiri vanek from comment #29)
> I had split the links to runtime and devel, don't you think it have sense to
> split also yours package?

I think it make sense and it should be easy to split the package. I have not slitted it yet to avoid adding complexity.

Can you start a discussion on bug 1145303?

(In reply to Jonny Heggheim from comment #30)
> (In reply to jiri vanek from comment #29)
> > I had split the links to runtime and devel, don't you think it have sense to
> > split also yours package?
> 
> I think it make sense and it should be easy to split the package. I have not
> slitted it yet to avoid adding complexity.
> 
> Can you start a discussion on bug 1145303?

Done: https://bugzilla.redhat.com/show_bug.cgi?id=1145303#c93

Btw.. I can see there was already several various commentators, but no one took the bug.  As I myself participate in decisions and java-1.8.0-openjdk I dont wont to take it unless there are other options....

(In reply to jiri vanek from comment #31)
> Done: https://bugzilla.redhat.com/show_bug.cgi?id=1145303#c93

Thanks.

> Btw.. I can see there was already several various commentators, but no one
> took the bug.  As I myself participate in decisions and java-1.8.0-openjdk I
> dont wont to take it unless there are other options....

I updated the .spec and srpm file with a -devel sub package, let me know how it fits.

Spec URL: https://jonny.fedorapeople.org/openjfx.spec
SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-7.b03.fc25.src.rpm

(In reply to Jonny Heggheim from comment #32)
> (In reply to jiri vanek from comment #31)
> > Done: https://bugzilla.redhat.com/show_bug.cgi?id=1145303#c93
> 
> Thanks.
> 
> > Btw.. I can see there was already several various commentators, but no one
> > took the bug.  As I myself participate in decisions and java-1.8.0-openjdk I
> > dont wont to take it unless there are other options....
> 
> I updated the .spec and srpm file with a -devel sub package, let me know how
> it fits.
> 
> Spec URL: https://jonny.fedorapeople.org/openjfx.spec
> SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-7.b03.fc25.src.rpm

Hi,
I'm unable to build. Got this error:

+ /usr/lib/rpm/find-debuginfo.sh --strict-build-id -m --run-dwz --dwz-low-mem-die-limit 10000000 --dwz-max-die-limit 110000000 /home/mvala/rpmbuild/BUILD/rt-8u152-b03
extracting debug info from /home/mvala/rpmbuild/BUILDROOT/openjfx-8.0.152-7.b03.fc26.x86_64/usr/lib/jvm/openjfx/rt/lib/amd64/libglass.so
*** ERROR: No build ID note found in /home/mvala/rpmbuild/BUILDROOT/openjfx-8.0.152-7.b03.fc26.x86_64/usr/lib/jvm/openjfx/rt/lib/amd64/libglass.so
error: Bad exit status from /var/tmp/rpm-tmp.waBuNH (%install)

Do you have some more information? It builds on koji https://koji.fedoraproject.org/koji/taskinfo?taskID=19564638

I can build it in mock. So I guess it's ok.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Package installs properly.
  Note: Installation errors (see attachment)
  See: https://fedoraproject.org/wiki/Packaging:Guidelines
- There are more different licenses than mentioned in spec file. run licensecheck
- Skipped tests. Run them or make clear why they're not run.
- Package does not own all dirs it creates. 


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

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.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Apache (v2.0)", "*No copyright* LGPL (v2 or later)", "MPL
     (v5)", "MPL (v1.1) GPL (v2 or later) or LGPL (v2.1 or later)", "Public
     domain BSD (3 clause)", "GPL (v2 or later) or LGPL (v2.1 or later)",
     "MPL (v1.1) LGPL (v2 or later)", "Apache (v2.0) BSD (2 clause)", "MPL
     (v1.1) LGPL (v2.1 or later)", "LGPL (v2.1)", "ISC", "LGPL (v2 or
     later)", "GPL (v3 or later)", "zlib/libpng", "QPL (v6)", "GPL (v2 or
     later) (with incorrect FSF address)", "BSD (2 clause)", "CC by
     (v3.0)", "GPL", "MIT/X11 (BSD like)", "MPL (v1.1)", "*No copyright*
     Public domain", "Apache", "NTP (legal disclaimer)", "*No copyright*
     BSD (unspecified)", "BSD (3 clause)", "ICU", "LGPL (v2)", "Unknown or
     generated", "*No copyright* BSD (2 clause)", "QPL (v3)", "CC0", "*No
     copyright* CDDL", "QPL (v7)", "LGPL (v2.1 or later)", "GPL (v2)". 8711
     files have unknown license. Detailed output of licensecheck in
     /home/mvala/rpmbuild/review/1438673-openjfx/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[x]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/man/ja_JP.UTF-8/man1,
     /usr/share/man/ja_JP.UTF-8
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/man/ja_JP.UTF-8,
     /usr/lib/jvm/openjfx, /usr/lib/jvm, /usr/share/man/ja_JP.UTF-8/man1
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: 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.
[-]: 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]: Useful -debuginfo package or justification otherwise.
[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.
     Note: Documentation size is 20480 bytes in 4 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 %license.
[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]: Dist tag is present.
[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 does not use a name that already exists.
[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.
[x]: Packages must not store files under /srv, /opt or /usr/local

Java:
[x]: Bundled jar/class files should be removed before build
[x]: Javadoc documentation files are generated and included in -javadoc
     subpackage
[x]: Javadoc subpackages should not have Requires: jpackage-utils
[x]: Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlink)

Maven:
[-]: If package contains pom.xml files install it (including metadata) even
     when building with ant
[!]: If tests are skipped during package build explain why it was needed in
     a comment
     Note: Tests seem to be skipped. Verify there is a commment giving a
     reason for this
[x]: Old add_to_maven_depmap macro is not being used

===== 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).
[-]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in openjfx-
     devel , openjfx-src , openjfx-javadoc , openjfx-debuginfo
[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.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define unless justified.

Java:
[x]: Packages are noarch unless they use JNI
     Note: openjfx subpackage is not noarch. Please verify manually
[x]: Package uses upstream build method (ant/maven/etc.)

===== EXTRA items =====

Generic:
[!]: Rpmlint is run on all installed packages.
     Note: Mock build failed
     See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Installation errors
-------------------
INFO: mock.py version 1.4.1 starting (python version = 3.6.1)...
Start: init plugins
INFO: selinux disabled
Finish: init plugins
Start: init plugins
INFO: selinux disabled
Finish: init plugins
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled dnf cache
Start: cleaning dnf metadata
Finish: cleaning dnf metadata
INFO: enabled HW Info plugin
Mock Version: 1.4.1
INFO: Mock Version: 1.4.1
Finish: chroot init
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled dnf cache
Start: cleaning dnf metadata
Finish: cleaning dnf metadata
INFO: enabled HW Info plugin
Mock Version: 1.4.1
INFO: Mock Version: 1.4.1
Finish: chroot init
INFO: installing package(s): /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-javadoc-8.0.152-7.b03.fc25.noarch.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-debuginfo-8.0.152-7.b03.fc25.x86_64.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-src-8.0.152-7.b03.fc25.noarch.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-8.0.152-7.b03.fc25.x86_64.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-devel-8.0.152-7.b03.fc25.noarch.rpm
ERROR: Command failed: 
 # /usr/bin/systemd-nspawn -q -M ad991bcb125141b89d1fcae9f44ff0d5 -D /var/lib/mock/fedora-25-x86_64-bootstrap/root -a --setenv=TERM=vt100 --setenv=SHELL=/bin/bash --setenv=HOME=/builddir --setenv=HOSTNAME=mock --setenv=PATH=/usr/bin:/bin:/usr/sbin:/sbin --setenv=PROMPT_COMMAND=printf "\033]0;<mock-chroot>\007" --setenv=PS1=<mock-chroot> \s-\v\$  --setenv=LANG=en_US.utf8 --setenv=LC_MESSAGES=C /usr/bin/dnf --installroot /var/lib/mock/fedora-25-x86_64/root/ --releasever 25 --disableplugin=local --setopt=deltarpm=false install /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-javadoc-8.0.152-7.b03.fc25.noarch.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-debuginfo-8.0.152-7.b03.fc25.x86_64.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-src-8.0.152-7.b03.fc25.noarch.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-8.0.152-7.b03.fc25.x86_64.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-devel-8.0.152-7.b03.fc25.noarch.rpm



Rpmlint
-------
Checking: openjfx-8.0.152-7.b03.fc25.x86_64.rpm
          openjfx-devel-8.0.152-7.b03.fc25.noarch.rpm
          openjfx-src-8.0.152-7.b03.fc25.noarch.rpm
          openjfx-javadoc-8.0.152-7.b03.fc25.noarch.rpm
          openjfx-debuginfo-8.0.152-7.b03.fc25.x86_64.rpm
          openjfx-8.0.152-7.b03.fc25.src.rpm
openjfx.x86_64: W: invalid-license GPL v2 with exceptions
openjfx.x86_64: W: invalid-license LGPL v2+
openjfx.x86_64: W: invalid-license LGPL v2+
openjfx.x86_64: E: incorrect-fsf-address /usr/share/licenses/openjfx/LICENSE
openjfx-devel.noarch: W: invalid-license GPL v2 with exceptions
openjfx-devel.noarch: W: invalid-license LGPL v2+
openjfx-devel.noarch: W: invalid-license LGPL v2+
openjfx-devel.noarch: W: only-non-binary-in-usr-lib
openjfx-devel.noarch: E: incorrect-fsf-address /usr/share/licenses/openjfx-devel/LICENSE
openjfx-src.noarch: W: invalid-license GPL v2 with exceptions
openjfx-src.noarch: W: invalid-license LGPL v2+
openjfx-src.noarch: W: invalid-license LGPL v2+
openjfx-src.noarch: W: only-non-binary-in-usr-lib
openjfx-src.noarch: W: no-documentation
openjfx-javadoc.noarch: W: invalid-license GPL v2 with exceptions
openjfx-javadoc.noarch: W: invalid-license LGPL v2+
openjfx-javadoc.noarch: W: invalid-license LGPL v2+
openjfx-javadoc.noarch: E: incorrect-fsf-address /usr/share/licenses/openjfx-javadoc/LICENSE
openjfx-debuginfo.x86_64: W: invalid-license GPL v2 with exceptions
openjfx-debuginfo.x86_64: W: invalid-license LGPL v2+
openjfx-debuginfo.x86_64: W: invalid-license LGPL v2+
openjfx-debuginfo.x86_64: E: debuginfo-without-sources
openjfx.src: W: invalid-license GPL v2 with exceptions
openjfx.src: W: invalid-license LGPL v2+
openjfx.src: W: invalid-license LGPL v2+
6 packages and 0 specfiles checked; 4 errors, 21 warnings.

Hi!
Except of other issues Michal will surely rise, I think

Package must own all directories that it creates[1]
  +
Fully versioned dependency in subpackages if applicable

Are necessary to fix. otherwise great job, all! TYVM

As for [1], Im wondering from where /usr/lib/jvm comes from... (as it should not own it, unlike other mentioned by review tool)

(In reply to Michal Vala from comment #36)
> Issues:
> =======
> - Package installs properly.
>   Note: Installation errors (see attachment)
>   See: https://fedoraproject.org/wiki/Packaging:Guidelines
> - There are more different licenses than mentioned in spec file. run
> licensecheck
> - Skipped tests. Run them or make clear why they're not run.
> - Package does not own all dirs it creates. 

Thanks for doing a review, I will look at these issues during this week/weekend

(In reply to jiri vanek from comment #37)
> As for [1], Im wondering from where /usr/lib/jvm comes from... (as it should
> not own it, unlike other mentioned by review tool)

Yes, /usr/lib/jvm sounds like a bug in the review tool, maybe it is triggered by no owner of /usr/lib/jvm/openjfx?

> Fully versioned dependency in subpackages if applicable
Thanks, I forgot that one, for all subpackages or only -devel?

> Are necessary to fix. otherwise great job, all! TYVM
Thanks, I am happy to help.

(In reply to Jonny Heggheim from comment #39)
> (In reply to jiri vanek from comment #37)
> > As for [1], Im wondering from where /usr/lib/jvm comes from... (as it should
> > not own it, unlike other mentioned by review tool)
> 
> Yes, /usr/lib/jvm sounds like a bug in the review tool, maybe it is
> triggered by no owner of /usr/lib/jvm/openjf

Just add: "Requires: javapackages-tools"

(In reply to gil cattaneo from comment #40)
> Just add: "Requires: javapackages-tools"

Thanks

A quick update:

Spec URL: https://jonny.fedorapeople.org/openjfx.spec
SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-8.b03.fc25.src.rpm
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=19610005

Changes:
 * Requires on javapackages-tools (owns /usr/lib/jvm)
 * Added requires on parent package for devel and src
 * Moved /usr/share/man/ja_JP.UTF-8/man1 to /usr/share/man/ja_JP/man1 (which is owned by the filesystem package)
 * Added %dir %{openjfxdir} in %files for parent package
 * Added comments why tests are disabled


I have not looked into any of the license issues, it would be great if other have time to help me.

> 
> I have not looked into any of the license issues, it would be great if other
> have time to help me.

I did.  Whole fx project should really be GPL2:
Thanx to Mario for pointing out http://hg.openjdk.java.net/openjfx/10/rt/file/48902e8e83a9/LICENSE

On other side, it claims to have same license as openjdk; http://pkgs.fedoraproject.org/cgit/rpms/java-1.8.0-openjdk.git/tree/java-1.8.0-openjdk.spec#n811

That is a bit contradiction, so maybe I have bad license in jdk package.

Your statement of:
License:        GPL v2 with exceptions and BSD and LGPL v2+ and (LGPL v2+ or BSD)
seems to be reflecting major of what review tool is saying. Hoowever I would go with simple GPL-2 with Classpath exception (/me  a bit afraid of BSD in license field)

The incorrect-fsf-address is actually patch for upstream, so we do not need to bother with it rigt now.

I will try to find some lawyer around, but with "GPL-2 with Classpath exception" I think we are ok to go.

(In reply to Jonny Heggheim from comment #42)
> A quick update:
> 
> Spec URL: https://jonny.fedorapeople.org/openjfx.spec
> SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-8.b03.fc25.src.rpm
> Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=19610005
> 
> Changes:
>  * Requires on javapackages-tools (owns /usr/lib/jvm)
>  * Added requires on parent package for devel and src
>  * Moved /usr/share/man/ja_JP.UTF-8/man1 to /usr/share/man/ja_JP/man1 (which
> is owned by the filesystem package)
>  * Added %dir %{openjfxdir} in %files for parent package
thanks, looks good

>  * Added comments why tests are disabled
do you know why those tests are failing? Can't be just excluded instead of not run all tests?

> 
> 
> I have not looked into any of the license issues, it would be great if other
> have time to help me.
Here is output from licensecheck https://michalvala.fedorapeople.org/openjfx/licensecheck.txt
Looks like openjfx include sources of several libs. I'm not sure how to solve that technically neither "legally".

(In reply to Michal Vala from comment #44)
> >  * Added comments why tests are disabled
> do you know why those tests are failing? Can't be just excluded instead of
> not run all tests?

Maybe it is not able to lookup the native library.. We can delete the test file as the first attempt.

It looks like most the the license issues is from the web mobule that we do not package.

The fxpackager module is licensed BSD

(In reply to Michal Vala from comment #44)
> do you know why those tests are failing? Can't be just excluded instead of
> not run all tests?

The tests for media and web is also executed :(

(In reply to Jonny Heggheim from comment #46)
> It looks like most the the license issues is from the web mobule that we do
> not package.

I gave a look at the package, it seems a very good start, good work!
 
> The fxpackager module is licensed BSD

I think the module is also GPLv2+Classpath, there are only few bits in the Makefile that have no license header, the tool incorrectly deduces them to be CDDL, but the license for the project applies there.

The webcore stuff is LGPLv2 or later, it's compatible with the GPLv2 but I understand we don't ship those, so we may want to consider to remove the sources we don't build/need.

The javascript stuff is more tricky, since it's Apache. Again, if we don't use those, we may want to remove this code from the source bundles.

That said, I don't think it makes that much of a difference, those are simply bundled libraries, not part of the JavaFX codebase. I will need to ask for advice here, but I doubt this is a real issue.

My hope is that we will be able to compile the web stuff too at some point, that's a really nice feature of JavaFX but we can't use it in the packaged version.

(In reply to Jonny Heggheim from comment #46)
> It looks like most the the license issues is from the web mobule that we do
> not package.

Thanks. I'm ok with that, but let's see if Mario comes up with something else.

Is removing unused code from source bundle easy to do?

(In reply to Michal Vala from comment #49)
> Thanks. I'm ok with that, but let's see if Mario comes up with something
> else.
> 
> Is removing unused code from source bundle easy to do?

I started making a repack script to reduce the size of the tarball, but the size saved did not justify the increased complexity.

I do not think there are license issues related to unused code, since there are no redistribution of it.

Re: comment 50

using modified sources is only required if needed for legal reasons.  Otherwise, doing so makes it harder/obfuscated if anyone ever wants to verify sources (ie, please don't do it, unless you really have to).

(In reply to Rex Dieter from comment #51)
> Re: comment 50
> 
> using modified sources is only required if needed for legal reasons. 
> Otherwise, doing so makes it harder/obfuscated if anyone ever wants to
> verify sources (ie, please don't do it, unless you really have to).

I think the same. And I think there is no reason in this case. Repack of sources always added burden not worthy of.

Mario, are you ok with keeping sources as they are, but present only license which aligns with binary shipped  stuff (gpl2 (fxpackager bsd))?


Thanx!

There is /usr/libs/.build-id directory in main package. It should get renamed to .openjfx-build-id (or similarly) or move to... I don't know where. Is it useful at all?

(In reply to Jonny Heggheim from comment #42)
> A quick update:
> 
> Spec URL: https://jonny.fedorapeople.org/openjfx.spec
> SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-8.b03.fc25.src.rpm
> Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=19610005
> 
> Changes:
>  * Requires on javapackages-tools (owns /usr/lib/jvm)
>  * Added requires on parent package for devel and src
>  * Moved /usr/share/man/ja_JP.UTF-8/man1 to /usr/share/man/ja_JP/man1 (which
> is owned by the filesystem package)
>  * Added %dir %{openjfxdir} in %files for parent package
>  * Added comments why tests are disabled
> 
> 
> I have not looked into any of the license issues, it would be great if other
> have time to help me.

You are not requireing any "java" ( or "java-devel" for devel subpackage) is it intentional? It may be (and my bindings are bringing those), but do not gave much sense....

hmm, the devel as noarch is interesting,  but those realy are scripts, and jars are plain java..... So well ,strange, but ok :)  /jsut for record

As this is going to final meter, I had pushed necessary changes to rawhide:
http://pkgs.fedoraproject.org/cgit/rpms/java-1.8.0-openjdk.git/commit/?id=562d2b38dd83268da9c22d1bf528b877d1235133

and lunched build: https://koji.fedoraproject.org/koji/taskinfo?taskID=19767533 (/me hopes it will pass)

(In reply to jiri vanek from comment #53)
> There is /usr/libs/.build-id directory in main package. It should get
> renamed to .openjfx-build-id (or similarly) or move to... I don't know
> where. Is it useful at all?

I did not notice this hidden file, I will not include then file if there are no code reading it.

(In reply to jiri vanek from comment #54)
> You are not requireing any "java" ( or "java-devel" for devel subpackage) is
> it intentional? It may be (and my bindings are bringing those), but do not
> gave much sense....

I did not want to introduce premature requires, I think now is a good time to decide what requires each package should have.

(In reply to jiri vanek from comment #55)
> hmm, the devel as noarch is interesting,  but those realy are scripts, and
> jars are plain java..... So well ,strange, but ok :)  /jsut for record

That is true, will test with devel as noarch.

(In reply to Jonny Heggheim from comment #58)
> (In reply to jiri vanek from comment #54)
> > You are not requireing any "java" ( or "java-devel" for devel subpackage) is
> > it intentional? It may be (and my bindings are bringing those), but do not
> > gave much sense....
> 
> I did not want to introduce premature requires, I think now is a good time
> to decide what requires each package should have.

Probably good idea.

Currently on my side:

java-1.8.0-openjdk-openjfx requires java-1.8.0-openjdk and openjfx
java-1.8.0-openjdk-openjfx-debug requires java-1.8.0-openjdk-debug and openjfx
java-1.8.0-openjdk-openjfx-devel requires java-1.8.0-openjdk-devel and openjfx-devel
java-1.8.0-openjdk-openjfx-devel-debug requires java-1.8.0-openjdk-devel-debug and openjfx-devel

Feel free to  ignore debug variants.  They have nothing to do with you.
java and java devel is always required in same VRA as fx subpackage.  In adition, openjdfx is required in same architecture as fx subpackage. See the patch from #c56

I think that your packages needs only:
openjfx requires java
openjfx-devel requires java-devel


Still I do not claims it as best idea, nor I'm saying that mya laready pushed requires are best what could be done. But well.. what else to do :)

(In reply to jiri vanek from comment #60)
> (In reply to Jonny Heggheim from comment #58)
> > (In reply to jiri vanek from comment #54)
> > > You are not requireing any "java" ( or "java-devel" for devel subpackage) is
> > > it intentional? It may be (and my bindings are bringing those), but do not
> > > gave much sense....
> > 
> > I did not want to introduce premature requires, I think now is a good time
> > to decide what requires each package should have.
> 
> Probably good idea.
> 
> Currently on my side:
> 
> java-1.8.0-openjdk-openjfx requires java-1.8.0-openjdk and openjfx
> java-1.8.0-openjdk-openjfx-debug requires java-1.8.0-openjdk-debug and
> openjfx
> java-1.8.0-openjdk-openjfx-devel requires java-1.8.0-openjdk-devel and
> openjfx-devel
> java-1.8.0-openjdk-openjfx-devel-debug requires
> java-1.8.0-openjdk-devel-debug and openjfx-devel
> 
> Feel free to  ignore debug variants.  They have nothing to do with you.
> java and java devel is always required in same VRA as fx subpackage.  In
> adition, openjdfx is required in same architecture as fx subpackage. See the
> patch from #c56
> 
> I think that your packages needs only:
> openjfx requires java
> openjfx-devel requires java-devel
> 
> 
> Still I do not claims it as best idea, nor I'm saying that mya laready
> pushed requires are best what could be done. But well.. what else to do :)


Maybe also your indivdual subpackages (src, javadoc, [devel]) should be bound by NVR[A] with main package. Well the devel have same fun with [arch] :)

(In reply to jiri vanek from comment #61) 
> Maybe also your indivdual subpackages (src, javadoc, [devel]) should be
> bound by NVR[A] with main package. Well the devel have same fun with [arch]
> :)

I used openjdk as a guide for what parent package to the subpacakges should require. javadoc packages tends to have no requires, while source and devel did have requires on the parent. 

Looks like there will be no issues with require with devel as noarch:

$ rpm -q --requires -p openjfx-devel-8.0.152-8.b03.fc25.noarch.rpm
/bin/sh
openjfx(x86-64) = 8.0.152-8.b03.fc25
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

(In reply to Jonny Heggheim from comment #62)
> (In reply to jiri vanek from comment #61) 
> > Maybe also your indivdual subpackages (src, javadoc, [devel]) should be
> > bound by NVR[A] with main package. Well the devel have same fun with [arch]
> > :)
> 
> I used openjdk as a guide for what parent package to the subpacakges should
> require. javadoc packages tends to have no requires, while source and devel
> did have requires on the parent. 

yup, go on with that.
> 
> Looks like there will be no issues with require with devel as noarch:
> 
> $ rpm -q --requires -p openjfx-devel-8.0.152-8.b03.fc25.noarch.rpm
> /bin/sh
> openjfx(x86-64) = 8.0.152-8.b03.fc25
> rpmlib(CompressedFileNames) <= 3.0.4-1
> rpmlib(FileDigests) <= 4.6.0-1
> rpmlib(PayloadFilesHavePrefix) <= 4.0-1
> rpmlib(PayloadIsXz) <= 5.2-1

Thank you for verification.


Imho except .build-id, license, which should be GPL2 [do we wont "with classpath exception"?] (javafxpackager BSD) and java/java-devel + self requires we are done here. Can you  publish one more spec+srpm (eventually also with scratch) which we can test?

TYVM!

Don't move or rename the .build-id directory.
It's created by RPM itself (it's the location where build IDs for debug info etc. are tracked).

see http://lists.rpm.org/pipermail/rpm-maint/2016-June/004365.html

(In reply to Fabio Valentini from comment #64)
> Don't move or rename the .build-id directory.
> It's created by RPM itself (it's the location where build IDs for debug info
> etc. are tracked).
> 
> see http://lists.rpm.org/pipermail/rpm-maint/2016-June/004365.html



But then it is in very bad directoryl Where it is supposed to be?

/usr/lib/.build-id is the correct location, so the "hidden-file-or-dir /usr/lib/.build-id" rpmlint warning is a false positive / can be ignored.

This directory is present in ~all packages that have been built since RPM in rawhide changed this behavior ~a few months back or so (which you can easily check by inspecting any recent package from rawhide).

> 
> Imho except .build-id, license, which should be GPL2 [do we wont "with
> classpath exception"?] (javafxpackager BSD) and java/java-devel + self
> requires we are done here. Can you  publish one more spec+srpm (eventually
> also with scratch) which we can test?
> 

.build-id is clarified by Fabio Valentini and self requires seems to be correct in latest state.

Jonny: Can you please add requires java/java-devel and I suggest licenses as GPL2 with classpath exception + BSD for fxpackager?

If no-one has any other comments and you include these changes, I'll approve this review.

Thanks!

(In reply to Michal Vala from comment #67)
> > 
> > Imho except .build-id, license, which should be GPL2 [do we wont "with
> > classpath exception"?] (javafxpackager BSD) and java/java-devel + self
> > requires we are done here. Can you  publish one more spec+srpm (eventually
> > also with scratch) which we can test?
> > 
> 
> .build-id is clarified by Fabio Valentini and self requires seems to be
> correct in latest state.
> 
> Jonny: Can you please add requires java/java-devel and I suggest licenses as
> GPL2 with classpath exception + BSD for fxpackager?
> 
> If no-one has any other comments and you include these changes, I'll approve
> this review.
> 
> Thanks!

You should/must use the "%mvn_install" macro https://fedora-java.github.io/howto/latest/index.html#gradle
e.g. %mvn_install -J build/sdk/docs/api

see https://fedora-java.github.io/howto/latest/index.html#mvn_file to configure the proper JARs install location.

(In reply to gil cattaneo from comment #68)
> You should/must use the "%mvn_install" macro
> https://fedora-java.github.io/howto/latest/index.html#gradle
> e.g. %mvn_install -J build/sdk/docs/api
> 
> see https://fedora-java.github.io/howto/latest/index.html#mvn_file to
> configure the proper JARs install location.

I will look into %mvn_file and %mvn_install, are you sure that these macros are the best to use for openjfx, since most of the files are going to be installed to /usr/lib/jvm/openjfx (no files in /usr/share/java/)

(In reply to Fabio Valentini from comment #66)
> /usr/lib/.build-id is the correct location, so the "hidden-file-or-dir
> /usr/lib/.build-id" rpmlint warning is a false positive / can be ignored.
> 
> This directory is present in ~all packages that have been built since RPM in
> rawhide changed this behavior ~a few months back or so (which you can easily
> check by inspecting any recent package from rawhide).

Thanks for providing this information.

(In reply to Jonny Heggheim from comment #69)
> (In reply to gil cattaneo from comment #68)
> > You should/must use the "%mvn_install" macro
> > https://fedora-java.github.io/howto/latest/index.html#gradle
> > e.g. %mvn_install -J build/sdk/docs/api
> > 
> > see https://fedora-java.github.io/howto/latest/index.html#mvn_file to
> > configure the proper JARs install location.
> 
> I will look into %mvn_file and %mvn_install, are you sure that these macros
> are the best to use for openjfx, since most of the files are going to be
> installed to /usr/lib/jvm/openjfx (no files in /usr/share/java/)

Our gudeline is pretty clear about JARs installation see https://fedoraproject.org/wiki/Packaging:Java#Installation_directory
othrewise you couls use mvn_file to set an alternative place or a symlinks if necessary "/usr/lib/jvm/openjfx"
e.g. %mvn_file :@ /usr/lib/jvm/openjfx/@
not sure if this now work ask in fedora-java irc channel for a better implementation/configuration

(In reply to Michal Vala from comment #67)
> .build-id is clarified by Fabio Valentini and self requires seems to be
> correct in latest state.
> 
> Jonny: Can you please add requires java/java-devel and I suggest licenses as
> GPL2 with classpath exception + BSD for fxpackager?
> 
> If no-one has any other comments and you include these changes, I'll approve
> this review.
> 
> Thanks!

Great, I have uploaded a new version with requires on java/java-devel + updated license. I will have a look at the comment #68 from Gil and see if I am able to use %mvn_install/%mvn_file. I also noticed there is a new version.

Spec URL: https://jonny.fedorapeople.org/openjfx.spec
SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-9.b03.fc25.src.rpm

(In reply to gil cattaneo from comment #71)
> Our gudeline is pretty clear about JARs installation see
> https://fedoraproject.org/wiki/Packaging:Java#Installation_directory
> othrewise you couls use mvn_file to set an alternative place or a symlinks
> if necessary "/usr/lib/jvm/openjfx"
> e.g. %mvn_file :@ /usr/lib/jvm/openjfx/@
> not sure if this now work ask in fedora-java irc channel for a better
> implementation/configuration

There have been a long discussion in bug 1145303 about the location of the libraries. JavaFX is intened to be installed directly into the JRE/JDK directories, so we ended up installing files to /usr/lib/jvm/openjfx and have a  JRE/JDK subpackage to provide symlinks.

Updated to latest upstream:

Spec URL: https://jonny.fedorapeople.org/openjfx.spec
SRPM URL: https://jonny.fedorapeople.org/openjfx-8.0.152-10.b04.fc25.src.rpm

(In reply to gil cattaneo from comment #71)
> (In reply to Jonny Heggheim from comment #69)
> > (In reply to gil cattaneo from comment #68)
> > > You should/must use the "%mvn_install" macro
> > > https://fedora-java.github.io/howto/latest/index.html#gradle
> > > e.g. %mvn_install -J build/sdk/docs/api
> > > 
> > > see https://fedora-java.github.io/howto/latest/index.html#mvn_file to
> > > configure the proper JARs install location.
> > 
> > I will look into %mvn_file and %mvn_install, are you sure that these macros
> > are the best to use for openjfx, since most of the files are going to be
> > installed to /usr/lib/jvm/openjfx (no files in /usr/share/java/)
> 
> Our gudeline is pretty clear about JARs installation see
> https://fedoraproject.org/wiki/Packaging:Java#Installation_directory
> othrewise you couls use mvn_file to set an alternative place or a symlinks
> if necessary "/usr/lib/jvm/openjfx"
> e.g. %mvn_file :@ /usr/lib/jvm/openjfx/@
> not sure if this now work ask in fedora-java irc channel for a better
> implementation/configuration

The .jar/.so files in the openjfx packages should be "treated" as if they are part of the openjdk package.

For example, rt.jar is included in java-1.8.0-openjdk-headless and installed here /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.131-1.b12.fc25.x86_64/jre/lib/rt.jar without using %mvn_install.

(In reply to gil cattaneo from comment #71)
> (In reply to Jonny Heggheim from comment #69)
> > (In reply to gil cattaneo from comment #68)
> > > You should/must use the "%mvn_install" macro
> > > https://fedora-java.github.io/howto/latest/index.html#gradle
> > > e.g. %mvn_install -J build/sdk/docs/api
> > > 
> > > see https://fedora-java.github.io/howto/latest/index.html#mvn_file to
> > > configure the proper JARs install location.
> > 
> > I will look into %mvn_file and %mvn_install, are you sure that these macros
> > are the best to use for openjfx, since most of the files are going to be
> > installed to /usr/lib/jvm/openjfx (no files in /usr/share/java/)
> 
> Our gudeline is pretty clear about JARs installation see
> https://fedoraproject.org/wiki/Packaging:Java#Installation_directory
> othrewise you couls use mvn_file to set an alternative place or a symlinks
> if necessary "/usr/lib/jvm/openjfx"
> e.g. %mvn_file :@ /usr/lib/jvm/openjfx/@
> not sure if this now work ask in fedora-java irc channel for a better
> implementation/configuration


It doesn't make much sense here. Guys elaborated it and came up with solution to place openjfx beside JDK/JRE and it makes perfect sense to me. Oracle has jfx included in their JDK and that's what developers expect. We decided to go with openjfx placed beside JDK and create openjfx subpackage that place links inside JDK.
Openjdk package itself also "violates" this and openjfx is same case.
So I don't see this to be an issue.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- %build honors applicable compiler flags or justifies otherwise.
  Note: I don't see this as an issue as I would take openjdx as exception
- Package installs properly.
  Note: Installation errors (see attachment)
  See: https://fedoraproject.org/wiki/Packaging:Guidelines
  Note: I see this just when ran via fedora-review tool. I keep it here, but you can probably ignore it. I can build and install bundles fine.


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

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: "*No copyright* LGPL (v2 or later)", "MPL (v5)", "MPL (v1.1)
     GPL (v2 or later) or LGPL (v2.1 or later)", "Public domain BSD (3
     clause)", "GPL (v2 or later) or LGPL (v2.1 or later)", "MPL (v1.1)
     LGPL (v2 or later)", "Apache (v2.0) BSD (2 clause)", "MPL (v1.1) LGPL
     (v2.1 or later)", "LGPL (v2.1)", "ISC", "LGPL (v2 or later)", "GPL (v3
     or later)", "QPL (v6)", "GPL (v2 or later) (with incorrect FSF
     address)", "BSD (2 clause)", "CC by (v3.0)", "GPL", "MIT/X11 (BSD
     like)", "MPL (v1.1)", "*No copyright* Public domain", "Apache", "NTP
     (legal disclaimer)", "*No copyright* BSD (unspecified)", "BSD (3
     clause)", "ICU", "LGPL (v2)", "Unknown or generated", "*No copyright*
     BSD (2 clause)", "QPL (v3)", "CC0", "*No copyright* CDDL", "QPL (v7)",
     "LGPL (v2.1 or later)", "GPL (v2)". 8649 files have unknown license.
     Detailed output of licensecheck in
     /home/mvala/rpmbuild/review/1438673-openjfx/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[-]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: 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.
[-]: 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]: Useful -debuginfo package or justification otherwise.
[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.
     Note: Documentation size is 20480 bytes in 4 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 %license.
[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]: Dist tag is present.
[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 does not use a name that already exists.
[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.
[x]: Packages must not store files under /srv, /opt or /usr/local

Java:
[x]: Bundled jar/class files should be removed before build
[x]: Javadoc documentation files are generated and included in -javadoc
     subpackage
[x]: Javadoc subpackages should not have Requires: jpackage-utils
[x]: Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlink)

Maven:
[-]: If package contains pom.xml files install it (including metadata) even
     when building with ant
[x]: Old add_to_maven_depmap macro is not being used

===== 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 openjfx-
     javadoc , openjfx-debuginfo
[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.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define unless justified.

Java:
[x]: Packages are noarch unless they use JNI
     Note: openjfx subpackage is not noarch. Please verify manually
[x]: Package uses upstream build method (ant/maven/etc.)

===== EXTRA items =====

Generic:
[!]: Rpmlint is run on all installed packages.
     Note: Mock build failed
     See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Installation errors
-------------------
INFO: mock.py version 1.4.1 starting (python version = 3.6.1)...
Start: init plugins
INFO: selinux disabled
Finish: init plugins
Start: init plugins
INFO: selinux disabled
Finish: init plugins
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled dnf cache
Start: cleaning dnf metadata
Finish: cleaning dnf metadata
INFO: enabled HW Info plugin
Mock Version: 1.4.1
INFO: Mock Version: 1.4.1
Finish: chroot init
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled dnf cache
Start: cleaning dnf metadata
Finish: cleaning dnf metadata
INFO: enabled HW Info plugin
Mock Version: 1.4.1
INFO: Mock Version: 1.4.1
Finish: chroot init
INFO: installing package(s): /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-debuginfo-8.0.152-10.b04.fc27.x86_64.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-8.0.152-10.b04.fc27.x86_64.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-src-8.0.152-10.b04.fc27.noarch.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-devel-8.0.152-10.b04.fc27.noarch.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-javadoc-8.0.152-10.b04.fc27.noarch.rpm
ERROR: Command failed: 
 # /usr/bin/systemd-nspawn -q -M cbe4abbb27bc448c80caec23908aae07 -D /var/lib/mock/fedora-rawhide-x86_64-bootstrap/root -a --setenv=TERM=vt100 --setenv=SHELL=/bin/bash --setenv=HOME=/builddir --setenv=HOSTNAME=mock --setenv=PATH=/usr/bin:/bin:/usr/sbin:/sbin --setenv=PROMPT_COMMAND=printf "\033]0;<mock-chroot>\007" --setenv=PS1=<mock-chroot> \s-\v\$  --setenv=LANG=en_US.utf8 --setenv=LC_MESSAGES=C /usr/bin/dnf --installroot /var/lib/mock/fedora-rawhide-x86_64/root/ --releasever 27 --disableplugin=local --setopt=deltarpm=false install /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-debuginfo-8.0.152-10.b04.fc27.x86_64.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-8.0.152-10.b04.fc27.x86_64.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-src-8.0.152-10.b04.fc27.noarch.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-devel-8.0.152-10.b04.fc27.noarch.rpm /home/mvala/rpmbuild/review/1438673-openjfx/results/openjfx-javadoc-8.0.152-10.b04.fc27.noarch.rpm



Rpmlint
-------
Checking: openjfx-8.0.152-10.b04.fc27.x86_64.rpm
          openjfx-devel-8.0.152-10.b04.fc27.noarch.rpm
          openjfx-src-8.0.152-10.b04.fc27.noarch.rpm
          openjfx-javadoc-8.0.152-10.b04.fc27.noarch.rpm
          openjfx-debuginfo-8.0.152-10.b04.fc27.x86_64.rpm
          openjfx-8.0.152-10.b04.fc27.src.rpm
openjfx.x86_64: W: invalid-license GPL v2 with exceptions
openjfx.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
openjfx.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
openjfx.x86_64: E: incorrect-fsf-address /usr/share/licenses/openjfx/LICENSE
openjfx-devel.noarch: W: invalid-license GPL v2 with exceptions
openjfx-devel.noarch: W: only-non-binary-in-usr-lib
openjfx-devel.noarch: E: incorrect-fsf-address /usr/share/licenses/openjfx-devel/LICENSE
openjfx-src.noarch: W: invalid-license GPL v2 with exceptions
openjfx-src.noarch: W: only-non-binary-in-usr-lib
openjfx-src.noarch: W: no-documentation
openjfx-javadoc.noarch: W: invalid-license GPL v2 with exceptions
openjfx-javadoc.noarch: E: incorrect-fsf-address /usr/share/licenses/openjfx-javadoc/LICENSE
openjfx-debuginfo.x86_64: W: invalid-license GPL v2 with exceptions
openjfx-debuginfo.x86_64: E: debuginfo-without-sources
openjfx.src: W: invalid-license GPL v2 with exceptions
6 packages and 0 specfiles checked; 4 errors, 11 warnings.

I don't want to block this review any more. All issues were clarified or fixed. When no other comments show up, I'll approve later today.

Thanks!

Maybe one little request about commented-out media and web parts in specfile. Can you please remove it or make clear comment why are those commented-out?

(In reply to Michal Vala from comment #79)
> Maybe one little request about commented-out media and web parts in
> specfile. Can you please remove it or make clear comment why are those
> commented-out?

Media is possible to build with the rpmfusion enabled, web depends on a ZIP file with precompiled libraries. I think the best is to remove the comments for now.

(In reply to Michal Vala from comment #77)
> Issues:
> =======
> - %build honors applicable compiler flags or justifies otherwise.
>   Note: I don't see this as an issue as I would take openjdx as exception

I agree, I tried earlier to pass in %optflags, but it took too much work and the build script got duplicated.

please remove commented-out code from spec before submit.

approved. Thank you for great job!

(In reply to Michal Vala from comment #82)
> please remove commented-out code from spec before submit.
> 
> approved. Thank you for great job!

Great, thanks!

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/openjfx

openjfx-8.0.152-10.b04.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-b9d1a0520b

openjfx-8.0.152-10.b04.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-2f213a60e5

openjfx-8.0.152-10.b04.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-2f213a60e5

openjfx-8.0.152-10.b04.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-b9d1a0520b

openjfx-8.0.152-10.b04.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-b9d1a0520b

openjfx-8.0.152-10.b04.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-b9d1a0520b

openjfx-8.0.152-10.b04.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

openjfx-8.0.152-10.b04.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.