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

Use github actions for testing PRs #151

Merged
merged 2 commits into from
Nov 10, 2023
Merged

Use github actions for testing PRs #151

merged 2 commits into from
Nov 10, 2023

Conversation

tresf
Copy link

@tresf tresf commented Nov 9, 2023

Adds dockerless CI jobs to compile and test for new PRs; removes Travis-CI which stopped running a while ago.

TODO:

  • Discuss SerialNativeInterfaceTest.reportsWriteErrorsAsIOException is failing in CI
    • Since #136 Pass-Through JNI exceptions to caller in waitEvents #137 GitHub actions fail no matter what I try. @hiddenalpha thoughts welcome, but the test passes locally on Mac and Linux. I can't figure out why GitHub actions allows me to call write(fd) on a file descriptor of -1. The test is currently disabled if running in CI. I'm fine keeping it this way, but I'd like your opinion first.
  • Windows tests for this are also failing. In hindsight, this is expected, the windows writeBytes() doesn't yet have any exception throwing. I'm not sure if it's worth adding. Thoughts welcome. The test is currently disabled if running on Windows.. Conversation deferred for another time.
  • Add missing (sane) Maven profiles + Cmake toolchains for riscv64, riscv32. At time of writing this, Ubuntu doesn't have a toolchain for riscv32, so I've omitted it from the matrix. If that changes, we can add it.

Preview:

image

@pietrygamat
Copy link
Collaborator

pietrygamat commented Nov 9, 2023

@tresf regarding failing tests e.g. https://github.com/tresf/jssc/actions/runs/6807689668/job/18510958314
The build logs for ubuntu (x86_64, g++) shows the action order a bit off, and the tests fail, because they are not testing the correct version of the library:

Here's whats going on:

  1. Decide to run tests
  2. Copy resources (including precompiled binaries to target/classes
  3. Compile new natives, copy them to target/cmake/natives and src/main/resources-precompiled (too late!)
  4. Copy test resources from src/test/resources (nothing todo, we don't have any)
  5. Run test. Tests run in /target/classes, so the libs copied here in step 2 are loaded, because that's the default priority for native lib loader.

To fix this we should do one of:

  • Configure cmake to copy natives to test/resources , not just main/resources-precompiled
  • Configure maven to copy natives after compilation
  • Set jssc.boot.library.path to path that we know has the correct version.

First option seems cleanest and easiest - see: pietrygamat@ca26e1a https://github.com/pietrygamat/jssc/actions/runs/6818119903/job/18543034333

@tresf

This comment was marked as outdated.

@tresf
Copy link
Author

tresf commented Nov 9, 2023

Hmm... I feel copying all natives to the test directory is an over-complicated approach, when we only need one library to complete the native tests. jssc.boot.library.path was really intended for exactly this purpose.

@pietrygamat
Copy link
Collaborator

Hmm... I feel copying all natives to the test directory is an over-complicated approach, when we only need one library to complete the native tests. jssc.boot.library.path was really intended for exactly this purpose.

That's effectively the same thing, because native compilation creates just one file per run.

@tresf
Copy link
Author

tresf commented Nov 9, 2023

Hmm... I feel copying all natives to the test directory is an over-complicated approach, when we only need one library to complete the native tests. jssc.boot.library.path was really intended for exactly this purpose.

That's effectively the same thing, because native compilation creates just one file per run.

Hmm... well, since we set the native library path in src/test/java/jssc/bootpath/ManualBootLibraryPathTest.java I feel like keeping this logic introduces less guesswork for future tests. I'm open to arguments to the contrary. Per cd632f9, I've moved the jssc.boot.library.path into a reusable helper.

@tresf
Copy link
Author

tresf commented Nov 9, 2023

That's effectively the same thing

I understand, but it introduces yet another copy to keep track of. I think testing the one we just built is better. Of course, this can fail if we skipped building too, which would fail the existing ManualBootLibraryPathTest. I'm not sure if we properly skip that when natives aren't built.

@pietrygamat
Copy link
Collaborator

pietrygamat commented Nov 9, 2023

yet another copy to keep track of.

Yeah, this is the confusing part for me. WHY do we even have the first copy in

COMMAND ${CMAKE_COMMAND} -E copy_directory ${CMAKE_CURRENT_BINARY_DIR}/natives/ ${CMAKE_CURRENT_SOURCE_DIR}/src/main/resources-precompiled/natives/
in the first place? CMake builds its artifacts to /target/cmake/natives, and keeps clean distinction between src and target, then it is explicitly broken. It looks to me it was an attempt to do the same thing I did today.

If we were to skip copying and use jssc.boot.library.path - it's fine by me by all means, but then let's please clean up this part too - and stop casually modifying source tree with build artifacts, as it may lead to accidental commits of broken artifacts.

Now that I think about it, we should only have one copy from cmake's /target/cmake/native to java's /target/classes (and not /src/). Jar's would be packaged correctly, test would execute as expected, and src-target convention would not be broken.

pom.xml Outdated Show resolved Hide resolved
@tresf
Copy link
Author

tresf commented Nov 10, 2023

Hmm... I feel copying all natives to the test directory is an over-complicated approach, when we only need one library to complete the native tests. jssc.boot.library.path was really intended for exactly this purpose.

That's effectively the same thing, because native compilation creates just one file per run.

Sorry, this statement is right. I misunderstood.

@tresf
Copy link
Author

tresf commented Nov 10, 2023

in the first place? CMake builds its artifacts to /target/cmake/natives, and keeps clean distinction between src and target, then it is explicitly broken. It looks to me it was an attempt to do the same thing I did today.

Copying to src is so that it can (rather easily) be committed back into the source tree. We could argue that this task is best left as a dedicated maven step, but it's helpful for people that are forking the project and want to know the source path that gets updated when a native is rebuilt, so I vote to keep it for now.

If we were to skip copying and use jssc.boot.library.path - it's fine by me by all means, but then let's please clean up this part too - and stop casually modifying source tree with build artifacts, as it may lead to accidental commits of broken artifacts.

I accidentally committed one in this branch. I'm welcome to ideas. One workaround is to add the directory to the .gitignore so that we must explicitly commit it.

Now that I think about it, we should only have one copy from cmake's /target/cmake/native to java's /target/classes (and not /src/). Jar's would be packaged correctly, test would execute as expected, and src-target convention would not be broken.

THIS is the fix. I'm not sure if we should keep around the cmake/natives copy, but copying back to classes is the puzzle piece I was missing. I don't hate having a second copy though, as it ensures that jssc.boot.library.path is working.

8e6bc4a properly copies the recently compiled native library to target/classes/natives.

@tresf
Copy link
Author

tresf commented Nov 10, 2023

I think this is finally ready for merge. I was hoping to hear from @hiddenalpha in regards to Windows exception testing, but we can discuss that in another thread.

@pietrygamat in regards to polluting the source tree, I think that's a good conversation. Do you mind if we move it to another issue or to Discord?

Also, a final review is appreciated.

@tresf tresf merged commit ea2ade2 into java-native:master Nov 10, 2023
13 checks passed
@tresf tresf deleted the actions branch November 10, 2023 17:40
@tresf tresf mentioned this pull request Nov 10, 2023
@pietrygamat pietrygamat added this to the 2.9.6 milestone Dec 1, 2023
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