Closed tresf closed 1 year ago
@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:
target/classes
target/cmake/natives
and src/main/resources-precompiled
(too late!)src/test/resources
(nothing todo, we don't have any)/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:
test/resources
, not just main/resources-precompiled
jssc.boot.library.path
to path that we know has the correct version.First option seems cleanest and easiest - see: https://github.com/pietrygamat/jssc/commit/ca26e1a0e3390fed7e7788c57b3df8c74b724e97 https://github.com/pietrygamat/jssc/actions/runs/6818119903/job/18543034333
Whoa, great find. Do you have permission to push to my branch?
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.
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... 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.
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.
yet another copy to keep track of.
Yeah, this is the confusing part for me. WHY do we even have the first copy in https://github.com/java-native/jssc/blob/0d106b74c04b75c2b8120aa08b78e8fa4f849590/CMakeLists.txt#L196 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.
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.
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
.
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.
Adds dockerless CI jobs to compile and test for new PRs; removes Travis-CI which stopped running a while ago.
TODO:
DiscussSerialNativeInterfaceTest.reportsWriteErrorsAsIOException
is failing in CISince #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 callwrite(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. Conversation deferred for another time.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.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: