Bug 1443076 - Review Request: java-9-openjdk - OpenJDK Runtime Environment in implementation of java 9 specification
Summary: Review Request: java-9-openjdk - OpenJDK Runtime Environment in implementatio...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Severin Gehwolf
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1482192
Blocks: 1447237
TreeView+ depends on / blocked
 
Reported: 2017-04-18 12:28 UTC by jiri vanek
Modified: 2017-10-19 08:30 UTC (History)
10 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-08-19 15:06:00 UTC
Type: ---
Embargoed:
jerboaa: fedora-review+


Attachments (Terms of Use)
Full review document including rpmlint output (which I've trimmed for noise) (78.18 KB, text/plain)
2017-05-22 14:23 UTC, Severin Gehwolf
no flags Details

Description jiri vanek 2017-04-18 12:28:17 UTC
Spec URL: coming soon
SRPM URL: coming soon
Description: OpenJDK Runtime Environment in implementation of java 9 specification
Fedora Account System Username: jvanek

Comment 1 jiri vanek 2017-04-18 12:34:38 UTC
The srpm/spec are now missing, as they are work in progress. This bug was added to: https://fedoraproject.org/wiki/Changes/Java9TechPreview
The jdk9 packages will be based on https://copr.fedorainfracloud.org/coprs/omajid/openjdk9/package/java-9-openjdk/

Although the copr state is failed, the packages are in better state then they look - the scratch builds passes outside copr ( == in koji) for f25 and up

Comment 2 jiri vanek 2017-04-18 15:05:22 UTC
https://pagure.io/copy_jdk_configs/c/ac945b7e2c6ea321c47f4d7568159a72fc9f1a77?branch=master

needed for multiple installs

Comment 3 jiri vanek 2017-04-19 12:33:52 UTC
Spec URL: https://jvanek.fedorapeople.org/openjdk9Review/V3/java-9-openjdk.spec
SRPM URL: https://jvanek.fedorapeople.org/openjdk9Review/V3/java-9-openjdk-9.0.0.163-2.fc24.src.rpm
Description: OpenJDK Runtime Environment in implementation of java 9 specification
Fedora Account System Username: jvanek


For record:
 * https://jvanek.fedorapeople.org/openjdk9Review/V1 is original Omair's work
 * https://jvanek.fedorapeople.org/openjdk9Review/V2 is broken base merge with openjdk
 * finally V3 is a bit sane (Still work in progress, will publish scratch builds)
 * #c2 patch for copy-jdk-configs is needed to properly process config files in case of multiple installs (not yet tested
 * unlike java-1.8.0-openjdk bundled NSS is used
 * unlike java-1.8.0-openjdk various export dirs (useless in jdk8) were removed
 * jre symlink is based only for backwardcomaptibility, as jre and sdk is now dereferenced only by splitting few binaries
 * shenandoah hotspot is not yet used, butI hope to integrate it before review ends
 * the src tarballs generating scripts are also missing, as they require already pushed package (will bedoen after the review)
 * policy tool is still cripled - man page and slave in headless, binary and desktop icon in head-full (default one)
 * priority of packages is 1, and debug subpackages 0
 * system tap and desktop integration was not (yet) tested at all

Comment 4 jiri vanek 2017-04-19 13:34:03 UTC
ok. That built for me:
 21M 29 java-9-openjdk-javadoc-9.0.0.163-2.fc24.noarch.rpm
 21M 29 java-9-openjdk-javadoc-debug-9.0.0.163-2.fc24.noarch.rpm
 49M 29 java-9-openjdk-javadoc-zip-9.0.0.163-2.fc24.noarch.rpm
 49M 30 java-9-openjdk-javadoc-zip-debug-9.0.0.163-2.fc24.noarch.rpm
 219K 24 java-9-openjdk-9.0.0.163-2.fc24.x86_64.rpm
 6.8K 30 java-9-openjdk-accessibility-9.0.0.163-2.fc24.x86_64.rpm
 6.5K 30 java-9-openjdk-accessibility-debug-9.0.0.163-2.fc24.x86_64.rpm
 217K 24 java-9-openjdk-debug-9.0.0.163-2.fc24.x86_64.rpm
 130M 31 java-9-openjdk-debuginfo-9.0.0.163-2.fc24.x86_64.rpm
 1.3M 28 java-9-openjdk-demo-9.0.0.163-2.fc24.x86_64.rpm
 1.3M 28 java-9-openjdk-demo-debug-9.0.0.163-2.fc24.x86_64.rpm
 3.4M 25 java-9-openjdk-devel-9.0.0.163-2.fc24.x86_64.rpm
 3.4M 25 java-9-openjdk-devel-debug-9.0.0.163-2.fc24.x86_64.rpm
  39M 25 java-9-openjdk-headless-9.0.0.163-2.fc24.x86_64.rpm
  40M 25 java-9-openjdk-headless-debug-9.0.0.163-2.fc24.x86_64.rpm
 224M 27 java-9-openjdk-jmods-9.0.0.163-2.fc24.x86_64.rpm
 173M 28 java-9-openjdk-jmods-debug-9.0.0.163-2.fc24.x86_64.rpm
  53M 28 java-9-openjdk-src-9.0.0.163-2.fc24.x86_64.rpm
  53M 28 java-9-openjdk-src-debug-9.0.0.163-2.fc24.x86_64.rpm


f25 https://koji.fedoraproject.org/koji/taskinfo?taskID=19079859
f26 https://koji.fedoraproject.org/koji/taskinfo?taskID=19079872
f24 https://koji.fedoraproject.org/koji/taskinfo?taskID=19079856
f27 https://koji.fedoraproject.org/koji/taskinfo?taskID=19079857

Comment 5 jiri vanek 2017-04-20 06:25:28 UTC
I have forget to add s390x to jit arches. WIll be fixed in next iteration.

Comment 6 Jan Kurik 2017-05-02 08:08:40 UTC
Tracking bug: https://bugzilla.redhat.com/show_bug.cgi?id=1447237

Comment 7 Severin Gehwolf 2017-05-19 08:35:18 UTC
New scratch build for F26 x86_64 as old built artifacts expired:
https://koji.fedoraproject.org/koji/taskinfo?taskID=19617138

Comment 8 Severin Gehwolf 2017-05-22 14:14:57 UTC
First round of review:

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

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


Issues:
=======
- No %config files under /usr.
  Note: %config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/security/default.policy%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/security/blacklisted.certs%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security/policy/limited/exempt_local.policy%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security/policy/limited/default_local.policy%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security/policy/limited/default_US_export.policy%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security/policy/unlimited/default_local.policy%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security/policy/unlimited/default_US_export.policy%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security/java.policy%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security/java.security%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/logging.properties%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security/nss.cfg%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/management/jmxremote.access%config
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/management/jmxremote.password.template%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/management/management.properties%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/net.properties%config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/sound.properties%config
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/accessibility.properties
  See:
  http://fedoraproject.org/wiki/Packaging/Guidelines#Configuration_files
  Perhaps symlink config files from /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf to /etc/java-9-openjdk/java-9-openjdk-9.0.0.163-2.fc26.x86_64/
  accessibility.properties seems to come from the spec. Would accessibility still work if placed in conf/accessibility.properties?
- Some directories are not owned by the package, but should be:
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/management,
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/security,
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf,
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security
- /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/accessibility.properties
  Should probably move to conf directory since the new layout has an explicit
  directory for configuration files. Not sure if there will be changes needed in
  the accessibility bridge (java-atk-wrapper).
- Shenandoah GC not yet supported. You mentioned that it'll be included once review completes.
  Consider this a TODO list item :)
  $ java -XX:+UseShenandoahGC -version
  Unrecognized VM option 'UseShenandoahGC'
  Error: Could not create the Java Virtual Machine.
  Error: A fatal exception has occurred. Program will exit.
  $ java -version
  openjdk version "9-ea"
  OpenJDK Runtime Environment (build 9-ea+163)
  OpenJDK 64-Bit Server VM (build 9-ea+163, mixed mode)
- Latest available snapshot should be packaged.
  Latest EA build is: jdk-9+170. See: http://jdk.java.net/9/

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

C/C++:
[x]: Package does not contain kernel modules.
[?]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.
     Note: See rpmlint output
     I see many rpmlint errors related to $ORIGIN and rpath. All of them seem
     to be internal libs. java-1.8.0-openjdk shows similar issues.
[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.
     All so files are internal OpenJDK libraries and are not in the ld path
     as far as I can tell.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)

Generic:
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
     Packager provided scratch build links which showed packages building.
     Reviewer submitted scratch build for SRPM on x86_64 which built fine.
[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.
     It's a combination of various licenses.
     NOTE: LGPL+ is unknown to rpmlint??!
[?]: License file installed when any subpackage combination is installed.
[?]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[!]: Package requires other packages for directories it uses.
     Note: No known owner of
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/management,
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/security,
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf,
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security
[!]: Package must own all directories that it creates.
     Note: Directories without known owners:
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf,
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/security,
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/management,
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security,
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package contains no bundled libraries without FPC exception.
     Jar files in sources 
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[!]: %config files are marked noreplace or the reason is justified.
     Note: No (noreplace) in %config
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/management/jmxremote.password.template
     This is a passwords template file.
     %config
     /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/accessibility.properties
     Should this be %config(noreplace) in addition to being moved to conf? --^
[x]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed. Same spec is used on older rpm versions. OK.
[x]: Development files must be in a -devel package
     Note: In addition to -devel subpackage there is a jmods-devel package for creating
     custom images.
[?]: 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.
[?]: gtk-update-icon-cache is invoked in %postun and %posttrans if package
     contains icons.
     Note: icons in java-9-openjdk
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package installs properly.
[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]: 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 contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     There are variants of -javadoc subpackages.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[-]: 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.
[x]: Package functions as described.
[!]: Latest version is packaged.
     Latest EA build is: jdk-9+170. See: http://jdk.java.net/9/
[?]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[?]: Scriptlets must be sane, if used.
[x]: SourceX tarball generation or download is documented.
     Script + documentation will follow after dist-git gets created.
[?]: 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.
[?]: Packages should try to preserve timestamps of original installed
     files.
[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]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Package should not use obsolete m4 macros
[x]: Spec file according to URL is the same as in SRPM.

Comment 9 Severin Gehwolf 2017-05-22 14:17:02 UTC
Correction:

[x]: Package contains no bundled libraries without FPC exception.
     Jar files in sources are a) development aids not being shipped
     b) test artifacts for testing the JDK

Comment 10 Severin Gehwolf 2017-05-22 14:23:44 UTC
Created attachment 1281077 [details]
Full review document including rpmlint output (which I've trimmed for noise)

Comment 11 Severin Gehwolf 2017-05-22 14:57:58 UTC
(In reply to jiri vanek from comment #3)
>  * unlike java-1.8.0-openjdk bundled NSS is used

Can we actually do this? I don't think so. Otherwise why would we remove that code from source in the generate-source-tarball script for JDK 8? Actually comparing ./jdk/src/jdk.crypto.ec/share/native/libsunec/impl from JDK 9 and ./jdk/src/share/native/sun/security/ec/impl from JDK 8 yields no difference for me.

Comment 12 jiri vanek 2017-05-24 15:33:04 UTC
(In reply to Severin Gehwolf from comment #11)
> (In reply to jiri vanek from comment #3)
> >  * unlike java-1.8.0-openjdk bundled NSS is used
> 
> Can we actually do this? I don't think so. Otherwise why would we remove
> that code from source in the generate-source-tarball script for JDK 8?
> Actually comparing ./jdk/src/jdk.crypto.ec/share/native/libsunec/impl from
> JDK 9 and ./jdk/src/share/native/sun/security/ec/impl from JDK 8 yields no
> difference for me.

Tahts surprising. I did not do work on that. That means that all patches form 8 are upstreamed, and it is default choice in OJDK.... (see the make call)

Comment 13 jiri vanek 2017-05-24 15:40:25 UTC
- No %config files under /usr.
  Note: %config(noreplace)
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/security/default.policy%config(noreplace)
...
  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/accessibility.properties
  See:
  http://fedoraproject.org/wiki/Packaging/Guidelines#Configuration_files
  Perhaps symlink config files from /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf to /etc/java-9-openjdk/java-9-openjdk-9.0.0.163-2.fc26.x86_64/

Well.. why? i dont see a reason for this change.  java config files are quite specific, and always lied in java_home.

Every config file we ever moved outised was casuing some not expected behavior (cacerts, etc/java).

To symlink, seems like just bending over knee to make guideliens happy.

If you isnists, I will do as you advice. But I really think it is unnecessary work which will help nothing.

Comment 14 jiri vanek 2017-05-24 15:42:23 UTC
> - Shenandoah GC not yet supported. You mentioned that it'll be included once review completes.
>   Consider this a TODO list item :)
>   $ java -XX:+UseShenandoahGC -version
>   Unrecognized VM option 'UseShenandoahGC'
>   Error: Could not create the Java Virtual Machine.
>   Error: A fatal exception has occurred. Program will exit.
>   $ java -version
>   openjdk version "9-ea"
>   OpenJDK Runtime Environment (build 9-ea+163)
>   OpenJDK 64-Bit Server VM (build 9-ea+163, mixed mode)

Yup, I hope to do so. Please dont insists on it. Stability of shenandoah in jdk9 is currently discussed issue.

> - Latest available snapshot should be packaged.
>   Latest EA build is: jdk-9+170. See: http://jdk.java.net/9/

Yes, in next iteration I will update. Tahnx!

Comment 15 jiri vanek 2017-05-24 15:44:57 UTC
>  accessibility.properties seems to come from the spec. Would accessibility still work if placed in conf/accessibility.properties?
> - /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64 lib/accessibility.properties
>  Should probably move to conf directory since the new layout has an explicit directory for configuration files. Not sure if there will be changes needed in the accessibility bridge (java-atk-wrapper).

I dont know. Accessibility is painful. I doubt it even works with 9. But probably worhty of moving. 

Will fix.


> Some directories are not owned by the package, but should be:
>  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/management,
>  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/security,
>  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf,
>  /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf/security

Hmm.. Will be fixed.

Comment 16 Mikolaj Izdebski 2017-05-24 15:54:58 UTC
(In reply to jiri vanek from comment #13)
> - No %config files under /usr.
>   Note: %config(noreplace)
>  
> /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/security/default.
> policy%config(noreplace)
> ...
>  
> /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/accessibility.
> properties
>   See:
>   http://fedoraproject.org/wiki/Packaging/Guidelines#Configuration_files
>   Perhaps symlink config files from
> /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf to
> /etc/java-9-openjdk/java-9-openjdk-9.0.0.163-2.fc26.x86_64/
> 
> Well.. why? i dont see a reason for this change.  java config files are
> quite specific, and always lied in java_home.
> 
> Every config file we ever moved outised was casuing some not expected
> behavior (cacerts, etc/java).
> 
> To symlink, seems like just bending over knee to make guideliens happy.
> 
> If you isnists, I will do as you advice. But I really think it is
> unnecessary work which will help nothing.

There are several valid reasons for keeping config files in /etc:
 - /usr may be read-only, but users should be allowed to edit config files
 - /usr may be shared between many machines, may use different Java config
 - if config is in /etc then you need to back up only this directory, /usr is supposed to be restorable from package contents

From FHS: "/usr is shareable, read-only data. That means that /usr should be shareable between various FHS-compliant hosts and must not be written to. Any information that is host-specific or varies with time is stored elsewhere". So yes, config files definitely don't belong in /usr

Comment 17 Severin Gehwolf 2017-05-24 15:57:00 UTC
(In reply to jiri vanek from comment #12)
> (In reply to Severin Gehwolf from comment #11)
> > (In reply to jiri vanek from comment #3)
> > >  * unlike java-1.8.0-openjdk bundled NSS is used
> > 
> > Can we actually do this? I don't think so. Otherwise why would we remove
> > that code from source in the generate-source-tarball script for JDK 8?
> > Actually comparing ./jdk/src/jdk.crypto.ec/share/native/libsunec/impl from
> > JDK 9 and ./jdk/src/share/native/sun/security/ec/impl from JDK 8 yields no
> > difference for me.
> 
> Tahts surprising. I did not do work on that. That means that all patches
> form 8 are upstreamed, and it is default choice in OJDK.... (see the make
> call)

I don't understand what you are saying. Can you clarify what "That" in "That means ..." is?

Originally you've said that bundled NSS is being used. That's for EC support, right? However, I believe we cannot use a source tarball with the internal NSS fork of openjdk included, can we? Wouldn't we need to go down a similar path as with openjdk 8. Remove internal NSS fork from sources, use system NSS. Or, alternatively, don't support EC.

Comment 18 jiri vanek 2017-06-01 11:17:04 UTC
(In reply to Severin Gehwolf from comment #17)
> (In reply to jiri vanek from comment #12)
> > (In reply to Severin Gehwolf from comment #11)
> > > (In reply to jiri vanek from comment #3)
> > > >  * unlike java-1.8.0-openjdk bundled NSS is used
> > > 
> > > Can we actually do this? I don't think so. Otherwise why would we remove
> > > that code from source in the generate-source-tarball script for JDK 8?
> > > Actually comparing ./jdk/src/jdk.crypto.ec/share/native/libsunec/impl from
> > > JDK 9 and ./jdk/src/share/native/sun/security/ec/impl from JDK 8 yields no
> > > difference for me.
> > 
> > Tahts surprising. I did not do work on that. That means that all patches
> > form 8 are upstreamed, and it is default choice in OJDK.... (see the make
> > call)
> 
> I don't understand what you are saying. Can you clarify what "That" in "That
> means ..." is?

Should be "system nss":

 USage of system nss is surprising. I did not do work on that. System nss used by ddefault means that all patches form 8 are upstreamed, and it is default choice in OJDK.... (see the call to make )

Better? Sorry.

> 
> Originally you've said that bundled NSS is being used. That's for EC
> support, right? However, I believe we cannot use a source tarball with the
> internal NSS fork of openjdk included, can we? Wouldn't we need to go down a
> similar path as with openjdk 8. Remove internal NSS fork from sources, use
> system NSS. Or, alternatively, don't support EC.

Sure, removal of the sources iss deffinitley thing to do.

Comment 19 jiri vanek 2017-06-01 11:23:47 UTC
(In reply to Mikolaj Izdebski from comment #16)
> (In reply to jiri vanek from comment #13)
> > - No %config files under /usr.
> >   Note: %config(noreplace)
> >  
> > /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/security/default.
> > policy%config(noreplace)
> > ...
> >  
> > /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/lib/accessibility.
> > properties
> >   See:
> >   http://fedoraproject.org/wiki/Packaging/Guidelines#Configuration_files
> >   Perhaps symlink config files from
> > /usr/lib/jvm/java-9-openjdk-9.0.0.163-2.fc26.x86_64/conf to
> > /etc/java-9-openjdk/java-9-openjdk-9.0.0.163-2.fc26.x86_64/
> > 
> > Well.. why? i dont see a reason for this change.  java config files are
> > quite specific, and always lied in java_home.
> > 
> > Every config file we ever moved outised was casuing some not expected
> > behavior (cacerts, etc/java).
> > 
> > To symlink, seems like just bending over knee to make guideliens happy.
> > 
> > If you isnists, I will do as you advice. But I really think it is
> > unnecessary work which will help nothing.
> 
> There are several valid reasons for keeping config files in /etc:
>  - /usr may be read-only, but users should be allowed to edit config files
>  - /usr may be shared between many machines, may use different Java config
>  - if config is in /etc then you need to back up only this directory, /usr
> is supposed to be restorable from package contents

Any config file is, isnt? Even if it is in etc...
> 
> From FHS: "/usr is shareable, read-only data. That means that /usr should be
> shareable between various FHS-compliant hosts and must not be written to.
> Any information that is host-specific or varies with time is stored
> elsewhere". So yes, config files definitely don't belong in /usr


Thanx for wring this down. 
I see two issues
 - all javas up to now had configs in java_home, which in /usr/lib/jvm/theJdk
   - so if your concerns are applicable, then they have to be revisited
 - config files were decided to be in jdk, as they are supposed to be jdk x cluster specific. So this is fundametnal chage to java in fedora/rhel.

Still it may be good idea to allow per machine setup. 


If we agreed on this, are you ok with simply symlinks?

Comment 20 Mikolaj Izdebski 2017-06-01 11:38:03 UTC
(In reply to jiri vanek from comment #19)
> (In reply to Mikolaj Izdebski from comment #16)
> >  - if config is in /etc then you need to back up only this directory, /usr
> > is supposed to be restorable from package contents
> 
> Any config file is, isnt? Even if it is in etc...

If you modify config file, then how are you going to restore your (modified) config from package contents?

> I see two issues
>  - all javas up to now had configs in java_home, which in /usr/lib/jvm/theJdk
>    - so if your concerns are applicable, then they have to be revisited

Have to? Theoretically older Javas could use existing (non-FHS compliant) location, but Java 9 could use the new location.

>  - config files were decided to be in jdk, as they are supposed to be jdk x
> cluster specific. So this is fundametnal chage to java in fedora/rhel.

I don't see any fundamental change here. You would just move config files from /usr to /etc and leave symlinks where config files used to be. No functional changes, but you gain FHS compliance.

> If we agreed on this, are you ok with simply symlinks?

Yes, symlinks are perfectly fine.

Comment 21 jiri vanek 2017-06-01 13:17:54 UTC
(In reply to Mikolaj Izdebski from comment #20)
> (In reply to jiri vanek from comment #19)
> > (In reply to Mikolaj Izdebski from comment #16)
> > >  - if config is in /etc then you need to back up only this directory, /usr
> > > is supposed to be restorable from package contents
> > 
> > Any config file is, isnt? Even if it is in etc...
> 
> If you modify config file, then how are you going to restore your (modified)
> config from package contents?

I see,you wont to restore it also with custom changes. That do not have much sense, does it? It requires different layer of backuping

I was speaking about restoring the original file. That something yo do simply by rpm reinstall.

> 
> > I see two issues
> >  - all javas up to now had configs in java_home, which in /usr/lib/jvm/theJdk
> >    - so if your concerns are applicable, then they have to be revisited
> 
> Have to? Theoretically older Javas could use existing (non-FHS compliant)
> location, but Java 9 could use the new location.

Should. It is strange to have each JDK having different apporaches. 

> 
> >  - config files were decided to be in jdk, as they are supposed to be jdk x
> > cluster specific. So this is fundametnal chage to java in fedora/rhel.
> 
> I don't see any fundamental change here. You would just move config files
> from /usr to /etc and leave symlinks where config files used to be. No
> functional changes, but you gain FHS compliance.

it is quite hard to imagine. In this case,  if you use fs with different /etc and, jdk may  have those configs missing. Some of them are fundamental for its work and it will not survive theirs absence...

> 
> > If we agreed on this, are you ok with simply symlinks?
> 
> Yes, symlinks are perfectly fine.

Comment 22 jiri vanek 2017-06-13 15:57:39 UTC
> > 
> > > If we agreed on this, are you ok with simply symlinks?
> > 
> > Yes, symlinks are perfectly fine.

Just to ensure convention, those are links, where source (target) is lying inside jdk, and the link is in /etc/java-9-openjdk/NVRA

So whe somebody will change configuration, he will nee to change target of those links.

Comment 23 Severin Gehwolf 2017-06-13 16:11:25 UTC
(In reply to jiri vanek from comment #22)
> > > 
> > > > If we agreed on this, are you ok with simply symlinks?
> > > 
> > > Yes, symlinks are perfectly fine.
> 
> Just to ensure convention, those are links, where source (target) is lying
> inside jdk, and the link is in /etc/java-9-openjdk/NVRA

It should be the other way round:

ls -l /usr/lib/jvm/java-9-openjdk-NVRA/conf

/usr/lib/jvm/java-9-openjdk-NVRA/conf -> /etc/java-9-openjdk/NVRA

> So whe somebody will change configuration, he will nee to change target of
> those links.

Of course.

Comment 24 jiri vanek 2017-06-13 16:19:24 UTC
(In reply to Severin Gehwolf from comment #23)
> (In reply to jiri vanek from comment #22)
> > > > 
> > > > > If we agreed on this, are you ok with simply symlinks?
> > > > 
> > > > Yes, symlinks are perfectly fine.
> > 
> > Just to ensure convention, those are links, where source (target) is lying
> > inside jdk, and the link is in /etc/java-9-openjdk/NVRA
> 
> It should be the other way round:
> 
> ls -l /usr/lib/jvm/java-9-openjdk-NVRA/conf
> 
> /usr/lib/jvm/java-9-openjdk-NVRA/conf -> /etc/java-9-openjdk/NVRA

I thought so, but, I might be missing something about /etc mounting in education...


So rpm installs:
  /etc/java-9-openjdk/NVR/some_conf
and creates
 /usr/lib/jvm/java-9-openjdk-NVRA/some_conf -> /etc/java-9-openjdk/NVRA/some_conf

Now you mount different /etc, and the  /etc/java-9-openjdk/NVRA/some_conf willl be missing, and   /usr/lib/jvm/java-9-openjdk-NVRA/some_conf will become broken.

What now? usually it is not easy to write those (eg java.security) config files and java will not start without them..


Or is somehow guaranted, that all my /etcS are affected by rpm install?  I doubt. I doubt even more by introducing NVRA in etc.
> 
> > So whe somebody will change configuration, he will nee to change target of
> > those links.
> 
> Of course.

In the more logical vice versa then mine world (yours one), this would not work in described way.

Comment 25 Severin Gehwolf 2017-06-13 16:51:03 UTC
(In reply to jiri vanek from comment #24)
> (In reply to Severin Gehwolf from comment #23)
> > (In reply to jiri vanek from comment #22)
> > > > > 
> > > > > > If we agreed on this, are you ok with simply symlinks?
> > > > > 
> > > > > Yes, symlinks are perfectly fine.
> > > 
> > > Just to ensure convention, those are links, where source (target) is lying
> > > inside jdk, and the link is in /etc/java-9-openjdk/NVRA
> > 
> > It should be the other way round:
> > 
> > ls -l /usr/lib/jvm/java-9-openjdk-NVRA/conf
> > 
> > /usr/lib/jvm/java-9-openjdk-NVRA/conf -> /etc/java-9-openjdk/NVRA
> 
> I thought so, but, I might be missing something about /etc mounting in
> education...

To quote Mikolaj:

""""
There are several valid reasons for keeping config files in /etc:
 - /usr may be read-only, but users should be allowed to edit config files
 - /usr may be shared between many machines, may use different Java config
 - if config is in /etc then you need to back up only this directory, /usr is supposed to be restorable from package contents

From FHS: "/usr is shareable, read-only data. That means that /usr should be shareable between various FHS-compliant hosts and must not be written to. Any information that is host-specific or varies with time is stored elsewhere". So yes, config files definitely don't belong in /usr

""""

It's not /etc that's shared between machines. It's /usr which sometimes gets mounted.

In that scenario it's expected for /etc/java-9-openjdk/ to contain every NVRAs config that are installed via mounted /usr/lib/jvm. /etc would have to get replicated across all machines.

But yes, in the mounted /usr scenario, if there is a mismatch between /etc and what's installed via the /usr mount then things break. That's expected.

The common use-case for such set-ups is for the NFS-shared host to do RPM installs and consumers of the NFS-share get config synchronized via version control. I.e. they don't install via RPM directly.

Comment 26 jiri vanek 2017-07-18 11:59:15 UTC
Spec URL: https://jvanek.fedorapeople.org/openjdk9Review/V4/java-9-openjdk.spec
SRPM URL: https://jvanek.fedorapeople.org/openjdk9Review/V4/java-9-openjdk-9.0.0.178-1.fc24.src.rpm
Description: OpenJDK Runtime Environment in implementation of java 9 specification
Fedora Account System Username: jvanek

All should be fixed, except:
  *  the config files  are still in jdk. I need to resolve few more issues in https://pagure.io/copy_jdk_configs/ before move with them (you can see that work in progress)
   * also I need to find if to link whole conf dir, or individual configs.
  * it seems that crypto has changed between those two source tarbhalls, and there is newly ../lib/libsunec.so 

The linked srpm passed for me locally: 
    223610 Jul 18 11:45 java-9-openjdk-9.0.0.178-1.fc24.x86_64.rpm
      6878 Jul 18 11:49 java-9-openjdk-accessibility-9.0.0.178-1.fc24.x86_64.rpm
      6582 Jul 18 11:49 java-9-openjdk-accessibility-debug-9.0.0.178-1.fc24.x86_64.rpm
    221534 Jul 18 11:45 java-9-openjdk-debug-9.0.0.178-1.fc24.x86_64.rpm
 119179042 Jul 18 11:50 java-9-openjdk-debuginfo-9.0.0.178-1.fc24.x86_64.rpm
    614878 Jul 18 11:48 java-9-openjdk-demo-9.0.0.178-1.fc24.x86_64.rpm
    615810 Jul 18 11:48 java-9-openjdk-demo-debug-9.0.0.178-1.fc24.x86_64.rpm
   3503706 Jul 18 11:46 java-9-openjdk-devel-9.0.0.178-1.fc24.x86_64.rpm
   3504478 Jul 18 11:46 java-9-openjdk-devel-debug-9.0.0.178-1.fc24.x86_64.rpm
  42696374 Jul 18 11:45 java-9-openjdk-headless-9.0.0.178-1.fc24.x86_64.rpm
  43759982 Jul 18 11:46 java-9-openjdk-headless-debug-9.0.0.178-1.fc24.x86_64.rpm
 241491838 Jul 18 11:47 java-9-openjdk-jmods-9.0.0.178-1.fc24.x86_64.rpm
 187679398 Jul 18 11:48 java-9-openjdk-jmods-debug-9.0.0.178-1.fc24.x86_64.rpm
  58877670 Jul 18 11:48 java-9-openjdk-src-9.0.0.178-1.fc24.x86_64.rpm
  58876934 Jul 18 11:49 java-9-openjdk-src-debug-9.0.0.178-1.fc24.x86_64.rpm

But failed strangely  in koji. New built at:
  https://koji.fedoraproject.org/koji/taskinfo?taskID=20595078

Comment 27 jiri vanek 2017-07-18 14:29:37 UTC
> 
> But failed strangely  in koji. New built at:
>   https://koji.fedoraproject.org/koji/taskinfo?taskID=20595078

found the issue. Will respin as V4. Feel free to proceed with review.

Comment 28 jiri vanek 2017-07-18 14:38:40 UTC
updated:

Spec URL: https://jvanek.fedorapeople.org/openjdk9Review/V4/java-9-openjdk.spec
SRPM URL: https://jvanek.fedorapeople.org/openjdk9Review/V4/java-9-openjdk-9.0.0.178-1.fc24.src.rpm
Description: OpenJDK Runtime Environment in implementation of java 9 specification
Fedora Account System Username: jvanek

https://koji.fedoraproject.org/koji/taskinfo?taskID=20597202 now should pass.

Comment 29 jiri vanek 2017-07-19 11:10:58 UTC
(In reply to jiri vanek from comment #28)
> updated:
> 
> Spec URL:
> https://jvanek.fedorapeople.org/openjdk9Review/V4/java-9-openjdk.spec
> SRPM URL:
> https://jvanek.fedorapeople.org/openjdk9Review/V4/java-9-openjdk-9.0.0.178-1.
> fc24.src.rpm
> Description: OpenJDK Runtime Environment in implementation of java 9
> specification
> Fedora Account System Username: jvanek
> 
> https://koji.fedoraproject.org/koji/taskinfo?taskID=20597202 now should pass.

Faield too.

https://koji.fedoraproject.org/koji/taskinfo?taskID=20607323 have correct aot_arches, still, failed on aarch64 (imho not rpm bug, but dscovering in progress)

Comment 31 Severin Gehwolf 2017-08-03 15:56:24 UTC
(In reply to jiri vanek from comment #30)
> Indeed, needed fix to aarch64 hotspot:
> https://koji.fedoraproject.org/koji/taskinfo?taskID=20720992
> finally passed.

It failed on arm32:

         ^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:846: error: ';' expected
# A fatal error has been detected by the Java Runtime Environment:
                   ^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:846: error: ';' expected
# A fatal error has been detected by the Java Runtime Environment:
                                 ^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:846: error: ';' expected
# A fatal error has been detected by the Java Runtime Environment:
                                        ^
Generating source file: MenuBarPainter.java/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:846: error: ';' expected
# A fatal error has been detected by the Java Runtime Environment:
                                                     ^
Generating /builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/images/docs/api/java/awt/GridBagLayout.html...
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:846: error: <identifier> expected
# A fatal error has been detected by the Java Runtime Environment:
                                                                 ^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:847: error: illegal character: '#'
#
^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:848: error: illegal character: '#'
#  SIGSEGV (0xb) at pc=0xb656d00c, pid=10309, tid=10322
^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:848: error: invalid method declaration; return type required
#  SIGSEGV (0xb) at pc=0xb656d00c, pid=10309, tid=10322
   ^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:848: error: illegal start of type
#  SIGSEGV (0xb) at pc=0xb656d00c, pid=10309, tid=10322
            ^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:849: error: illegal character: '#'
#
^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:850: error: illegal character: '#'
# JRE version: OpenJDK Runtime Environment (9.0+178) (slowdebug build 9-ea+178)
^
Generating source file: MenuBarMenuPainter.java/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:851: error: illegal character: '#'
# Java VM: OpenJDK Server VM (slowdebug 9-ea+178, mixed mode, serial gc, linux-)
^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:852: error: illegal character: '#'
# Problematic frame:
^
/builddir/build/BUILD/java-9-openjdk-9.0.0.178-1.fc26.arm/openjdk/build-debug/bootcycle-build/support/gensrc/java.base/java/nio/DirectByteBufferR.java:853: error: illegal character: '#'
# V  [libjvm.so+0xb8500c]
^

Comment 32 Severin Gehwolf 2017-08-03 16:34:16 UTC
Second round of review:

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

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


Issues:
=======
- Spec has this:
  # PPC/PPC64 needs -fno-tree-vectorize since -O3 would
  # otherwise generate wrong code producing segfaults.
  %ifarch %{power64} ppc
  EXTRA_CFLAGS="$EXTRA_CFLAGS -fno-tree-vectorize"

  I believe this is because of an old GCC bug, which is no
  longer an issue. At least not for Fedora.
  See https://bugzilla.redhat.com/show_bug.cgi?id=1146897
  We should no longer pass '-fno-tree-vectorize' on PPC64.
- In order to match upstream, 'java -version' should output
  openjdk version "9". I.e. no '-ea' suffix any more.
  $ java -version
  openjdk version "9-ea"
  OpenJDK Runtime Environment (build 9-ea+178)
  OpenJDK 64-Bit Server VM (build 9-ea+178, mixed mode)
  See:
  http://mail.openjdk.java.net/pipermail/quality-discuss/2017-July/000706.html
- Note: No known owner of
  /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/server/classes.jsa
- Please move config files to /etc/ and link them to /etc in
  /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/conf/
- Latest available snapshot should be packaged.
  Latest EA build is: jdk-9+180. See: http://jdk.java.net/9/
  I'll not point this out in the next review since this is going to change soon
  again. I trust you'll update to latest available build once approved.
- Some config files are not in conf: It's likely an upstream issue
  as I see them in lib for an upstream build too. Please report a bug.
  $ rpm -Vv java-9-openjdk-headless | grep ' c ' | grep lib/security
.........  c /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/security/blacklisted.certs
.........  c /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/security/default.policy

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

C/C++:
[x]: Package does not contain kernel modules.
[?]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.
     Note: See rpmlint output
     I see many rpmlint errors related to $ORIGIN and rpath. All of them seem
     to be internal libs. java-1.8.0-openjdk shows similar issues.
[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.
     All so files are internal OpenJDK libraries and are not in the ld path
     as far as I can tell.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)

Generic:
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
     Packager provided scratch build links which showed packages building.
     Reviewer submitted scratch build for SRPM on x86_64 which built fine.
[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.
     It's a combination of various licenses.
     NOTE: LGPL+ is unknown to rpmlint??!
[?]: License file installed when any subpackage combination is installed.
[?]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[!]: Package requires other packages for directories it uses.
     Note: No known owner of
     /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/server/classes.jsa
[x]: Package must own all directories that it creates.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package contains no bundled libraries without FPC exception.
     Jar files in sources 
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed. Same spec is used on older rpm versions. OK.
[x]: Development files must be in a -devel package
     Note: In addition to -devel subpackage there is a jmods-devel package for creating
     custom images.
[?]: 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.
[?]: gtk-update-icon-cache is invoked in %postun and %posttrans if package
     contains icons.
     Note: icons in java-9-openjdk
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package installs properly.
[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]: 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 contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     There are variants of -javadoc subpackages.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[-]: 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.
[x]: Package functions as described.
[!]: Latest version is packaged.
     Latest EA build is: jdk-9+180. See: http://jdk.java.net/9/
[?]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[?]: Scriptlets must be sane, if used.
[x]: SourceX tarball generation or download is documented.
     Script + documentation will follow after dist-git gets created.
[?]: 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.
[?]: Packages should try to preserve timestamps of original installed
     files.
[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]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Package should not use obsolete m4 macros
[x]: Spec file according to URL is the same as in SRPM.

Comment 33 Severin Gehwolf 2017-08-03 16:42:47 UTC
(In reply to Severin Gehwolf from comment #31)
> (In reply to jiri vanek from comment #30)
> > Indeed, needed fix to aarch64 hotspot:
> > https://koji.fedoraproject.org/koji/taskinfo?taskID=20720992
> > finally passed.
> 
> It failed on arm32:

FWIW, I'd be fine with disabling the slowdebug build on arm32 until this is fixed.

Comment 34 jiri vanek 2017-08-04 12:54:13 UTC
Spec URL: https://jvanek.fedorapeople.org/openjdk9Review/V7/java-9-openjdk.spec
SRPM URL: https://jvanek.fedorapeople.org/openjdk9Review/V7/java-9-openjdk-9.0.0.181-1.fc24.src.rpm
https://koji.fedoraproject.org/koji/taskinfo?taskID=21044824

> 
> Issues:
> =======
> - Spec has this:
>   # PPC/PPC64 needs -fno-tree-vectorize since -O3 would
>   # otherwise generate wrong code producing segfaults.
>   %ifarch %{power64} ppc
>   EXTRA_CFLAGS="$EXTRA_CFLAGS -fno-tree-vectorize"
> 

removed!
...
> http://mail.openjdk.java.net/pipermail/quality-discuss/2017-July/000706.html
> - Note: No known owner of
>   /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/server/classes.jsa

Fixed

> - Please move config files to /etc/ and link them to /etc in
>   /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/conf/

See at bottom

> - Latest available snapshot should be packaged.
>   Latest EA build is: jdk-9+180. See: http://jdk.java.net/9/
>   I'll not point this out in the next review since this is going to change
> soon
>   again. I trust you'll update to latest available build once approved.

Moved to u181 with CPU patches

> - Some config files are not in conf: It's likely an upstream issue
>   as I see them in lib for an upstream build too. Please report a bug.
>   $ rpm -Vv java-9-openjdk-headless | grep ' c ' | grep lib/security
> .........  c
> /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/security/blacklisted.
> certs
> .........  c
> /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/security/default.
> policy
> 

If the file is not in expected location, jdk will not find it
I can move them to conf and link to original location, but as /etc is still in play, then I would rather not touch it.
If you insists, I will move them, and link them.

...
> > It failed on arm32:
> 
> FWIW, I'd be fine with disabling the slowdebug build on arm32 until this is
> fixed.

done



Config files in etc. I really wont to move them. However it will not be an easy task from point of view of multiple installs and copy-jdk-configs.
I would like to not block this review on it. I have already started to work on c-j-c to support this, but there is still some longer way to go.

Comment 35 Severin Gehwolf 2017-08-04 14:48:53 UTC
(In reply to jiri vanek from comment #34)
> > - Some config files are not in conf: It's likely an upstream issue
> >   as I see them in lib for an upstream build too. Please report a bug.
> >   $ rpm -Vv java-9-openjdk-headless | grep ' c ' | grep lib/security
> > .........  c
> > /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/security/blacklisted.
> > certs
> > .........  c
> > /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/security/default.
> > policy
> > 
> 
> If the file is not in expected location, jdk will not find it
> I can move them to conf and link to original location, but as /etc is still
> in play, then I would rather not touch it.
> If you insists, I will move them, and link them.

My point was that we should ask *upstream* whether or not this is intentional. It's one of the odd balls. Everything else is in conf upstream already. Thus, we should file a bug/ask upstream whether or not this should be in conf upstream. That's all that's to do here. Thank you!

> Config files in etc. I really wont to move them. However it will not be an
> easy task from point of view of multiple installs and copy-jdk-configs.
> I would like to not block this review on it. I have already started to work
> on c-j-c to support this, but there is still some longer way to go.

OK. This should be fine. I'm glad to hear this will be worked on.

Builds failed:

+ cp -a openjdk/build/bundles/jdk-9-ea+181-docs.zip /builddir/build/BUILDROOT/java-9-openjdk-9.0.0.181-1.fc26.x86_64/usr/share/javadoc/java-9-openjdk-9.0.0.181-1.fc26.zip
cp: cannot stat 'openjdk/build/bundles/jdk-9-ea+181-docs.zip': No such file or directory

This in the spec file should be the culprit:
cp -a %{buildoutputdir $suffix}/bundles/jdk-%{majorver}-ea+%{buildver}-docs.zip  $RPM_BUILD_ROOT%{_javadocdir}/%{uniquejavadocdir $suffix}.zip

No 'ea' anymore in the version.

Comment 36 jiri vanek 2017-08-04 14:51:07 UTC
Yah. I noted. Already respined:  https://koji.fedoraproject.org/koji/taskinfo?taskID=21048132

Comment 37 jiri vanek 2017-08-04 15:01:52 UTC
...
> 
> My point was that we should ask *upstream* whether or not this is
> intentional. It's one of the odd balls. Everything else is in conf upstream
> already. Thus, we should file a bug/ask upstream whether or not this should
> be in conf upstream. That's all that's to do here. Thank you!

fired: http://mail.openjdk.java.net/pipermail/build-dev/2017-August/019587.html

> 
> > Config files in etc. I really wont to move them. However it will not be an
> > easy task from point of view of multiple installs and copy-jdk-configs.
> > I would like to not block this review on it. I have already started to work
> > on c-j-c to support this, but there is still some longer way to go.
> 
> OK. This should be fine. I'm glad to hear this will be worked on.

Thank you for trust.

And thank you for careful review.

Comment 38 jiri vanek 2017-08-04 15:09:02 UTC
(In reply to jiri vanek from comment #36)
> Yah. I noted. Already respined: 
> https://koji.fedoraproject.org/koji/taskinfo?taskID=21048132

and updated V7

Comment 39 jiri vanek 2017-08-07 12:47:26 UTC
(In reply to Severin Gehwolf from comment #35)
> (In reply to jiri vanek from comment #34)
> > > - Some config files are not in conf: It's likely an upstream issue
> > >   as I see them in lib for an upstream build too. Please report a bug.
> > >   $ rpm -Vv java-9-openjdk-headless | grep ' c ' | grep lib/security
> > > .........  c
> > > /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/security/blacklisted.
> > > certs
> > > .........  c
> > > /usr/lib/jvm/java-9-openjdk-9.0.0.178-1.fc26.x86_64/lib/security/default.
> > > policy
> > > 
> > 
> > If the file is not in expected location, jdk will not find it
> > I can move them to conf and link to original location, but as /etc is still
> > in play, then I would rather not touch it.
> > If you insists, I will move them, and link them.
> 
> My point was that we should ask *upstream* whether or not this is
> intentional. It's one of the odd balls. Everything else is in conf upstream
> already. Thus, we should file a bug/ask upstream whether or not this should
> be in conf upstream. That's all that's to do here. Thank you!

explained: http://mail.openjdk.java.net/pipermail/build-dev/2017-August/019589.html
> 
> > Config files in etc. I really wont to move them. However it will not be an
> > easy task from point of view of multiple installs and copy-jdk-configs.
> > I would like to not block this review on it. I have already started to work
> > on c-j-c to support this, but there is still some longer way to go.
> 
> OK. This should be fine. I'm glad to hear this will be worked on.
> 

Suport for linked resources added: https://pagure.io/copy_jdk_configs/commits/master

Now it will need hours and hours of testing.

Comment 40 Severin Gehwolf 2017-08-07 16:52:24 UTC
(In reply to jiri vanek from comment #36)
> Yah. I noted. Already respined: 
> https://koji.fedoraproject.org/koji/taskinfo?taskID=21048132

BuildError: The following noarch package built differently on different architectures: java-9-openjdk-javadoc-zip-9.0.0.181-1.fc26.noarch.rpm

Not sure what do do about this. Make javadoc-zip archful?

Comment 41 Severin Gehwolf 2017-08-07 16:54:27 UTC
(In reply to Severin Gehwolf from comment #40)
> (In reply to jiri vanek from comment #36)
> > Yah. I noted. Already respined: 
> > https://koji.fedoraproject.org/koji/taskinfo?taskID=21048132
> 
> BuildError: The following noarch package built differently on different
> architectures: java-9-openjdk-javadoc-zip-9.0.0.181-1.fc26.noarch.rpm
> 
> Not sure what do do about this. Make javadoc-zip archful?

Note that aot is x86_64 Linux only. So that might be the reason why this happens.

Comment 42 jiri vanek 2017-08-14 18:31:44 UTC
(In reply to Severin Gehwolf from comment #41)
> (In reply to Severin Gehwolf from comment #40)
> > (In reply to jiri vanek from comment #36)
> > > Yah. I noted. Already respined: 
> > > https://koji.fedoraproject.org/koji/taskinfo?taskID=21048132
> > 
> > BuildError: The following noarch package built differently on different
> > architectures: java-9-openjdk-javadoc-zip-9.0.0.181-1.fc26.noarch.rpm
> > 
> > Not sure what do do about this. Make javadoc-zip archful?
> 
> Note that aot is x86_64 Linux only. So that might be the reason why this
> happens.

Yes, that would be the reason. So There are only two options - to disable aot, or make javadocs arch dependent:(

Comment 43 Severin Gehwolf 2017-08-14 19:10:51 UTC
(In reply to jiri vanek from comment #42)
> (In reply to Severin Gehwolf from comment #41)
> > (In reply to Severin Gehwolf from comment #40)
> > > (In reply to jiri vanek from comment #36)
> > > > Yah. I noted. Already respined: 
> > > > https://koji.fedoraproject.org/koji/taskinfo?taskID=21048132
> > > 
> > > BuildError: The following noarch package built differently on different
> > > architectures: java-9-openjdk-javadoc-zip-9.0.0.181-1.fc26.noarch.rpm
> > > 
> > > Not sure what do do about this. Make javadoc-zip archful?
> > 
> > Note that aot is x86_64 Linux only. So that might be the reason why this
> > happens.
> 
> Yes, that would be the reason. So There are only two options - to disable
> aot, or make javadocs arch dependent:(

The latter, please.

Comment 44 Severin Gehwolf 2017-08-15 15:08:54 UTC
I'm approving this request. Jiri will make the javadoc-zips an archful package to account for the jaot differences which is an x86_64 only Linux feature upstream at this point.

Comment 45 jiri vanek 2017-08-15 15:30:40 UTC
(In reply to Severin Gehwolf from comment #44)
> I'm approving this request. Jiri will make the javadoc-zips an archful
> package to account for the jaot differences which is an x86_64 only Linux
> feature upstream at this point.

Ty. + move configs to /etc once Ihave it fully tested.

Comment 46 Gwyn Ciesla 2017-08-15 16:02:26 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/java-9-openjdk

Comment 47 Gwyn Ciesla 2017-08-15 16:06:40 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/java-9-openjdk

Comment 48 jiri vanek 2017-08-15 18:10:34 UTC
...and building...

Comment 52 jiri vanek 2017-08-18 18:37:13 UTC
f27+ spec needed huge macro usage adaptation for rpmbuild 4.14 

f28 https://koji.fedoraproject.org/koji/buildinfo?buildID=958063
f27 https://koji.fedoraproject.org/koji/buildinfo?buildID=955944

for record, unchanged build with original mcros usage:
f26 https://koji.fedoraproject.org/koji/buildinfo?buildID=956299

Comment 53 jiri vanek 2017-08-19 15:06:00 UTC
as all the builds passed, and  bodhi updates are still not enabled for f27, and noupdate planned for f26 (just builds), closing.

Comment 55 Mikolaj Izdebski 2017-10-09 11:48:13 UTC
(In reply to jiri vanek from comment #54)
> Config files have been moved to /etc for rawhide:

Thank you!

Comment 56 Dominik 'Rathann' Mierzejewski 2017-10-18 21:22:52 UTC
Nobody noticed that %changelog has wrong versions:

%changelog
* Tue Oct 10 2017 Jiri Vanek <jvanek> - 1:1.9.0.0-9.b163
- now owning dir etcjavasubdir

* Tue Oct 10 2017 Jiri Vanek <jvanek> - 1:1.9.0.0-8.b163
- EC no longer built

* Tue Oct 10 2017 Jiri Vanek <jvanek> - 1:1.9.0.0-7.b163
- now owning dir etcjavadir

* Thu Oct 05 2017 Jiri Vanek <jvanek> - 1:1.9.0.0-4.b163
- config files moved to etc

* Tue Aug 29 2017 Michal Vala  <mvala> - 1:1.9.0.0-3.b163
- changed  archinstall to i686
- added ownership of lib/client/

while package versions are:
9.0.0.181-1.fc28
9.0.0.181-2.fc28
...
9.0.0.181-9.fc28

Where are the %changelog entries for -1, -2, -5 and -6?

You could also have dropped the Epoch: from the package, since it's a completely new package and kept it only in the virtual Provides:. Actually, the use of Epoch: in a NEW package must be justified in the review. It wasn't. See https://fedoraproject.org/wiki/Packaging:Versioning .

Requires: xorg-x11-fonts-Type1
Seriously? Does Java 9 still require legacy Type1 fonts in 2017?

# Require jpackage-utils for ownership of /usr/lib/jvm/
Requires: jpackage-utils
jpackage-utils was renamed to javapackages-tools in 2012, why are you still using the old name?

Patches are not accompanied by links to Fedora or upstream bug reports.

I'd argue that the spec file fails the spec legibility rule. There are many macros whose purpose isn't obvious. For example:
%global aarch64         aarch64 arm64 armv8
...
#images stub
%global jdkimage       jdk

It's also full of typos and unintelligible comments, for example:
# elfutils ony are ok for built without AOT
I have no idea what the above means.
Or this:
# Zero-assembler build requirement.
Comments are supposed to explain something. The one above doesn't.

See: https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_Legibility .

This package would not have passed review as-is if I was the reviewer. Sorry to be blunt, but it looks like a bad copy and paste from older openjdk spec file.

Comment 57 jiri vanek 2017-10-19 08:30:10 UTC
(In reply to Dominik 'Rathann' Mierzejewski from comment #56)
> Nobody noticed that %changelog has wrong versions:
> 
> %changelog
> * Tue Oct 10 2017 Jiri Vanek <jvanek> - 1:1.9.0.0-9.b163
> - now owning dir etcjavasubdir
> 
> * Tue Oct 10 2017 Jiri Vanek <jvanek> - 1:1.9.0.0-8.b163
> - EC no longer built
> 
> * Tue Oct 10 2017 Jiri Vanek <jvanek> - 1:1.9.0.0-7.b163
> - now owning dir etcjavadir
> 
> * Thu Oct 05 2017 Jiri Vanek <jvanek> - 1:1.9.0.0-4.b163
> - config files moved to etc
> 
> * Tue Aug 29 2017 Michal Vala  <mvala> - 1:1.9.0.0-3.b163
> - changed  archinstall to i686
> - added ownership of lib/client/
> 
> while package versions are:
> 9.0.0.181-1.fc28
> 9.0.0.181-2.fc28
> ...
> 9.0.0.181-9.fc28
> 
> Where are the %changelog entries for -1, -2, -5 and -6?

IIRC those were only bumps of releases to allow testing of new config files.
 
> 
> You could also have dropped the Epoch: from the package, since it's a
> completely new package and kept it only in the virtual Provides:. Actually,
> the use of Epoch: in a NEW package must be justified in the review. It
> wasn't. See https://fedoraproject.org/wiki/Packaging:Versioning .

I do not see reason to diverge from jdk8. We keep epoch injdk packages since jdk5.  I would be afraid to lower it. Maybe my fear is vain, but it would need proof.

> 
> Requires: xorg-x11-fonts-Type1
> Seriously? Does Java 9 still require legacy Type1 fonts in 2017?

Seriously, yes. Java is depnding on X11 for now. Although work has been initiated, its not sure when we will see X-impl-independent port.
> 
> # Require jpackage-utils for ownership of /usr/lib/jvm/
> Requires: jpackage-utils
> jpackage-utils was renamed to javapackages-tools in 2012, why are you still
> using the old name?
> 

Have you noted that it is using javapackages-tools in all except one place?
Thats obvious overlook. Patch/bugreport or any other  "let me know" is welcomed.
I fixed it now for jdk8 and 9.

> Patches are not accompanied by links to Fedora or upstream bug reports.
> 
> I'd argue that the spec file fails the spec legibility rule. There are many
> macros whose purpose isn't obvious. For example:
> %global aarch64         aarch64 arm64 armv8

Whats wrong with it? Its all versions of arm64 self identifications I ever encountered in all places I was rebuilding this SRPM.
> ...
> #images stub
> %global jdkimage       jdk

What do you see against this macro?
> 
> It's also full of typos and unintelligible comments, for example:

Please, contribute. Fix welcomed.

> # elfutils ony are ok for built without AOT
> I have no idea what the above means.

s/ony/only

> Or this:
> # Zero-assembler build requirement.
> Comments are supposed to explain something. The one above doesn't.

It does, if you know what is Zero.
> 
> See: https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_Legibility .
> 
> This package would not have passed review as-is if I was the reviewer. Sorry
> to be blunt, but it looks like a bad copy and paste from older openjdk spec
> file.

Unluckily, you weren't.  Be sure that non of the nits, you spotted were left intentionally or with evil purposes. I will fix everything you see invalid.
Probably except the linking of usptream patches which is mess to do


Note You need to log in before you can comment on or make changes to this bug.