Alon Bar-Lev has posted comments on this change.

Change subject: packaging: Find and check JVM during setup (#834436)
......................................................................


Patch Set 3: (4 inline comments)

....................................................
File packaging/fedora/setup/engine-setup.py
Line 1913:         logging.debug("The java version \"%s\" is not supported." % 
javaVersion)
Line 1914:         return False
Line 1915: 
Line 1916:     # Check that it is an OpenJDK:
Line 1917:     match = re.search(r'^OpenJDK .*$', str(javaErr), re.MULTILINE)
We can warn... but not allow?

For example, I worked with UBS which insisted on installing sun jdk on servers 
and not use openjdk... one major point of java is the jvm choice.
Line 1918:     if not match:
Line 1919:         logging.debug("The Java launcher \"%s\" is not OpenJDK." % 
javaLauncher)
Line 1920:         return False
Line 1921: 


Line 1921: 
Line 1922:     # It passed all the checks, so it is valid JVM:
Line 1923:     return True
Line 1924: 
Line 1925: def checkJdk(jvmPath):
I don't understand.

Why even bother and not use the JRE only?
Line 1926:     # We assume that this JVM path has already been checked and that 
it
Line 1927:     # contains a valid JVM, so we only need to check that it also
Line 1928:     # contains a Java compiler:
Line 1929:     javaCompiler = os.path.join(jvmPath, "bin", "javac")


Line 1949:                     targetPath = targetName
Line 1950:                 else:
Line 1951:                     targetPath = os.path.join(javaDir, targetName)
Line 1952:                 if not os.path.islink(targetPath):
Line 1953:                     jvmLinks.append(filePath)
I see... this what happens when distribution is primitive.

I also don't think this alternative implementation behaviour is something that 
one can rely on.

But primitive is primitive. This whole function can be replaced with one simple 
command at gentoo, if we use jdk7 and only icedtea.

 controller.CONF["JAVA_HOME"]=$(java-config --select-vm=icedtea-7 -g JAVA_HOME)

So it is not that matter if you think the rhel/fedora is appropriate.
Line 1954: 
Line 1955:     # For each possible JVM path check that it really contain a JVM 
and
Line 1956:     # that the version is supported:
Line 1957:     jvmLinks = [x for x in jvmLinks if checkJvm(x)]


Line 1964:         jvmLinks = jreLinks
Line 1965: 
Line 1966:     # Sort the list by the lenght of the link in order to use the
Line 1967:     # shorter one:
Line 1968:     jvmLinks.sort(key=lambda jvmLink: len(jvmLink))
alphabetical is better.
Line 1969: 
Line 1970:     # We need at least one value in the list:
Line 1971:     if not jvmLinks:
Line 1972:         logging.error("Can't find any supported JVM.")


--
To view, visit http://gerrit.ovirt.org/7549
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib279ea8111825ec35a25f9ba6ee37b481ed4354f
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Alex Lourie <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Moran Goldboim <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to