Skip to content
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

[#40] CI update, builds with IBM Semeru and Java 11, 17 #41

Merged
merged 1 commit into from
Apr 23, 2022

Conversation

bmarwell
Copy link
Contributor

Some hints:

  • Because these are a lot of builds, I did a pre-step with only Java 8 Temurin. If that fails, no need to build the others.
  • The others use the matrix as expected. Excluded: The build which was already done.
  • Build with IBM Semeru is a bit special. In contrast to Zulu and Temurin, it is not based on hotspot but Eclipse OpenJ9 and might have different encryption settings: algorithm aliases, defaults, etc.
    Therefore I consider it highly valuable for anything security-related.
  • Build with recent JDKs should be a no-brainer.
  • I recommend to exclude some Mac builds to speed up the build.
  • Semeru is not ready yet (IBM Semeru (OpenJ9) Support actions/setup-java#289), using adopt-openj9 until then.

@bmarwell
Copy link
Contributor Author

The failing checks in 11/17:

 80 [INFO] --- keytool-maven-plugin:1.6-SNAPSHOT:generateKeyPair (default) @ MKEYTOOL-10 ---
 81 [INFO] /bin/sh -c cd '$HOME/git/mojohaus/keytool-maven-plugin/keytool-maven-plugin/target/it/MKEYTOOL-10' && '$HOME/.sdkman/candidates/java/17.0.2-tem/bin/keytool' '-genkeypair' '-v' '-keystore' '$HOME/git/mojohaus/keytool-maven-plugin/keytool-maven-plugin/target/it/MKEYTOOL-10/ta    rget/keystore.jks' '-storepass' 'secret' '-alias' 'alias' '-dname' 'cn=Test Development, ou=Development, L=Herzele, ST=VL, o=FrankCornelis, c=BE' '-keypass' 'secret'
 82 [INFO] Keytool-Fehler: java.lang.Exception: The -keyalg option must be specified.                                                                                                                                                                                                                                   
 83 [INFO] java.lang.Exception: The -keyalg option must be specified.
 84 [INFO]  at java.base/sun.security.tools.keytool.Main.doCommands(Main.java:1168)
 85 [INFO]  at java.base/sun.security.tools.keytool.Main.run(Main.java:415)
 86 [INFO]  at java.base/sun.security.tools.keytool.Main.main(Main.java:408)

Fix: -keyalg parameter.

@bmarwell bmarwell requested a review from olamy April 22, 2022 17:04
uses: actions/setup-java@v3
with:
distribution: temurin
java-version: 8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 11 here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do. Any particular reason though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decrease usage of Java 8 😜 - 11 should be just faster. Personally i would use 17

@bmarwell
Copy link
Contributor Author

@slachiewicz what do you think about excluding Zulu on Mac?
Mac jobs tend to fail due to timeouts frequently.

@slachiewicz
Copy link
Member

maybe let's use defaults for MacOS ie. Temurin?
https://github.com/actions/virtual-environments/blob/main/images/macos/macos-11-Readme.md#java

@bmarwell bmarwell merged commit 2491201 into master Apr 23, 2022
@bmarwell bmarwell deleted the #40_ci_update branch April 23, 2022 10:49
@olamy
Copy link
Member

olamy commented Apr 24, 2022

why do you merge that when the checks are still failing?

@bmarwell
Copy link
Contributor Author

Because this PR was not about solving the Java 17 incompatibility, just about setting up the CI properly. See above, we identified the reason which is out of the scope of this PR.

Will create another issue for the.
But.
See the other issues: we are thinking about using the Keystore API instead of the executable. So even less reason to fix this one test here.

@olamy
Copy link
Member

olamy commented Apr 24, 2022

still doesn't make sense to merge a PR which breaks.
just fix the issues then merge it but the master in a working status.

@bmarwell
Copy link
Contributor Author

It didn't break it. It wasn't working with JDK 17 before my PR. It just didn't show up.

@olamy
Copy link
Member

olamy commented Apr 24, 2022

I mean why adding 17 if it break??(9e0adf3)
if you make a requirement to build with 17 by adding it in the build file you must fix it as well or you do not add it.
if it's not in the build file it means it's not requirement (yet) but we know it doesn't work.
Now with the current situation if someone make a PR the PR will fail.

@bmarwell
Copy link
Contributor Author

Oh I see what you mean.

I meant to fix it afterwards right away but had to go spontaneously. Will fix it later today. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants