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

fix native compile error due to use of ant if,unless #1302

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

bhamail
Copy link
Contributor

@bhamail bhamail commented Jan 28, 2021

I think this will fix the build failure that occurred recently in the JNA-maven build. Was missing some ant namespace stuff to enable nifty new things.

@tresf
Copy link
Contributor

tresf commented Jan 29, 2021

Thanks, completely overlooked this.

@matthiasblaesing
Copy link
Member

If I'm not mistaken, the native/build.xml will need the same ARCH fix, that build.xml received (adf2c97).

Another question, that runs in my mind: Why do we have two definitions of the same build? We should unify that, but I admit that is out of scope for this PR.

@tresf
Copy link
Contributor

tresf commented Jan 29, 2021

fix, that build.xml received (adf2c97).

This commit confuses me, but I don't want to get too off-topic. The generic suffix handling should be simpler (and I'd expect, better), minus whatever bug it introduced. Probably not worth nitpicking, but the pattern matching seems less likely to falter over time when compared to the hard-coded platforms.

Why do we have two definitions of the same build

I too found this all to be quite redundant. Ant handles imported build files well, so the redundancy seems -- from a high level -- unwarranted.

@tresf
Copy link
Contributor

tresf commented Jan 29, 2021

fix, that build.xml received (adf2c97).

This commit confuses me, but I don't want to get too off-topic. The generic suffix handling should be simpler (and I'd expect, better), minus whatever bug it introduced. Probably not worth nitpicking, but the pattern matching seems less likely to falter over time when compared to the hard-coded platforms.

Oh, was it the amd64 being wiped out? That probably could have been fixed here instead. We tried our hardest to standardize the suffixes throughout to make macOS to mimic the most common naming convention.

Yeah it seems it's used in quite a few places. 😕

@matthiasblaesing
Copy link
Member

@tresf at the time that I backed it out, it was intended as a change with minimal risk. It restored the original logic on the non-darwin platforms, while retaining the updated behavior for darwin. A cleanup followup can be of course done.

Copy link
Member

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Good to merge if it fixes the build. My initial assessment, that fixes for windows need to be included were wrong, as the prefix in this build file is not correctly determined. The TL;DR version is that the build script is broken:

  • native/build.xml still calls into javah, which is gone in recent JDKs
  • the version of the native library is not correct
  • the checksum for the api is not correct

and most probably other fixes are missing.

@bhamail bhamail merged commit 58735db into java-native-access:master Jan 29, 2021
@bhamail bhamail deleted the fix_ant_compile-native branch January 29, 2021 20:18
@bhamail
Copy link
Contributor Author

bhamail commented Jan 29, 2021

Good to merge if it fixes the build. My initial assessment, that fixes for windows need to be included were wrong, as the prefix in this build file is not correctly determined. The TL;DR version is that the build script is broken:

  • native/build.xml still calls into javah, which is gone in recent JDKs
  • the version of the native library is not correct
  • the checksum for the api is not correct

and most probably other fixes are missing.

Thanks for looking at it. I went ahead and merged (to get build happy), and hope to get some time to look over the other issues to mentioned.

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.

3 participants