Skip to content

Conversation

@gnu-andrew
Copy link

Currently, the makefile excludes building libsystemconf on any platform but Linux, but the JNI code in SystemConfiguration.java then unconditionally tries to load the library.

We could make this conditional on running on Linux, but we still have the problem of a native method in the class that doesn't resolve. The best solution seems to be to build the library, but provide a simple JNI_FALSE return for non-Linux systems. This also leaves the door open for an implementation on these platforms in future.

@martinuy
Copy link

One question, do our non-Linux builds (which I assume it's Windows) already include the RPM patches that, for example, introduce the SystemConfigurator.java?

@gnu-andrew
Copy link
Author

One question, do our non-Linux builds (which I assume it's Windows) already include the RPM patches that, for example, introduce the SystemConfigurator.java?

I don't know. @akashche?

My aim with this patch is to give us a clean set of PR tests. The alternative would be to turn off Windows & Mac builds, but knowing we don't break them seems useful.

Incidentally, it's just one patch now: https://src.fedoraproject.org/rpms/java-17-openjdk/c/756a991906919de0d448abf84e9a66cf96dc6afd?branch=rawhide

@judovana
Copy link

Hello. thanx a lot. Sorry for troubeling you with this.
I had jsut enabled basic upstream qa upon fips brnaches, and that includes windows buildability.

As for downstream, I really do not know. Windows rh builds are based on srpm, but then half of the patches is gone. @stooke woud know more now.

@franferrax franferrax requested review from franferrax and removed request for martinuy August 11, 2022 18:33
Copy link

@franferrax franferrax left a comment

Choose a reason for hiding this comment

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

Hi @gnu-andrew, it looks good to me.

I'm wondering about how much is the benefit of including a build-stage + test-suite running on platforms whose official build don't (or shouldn't) include changes from the FIPS branches we are testing, but it doesn't harm.

@gnu-andrew
Copy link
Author

Hi @gnu-andrew, it looks good to me.

I'm wondering about how much is the benefit of including a build-stage + test-suite running on platforms whose official build don't (or shouldn't) include changes from the FIPS branches we are testing, but it doesn't harm.

Well, you've kind of answered the question; it's ensuring they don't include the changes and still work. I think it's a good test that our changes aren't breaking things or making assumptions that the FIPS mode always operates a certain way. We've had issues raised before where we broke the scenario where FIPS was disabled on Linux. This is why the PRs now test both on and off, and I think unavailable is a good additional test as well. Prior to this change, we couldn't even build on the other platforms and so also couldn't test that the code still operated correctly.

@gnu-andrew
Copy link
Author

Thanks for the review. Merging.

@gnu-andrew gnu-andrew merged commit bb46af0 into rh-openjdk:fips-17u Aug 12, 2022
gnu-andrew added a commit that referenced this pull request Aug 29, 2022
Build the systemconf library on all platforms, simply returning JNI_FALSE if !LINUX
Reviewed-by: @franferrax
gnu-andrew added a commit that referenced this pull request Apr 4, 2023
Build the systemconf library on all platforms, simply returning JNI_FALSE if !LINUX
Reviewed-by: @franferrax
gnu-andrew added a commit that referenced this pull request Aug 22, 2023
Build the systemconf library on all platforms, simply returning JNI_FALSE if !LINUX
Reviewed-by: @franferrax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants