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

duck: fix build for Linux #83110

Closed
wants to merge 1 commit into from
Closed

Conversation

danielnachun
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@danielnachun danielnachun added linux to homebrew-core Migration of linuxbrew-core to homebrew-core CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. labels Aug 11, 2021
@danielnachun
Copy link
Member Author

This formula had depend_on :macos in linuxbrew-core, but the CLI component supports Linux, and upstream even provides RPM and DEB packages. I had to hack the build.xml file so that it would build an app-image target which just provides the binaries and libraries in a directory we can install to libexec.

I will open an issue on the upstream Trac for Cyberduck (https://trac.cyberduck.io/query) once the CI passes here asking for guidance on how to make a PR to properly add app-image support to the build.xml. Merely adding app-image as a build type is not quite enough for it to work, as their call to jpackage includes 3 flags which are only valid for RPM or DEB targets. I circumvented this for now by simply deleting those lines from the build.xml (we don't care about breaking RPM or DEB in our setup), but I want to get guidance from upstream about how to add app-image support without breaking the other targets.

@danielnachun danielnachun added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Aug 11, 2021
@danielnachun
Copy link
Member Author

danielnachun commented Aug 12, 2021

Ironically this now builds fine on Linux but fails on macOS because of universal/non-native binaries.

It seems this does not actually support native ARM yet: https://trac.cyberduck.io/ticket/11101, which is blocked by iterate-ch/rococoa#20. So I think this needs depends_on :x86_64.

As for the universal binaries, I think I have seen elsewhere that these are actually from macOS itself? I'm not sure how we've handled this before.

@carlocab
Copy link
Member

carlocab commented Aug 12, 2021

JavaNativeFoundation.framework is likely copied from an Xcode SDK, which is why it's universal. Not sure about libjnidispatch.dylib. If we can figure out where duck is getting these from it'd be a lot easier to figure out what to do about them. Will try to poke at their build system a bit.

Agreed that this needs depends_on arch: :x86_64.

@cho-m
Copy link
Member

cho-m commented Aug 12, 2021

There is a new bump PR for this formula #83146 which will probably fail for same reason on macOS.

Can either combine into single PR, or will need to fix to one PR and then rebase the other.

@danielnachun
Copy link
Member Author

I can confirm that the libjnidispatch.dylib is indeed being downloaded as a prebuilt binary:

Downloading from maven.cyberduck.io-release: https://repo.maven.cyberduck.io.s3.amazonaws.com/releases/net/java/dev/jna/libjnidispatch/5.7.0/libjnidispatch-5.7.0-x86_64.dylib
Downloaded from maven.cyberduck.io-release: https://repo.maven.cyberduck.io.s3.amazonaws.com/releases/net/java/dev/jna/libjnidispatch/5.7.0/libjnidispatch-5.7.0-x86_64.dylib (291 kB at 293 kB/s)

I think I've figured how to build libjnidispatch.dylib as a resource from upstream: https://github.com/java-native-access/jna, and now I'm just cleaning up the changes before trying them here.

@BrewTestBot BrewTestBot added the java Java use is a significant feature of the PR or issue label Aug 14, 2021
@danielnachun
Copy link
Member Author

I've now pushed some major changes that should build libjnidispatch from source on macOS and Linux. The build system for java-native-access took quite a while to understand, and I'll explain below:

  1. A call is made to ant to generate some platform-specific native JNA C header files from the JAR files.
  2. A bash build script (build.sh) is generated for building the native library with a Makefile.
  3. The JNA header files and build script are packaged with rest of the native library C source (which is not platform-specific) into a zip. Unfortunately the original files are deleted after the zip is made.
  4. The end user is supposed to extract the zip file, then run build.sh to actually build the final shared library. The shared library needs libffi, which is supposed to be found by pkg-config (which works on Linux but not on macOS). On Linux, the library ends in .so, but on macOS it ends in .jnilib and is later renamed to .dylib.

Initially I just extracted the zip archive after it was made and then ran the build script inside of the folder. However, I realized that the naming of the zip file is rather inconvient to work with programmatically - it changes based on platform, and based on the JNI version, which cannot easily be extracted from anywhere. I also found it cumbersome to have to unzip a file like this. My workaround for this was to just delete the line that packages the header files into the zip file. Along with a small change in the build.sh, this was sufficient to make that script work as desired. The other fixes I made were for some bugs on macOS.

@carlocab I'm pretty sure after checking the logs that maven completely ignores the host JDK and Framework installations altogether:

Downloading from maven.cyberduck.io-release: https://repo.maven.cyberduck.io.s3.amazonaws.com/releases/ch/iterate/JavaNativeFoundation/81/JavaNativeFoundation-81.zip
Downloading from maven.cyberduck.io-release: https://repo.maven.cyberduck.io.s3.amazonaws.com/releases/net/adoptopenjdk/jre/15.0.1/jre-15.0.1.zip

We happened to catch libjnidispatch.dylib and JavaNativeFoundation becuase they were built as universal binaries, but technically the JRE this is using is also not the brewed one, but a copy downloaded from AdoptOpenJDK.

There is a Github repo for JavaNativeFoundation from none other than Apple themselves: https://github.com/apple/openjdk/tree/xcodejdk14-release/apple/JavaNativeFoundation. That is probably what is being used to build the universal JavaNativeFoundation artifact. I think we can also build this from source as another resource.

For now I think we should focus on getting the universal native binaries built from source so that this passes our audit. However, it would be great for formulae like this to make sure we are building all the native binaries from source.

@danielnachun
Copy link
Member Author

I did it! JavaNativeFoundation was quite trivial to build from source, and after replacing that in the Frameworks folder, this passes the audit! To me this gives us hope that we'll be able to fix these issues in other formulae using the same approach. As I mentioned before we can consider when it makes sense to create formulae for native dependencies, and when it should just be a resource.

SMillerDev
SMillerDev previously approved these changes Aug 15, 2021
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

It's a huge change and I hope those two resources will become formula, but for now this should be enough.

Comment on lines +44 to +52
resource "jna" do
url "https://github.com/java-native-access/jna/archive/refs/tags/5.8.0.tar.gz"
sha256 "97680b8ddb5c0f01e50f63d04680d0823a5cb2d9b585287094de38278d2e6625"
end

resource "JavaNativeFoundation" do
url "https://github.com/apple/openjdk/archive/refs/tags/iTunesOpenJDK-1014.0.2.12.1.tar.gz"
sha256 "e8556a73ea36c75953078dfc1bafc9960e64593bc01e733bc772d2e6b519fd4a"
end
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment explaining how to update these resources? This can be in a follow-up syntax-only PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am actually thinking we should make these resources into formulae. I am going to update this to the newest version and then I'd like to make formulae out of the resources and remove those parts of this formulae. All we'd have to do if the resources are formulae is link the necessary files into libexec after deleting the pre-packaged ones. The resources themselves both use GitHub so we can use livecheck to keep them up to date if they are formulae.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me. Thanks for working on this!

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you approve again? I had to dismiss the previous approval from @SMillerDev when I updated the version.

@danielnachun
Copy link
Member Author

It's a huge change and I hope those two resources will become formula, but for now this should be enough.

For jna making a formula should be pretty easy now that I figured out its build system. It is cross-platform and used in many packages so I would not be surprised if we end up using it elsewhere. For JavaNativeFoundation, the only snag here is that the repo it is part of doesn't quite meet our notability requirements. Perhaps we can make an exception?

@SMillerDev
Copy link
Member

I'm fine with an exception if other tools need it.

@BrewTestBot
Copy link
Member

:shipit: @danielnachun has triggered a merge.

ignore_missing_libraries "libjvm.so"
end

resource "jna" do
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break when we update the JNA dependency upstream and this formulae will take the older version instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for following up on this. I have 2 thoughts about this:

  1. These resources will be made into formulae soon, which will be updated automatically with livecheck and brew bump.
  2. The only thing we're actually taking from JNA is the dylib/so, which is built with -compatibility_version 5 such that it should maintain ABI compatibility as long as the major version remains at 5. If and when a version 6 is released which breaks ABI compatibility, we'd either rebuild duck against version 6 (if it has been updated), or if it takes a while for upstream to update, create a jna@5 formula to use in the interim.

I'm not intimately familiar with the details of how JNA is implemented, but it's generally rare for native dynamic libraries to break ABI compatibility between minor versions, let alone point releases. I'm certainly open to being proven wrong on this but we'll have to see how the next update goes.

I should also add here that we are pushing harder to try to avoid downloading pre-built native binaries because doing so has both security and compatibility implications. However we're hoping to do so without huge inconvenience to upstream projects. In this particular case, no changes are needed in how the package is built upstream - we simply delete the binaries downloaded by Maven and replace them with ones that will eventually come from formulae.

@danielnachun danielnachun deleted the duck branch August 22, 2021 18:20
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. java Java use is a significant feature of the PR or issue linux to homebrew-core Migration of linuxbrew-core to homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants