-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update rpminspect test #4868
Update rpminspect test #4868
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is OK moving from the plugin to the compiler properties. Additionally, it simplify the version update but I do not think it is good to get the release version from the java
available.
Additionally, default fedora check verify bytecode version 55 for fc40 (source here, is there a reason to be more strict considering this apply to the pki and all related libraries?
pki.spec
Outdated
%pom_xpath_set \ | ||
"pom:properties/pom:maven.compiler.release" \ | ||
"$JAVA_VERSION" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think the compiled bytecode release should be related to the default JVM available in the build environment. I think we should set a value based on the language and support all the version from that.
tests/pki-rpminspect.yaml
Outdated
- default: 65 | ||
- fc39: 55 | ||
- fc40: 55 | ||
- default: 43 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are the values defined for fedora, are they needed or can be removed here to get them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like they can be removed.
d442069
to
a170126
Compare
Thanks for the feedback! Please see the updated PR. So the target Java version is hard-coded to 17. I'm just wondering if |
It should build without problems for version 17 but we can modify the default in case. An alternative could be to use only the source option instead of release (they map to the javac options) but I would not consider for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The pom.xml files have been updated to define the target Java version using maven.compiler.release property. The maven-compiler-plugin is no longer used so it has been removed. The pki-rpminspect.yaml has been updated to no longer define the javabytecode requirement so it will use the standard requirement for the platform. The rpminspect test has been updated to call rpminspect directly in separate steps to make it easier to inspect the failures. The rpminspect.sh is no longer used so it has been removed. Currently the test is failing because some dependencies were built with older Java bytecode versions. They need to be rebuilt with the proper version.
@fmarco76 Thanks! We can try enabling that test in the next CentOS build and see how it goes. |
Quality Gate passedIssues Measures |
The
pom.xml
files have been updated to define the target Java version usingmaven.compiler.release
property. Themaven-compiler-plugin
is no longer used so it has been removed.The
pki-rpminspect.yaml
has been updated to no longer define thejavabytecode
requirement so it will use the standard requirement for the platform.The
rpminspect
test has been updated to callrpminspect
directly in separate steps to make it easier to inspect the failures. Therpminspect.sh
is no longer used so it has been removed.Currently the test is failing because some dependencies were built with older Java bytecode versions. They need to be rebuilt with the proper version.