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

Update cross compile job #178

Merged
merged 8 commits into from
Jan 9, 2025

Conversation

pietrygamat
Copy link
Collaborator

Cross compilation Github action got obsoleted again:

  • The Dockcross images were rebased to Debian Bookworkm, whose default repositories no longer offer openjdk-11-jdk. As the cross compilation does not really care about the class level, it does not hurt us to update to openjdk-17-jdk.
  • The macos-11 runner type got obsoleted, the oldest now being macos-latest, aka macos-14.

…ockcross repository after rebasing to Debian Bookworm
@tresf
Copy link

tresf commented Dec 6, 2024

As the cross compilation does not really care about the class level, it does not hurt us to update to openjdk-17-jdk.

Interesting, I hadn't thought of this. No objections.

  • The macos-11 runner type got obsoleted, the oldest now being macos-latest, aka macos-14.

According to the this wiki article, the oldest is now macos-13. https://github.com/actions/runner-images/blob/main/README.md#available-images. Since we coerce the SDK version through actions, I think this is fine left as macos-latest.

The actions code that coerces this is here:

if: ${{ matrix.sdk-version == 'MacOSX10.9.sdk' }}
run: |
wget -qO- https://github.com/phracker/MacOSX-SDKs/releases/download/11.3/MacOSX10.9.sdk.tar.xz \

... however, I'm unsure as to whether or not this will impact the ARM64 builds, they don't seem to have an SDK hard-coded...

Perhaps we should remove the conditional and change the URL to use the variable?

-          if: ${{ matrix.sdk-version == 'MacOSX10.9.sdk' }} 
[...]
- wget -qO- https://github.com/phracker/MacOSX-SDKs/releases/download/11.3/MacOSX10.9.sdk.tar.xz \ 
+ wget -qO- https://github.com/phracker/MacOSX-SDKs/releases/download/11.3/${matrix.sdk-version}.tar.xz \ 

@pietrygamat pietrygamat force-pushed the update-cross-compile-job branch from bfbf2ca to eb9542e Compare December 7, 2024 20:43
@pietrygamat
Copy link
Collaborator Author

I'm unsure as to whether or not this will impact the ARM64 builds, they don't seem to have an SDK hard-coded...
Perhaps we should remove the conditional and change the URL to use the variable?

Good point, SDKs shipped in macos-latest are now just:

Run ls -al $XCODE_16_DEVELOPER_DIR/Platforms/MacOSX.platform/Developer/SDKs
total 0
drwxr-xr-x  6 runner  staff  192 Dec  7 20:33 .
drwxr-xr-x  6 runner  staff  192 Dec  2 12:57 ..
drwxr-xr-x  7 runner  staff  224 Dec  2 12:57 MacOSX.sdk
lrwxr-xr-x  1 runner  staff   10 Dec  2 12:57 MacOSX15.1.sdk -> MacOSX.sdk
lrwxr-xr-x  1 runner  staff   10 Dec  2 12:57 MacOSX15.sdk -> MacOSX.sdk

So downloading 11.0 explicitly should happen.

@tresf
Copy link

tresf commented Dec 10, 2024

Thanks! I wanted to see if there was a way to validate that we're compiling for the platform that we expect. I've proposed a minos field to the show-file-info task:

This should allow us to review the CI logs to ensure we're compiling for the target platform that we think we are.

@tresf
Copy link

tresf commented Dec 19, 2024

Thanks! I wanted to see if there was a way to validate that we're compiling for the platform that we expect. I've proposed a minos field to the show-file-info task:

This should allow us to review the CI logs to ensure we're compiling for the target platform that we think we are.

@pietrygamat just bumping this request to PR against your fork here. It should allow us to see the target OS version from CI.

@tresf
Copy link

tresf commented Dec 19, 2024

Hmm... both CI runs on the PR show minos: 14.0, which seems like it's not working as expected (well, my PR is, but this PR isn't).

https://github.com/pietrygamat/jssc/actions/runs/12266284483/job/34224117544#step:5:186

@pietrygamat pietrygamat force-pushed the update-cross-compile-job branch from eb9542e to 263ce36 Compare December 19, 2024 20:49
@pietrygamat
Copy link
Collaborator Author

both CI runs on the PR show minos: 14.0

Found the issue, the export statement does not preserve variables between Github actions steps, we need to store them in $GITHUB_ENV.

      export MACOSX_DEPLOYMENT_TARGET=${{ matrix.macos-deployment-target }}
      export SDKROOT=$XCODE_16_DEVELOPER_DIR/Platforms/MacOSX.platform/Developer/SDKs/${{ matrix.sdk-version }}
      export CMAKE_OSX_SYSROOT=$SDKROOT

      echo "MACOSX_DEPLOYMENT_TARGET=$MACOSX_DEPLOYMENT_TARGET" >> $GITHUB_ENV
      echo "SDKROOT=$SDKROOT" >> $GITHUB_ENV
      echo "CMAKE_OSX_SYSROOT=$CMAKE_OSX_SYSROOT" >> $GITHUB_ENV

Looks like we have been doing it wrong all the time. This however shows, that the project does not build in OSX x64

@pietrygamat
Copy link
Collaborator Author

pietrygamat commented Dec 19, 2024

C is not my thing, so I have ChatGPTd the error:

  [exec] /Users/runner/work/jssc/jssc/src/main/cpp/_nix_based/jssc.cpp:575:9: error: use of undeclared identifier 'static_assert'
 [exec]         static_assert(EBADF > 0, "EBADF > 0");

and tried the first thing it suggested:

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

which fixes compilation, but the output file works weirdly with otool (no minos value)

@tresf
Copy link

tresf commented Dec 20, 2024

which fixes compilation, but the output file works weirdly with otool (no minos value)

I think minos is a newer flag, so the code at pietrygamat#5 needs to be more generic on how it matches the otool output.

If you look at the CI job, it does show the minimum OS version:

     [exec]       cmd LC_VERSION_MIN_MACOSX
     [exec]   cmdsize 16
     [exec]   version 10.8
     [exec]       sdk 10.8

Per a comment on this post, I've updated the code here: pietrygamat@3eb6b59.

@tresf
Copy link

tresf commented Dec 20, 2024

and tried the first thing it suggested:

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

This is great, thanks!

@tresf
Copy link

tresf commented Dec 20, 2024

and tried the first thing it suggested:

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

This is great, thanks!

Oh, I just realized this commit never made it to this PR, it's how I would do it. The other option if we're worried about old compilers is to use an older assertion technique but I don't think that's warranted.

@tresf
Copy link

tresf commented Dec 20, 2024

Oh, I just realized this commit never made it to this PR, it's how I would do it.

Here's where I'd place them in the CMakeLists.txt file pietrygamat@c7eecb2.

Echo minos value of binary when compiling for macOS
@tresf
Copy link

tresf commented Dec 20, 2024

@pietrygamat thanks for merging the minos feature. I'm scratching my head though... why do your own runs show the correct values, but the CI runs in this PR don't?

Your CI run:
https://github.com/pietrygamat/jssc/actions/runs/12436478750/job/34724409810#step:6:206

show-file-info:
     [echo] File information:
     [exec] /Users/runner/work/jssc/jssc/target/cmake/natives/osx_64/libjssc.dylib: Mach-O 64-bit dynamically linked shared library x86_64
     [exec]   version 10.8
     [exec]       sdk 10.8
     [echo] 

This CI run:
https://github.com/java-native/jssc/actions/runs/12436451770/job/34724319570#step:5:161

show-file-info:
     [echo] File information:
     [exec] /Users/runner/work/jssc/jssc/target/cmake/natives/osx_64/libjssc.dylib: Mach-O 64-bit dynamically linked shared library x86_64
     [exec]     minos 14.0
     [exec]       sdk 14.5
     [echo] 

@pietrygamat
Copy link
Collaborator Author

@tresf it seems the pipeline definitions are loaded from repo's master branch only, and not from the branch selected for the run.

@pietrygamat
Copy link
Collaborator Author

Well, that would be the case for cross-compile manually triggered job. But the PRs are triggering a different workflow, where the env variables are not set.

@tresf
Copy link

tresf commented Dec 20, 2024

But the PRs are triggering a different workflow

Ok. I thought this may be happening because the CI step numbers differ. I'm a bit confused by this but I assume it'll be sorted prior to merging this PR.

@pietrygamat
Copy link
Collaborator Author

OK, I have updated the workflow that is triggered automatically for PRs with the same configuration. Should we be worried about these deprecation warnings in OSX x86_64 build?

@tresf
Copy link

tresf commented Dec 24, 2024

OK, I have updated the workflow that is triggered automatically for PRs with the same configuration. Should we be worried about these deprecation warnings in OSX x86_64 build?

In classic Apple fashion, there's nearly no information about this. "Text based stubs" or ".tbd" files are best described here: https://github.com/fanquake/core-review/blob/master/tbd-stubs.md.

Pasting the CI's warnings here for others:

ld: warning: Support for dylib stub (MH_DYLIB_STUB) file '/Applications/Xcode_16.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/lib/libc++.1.dylib' is deprecated, text-based stubs should be used instead (with GENERATE_TEXT_BASED_STUBS build setting, or tapi tool)

Apple does document this flag, but without knowledge of the history and/or inner workings of this flag, understanding it's implication is difficult.

I took the question to meta.ai, which seems to offer some more insight... quoting:


Compatibility

The GENERATE_TEXT_BASED_STUBS build setting is compatible with macOS 10.12 (Sierra) and later versions.

Minimum Deployment Target

To use GENERATE_TEXT_BASED_STUBS, your project's minimum deployment target should be set to macOS 10.12 or later.
Here's a summary of the compatibility:
macOS Version | Compatibility -- | -- 10.12 (Sierra) and later | Compatible 10.11 (El Capitan) and earlier | Not compatible
By ensuring your project's minimum deployment target is set to macOS 10.12 or later, you can safely use the GENERATE_TEXT_BASED_STUBS build setting to generate text-based stubs for your dylib.

... so this means that we can do ONE OF three things:

  1. Toggle this on, assuming the compiler supports it, but it's likely to cause issues with versions of macOS older than 10.12. (or)
  2. Generate these manually using tapi. (or)
  3. Ignore this and hope Apple doesn't break them.

If we're to fix it now, we may consider bumping our minos version requirement to 10.12 as well.

As to the ACTUAL impact of this incompatible, I'm uncertain. I would assume that macOS will continue to read these stubs from the Mach-O Headers for the foreseeable future and break it in a major OS update.

It's also possible that they've already killed these on newer (e.g. M1) OS flavors and that it's a benign warning for an architecture that'll never actually see the technology killed off.

From a project perspective, I'm OK ignoring the warning for now. Worst case, users complain and we bump the minos and make a new release.

@pietrygamat
Copy link
Collaborator Author

From a project perspective, I'm OK ignoring the warning for now.

So it's ready to merge then?

@tresf
Copy link

tresf commented Jan 9, 2025

From a project perspective, I'm OK ignoring the warning for now.

So it's ready to merge then?

Yes. If and when Apple breaks this, we can revisit (or it will go away naturally when our SDK raises in version number)

@tresf tresf self-requested a review January 9, 2025 15:42
@tresf tresf merged commit 4fcddd8 into java-native:master Jan 9, 2025
13 checks passed
@pietrygamat pietrygamat deleted the update-cross-compile-job branch January 29, 2025 19:38
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.

2 participants