-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
structured_sources: Allow build_tgt to be added as source #13351
base: master
Are you sure you want to change the base?
Conversation
Can you please extend a test? |
7ca8e71
to
501ec8f
Compare
@tristan957 added seems like this needs at least java 9. Is it enough to bump the dependency('jni') or do I need to add another mechanism the skip the builds for java 1.8? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For skipping on < Java 9, do this:
jni_dep = dependency('jni')
if jni_dep.version().version_compare('< 1.9')
error('MESON_SKIP_TEST: this test requires Java 9')
endif
You may also want to take a look at the CI images to make sure they aren't set to install Java 8. You'll also want to skip the test if the dependency isn't found.
static { | ||
String fullname = System.mapLibraryName(libname); | ||
// this is somewhat hacky, but attempt to split off the extension | ||
int ext = fullname.indexOf("."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java doesn't have a stem function for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemingly doesn't. And even if it did, I'd consider a stem function is just as hacky. Ideally you could just access lib prefix and suffix from java, but you can't, so you have to gain access to the suffix for mkstemp
somehow.
No idea if indexOf or lastIndexOf is better, the first breaks if the library prefix or libname contains a .
, the second breaks if the extension were to contain a .
(dunno, imagine a system capable of loading .so.gz
or so)
I don't think you need a whole new test for this. I would extend this one: https://github.com/mesonbuild/meson/tree/master/test%20cases/java/9%20jni. What do you think? |
@dcbaker is there any other place that would be a good test in addition to this? Not sure if there is a more generic |
I've thought about it, but it doesn't really work well with how the test is structured in One could generate the native headers in |
If you see a good way to refactor the existing test, that would be fine. |
501ec8f
to
2e87b14
Compare
2e87b14
to
9e0fe01
Compare
We need to get the image jobs to pass. Not your fault. I'm working on fixing Fedora. |
If you rebase on master, you should pick up the Fedora CI fixes. I think msys2 will also start to pass. |
f95bbcf
to
d103a67
Compare
Status report:
|
ci/ciimage/arch/install.sh
Outdated
@@ -15,6 +15,7 @@ pkgs=( | |||
doxygen vulkan-validation-layers openssh mercurial gtk-sharp-2 qt5-tools | |||
libwmf cmake netcdf-fortran openmpi nasm gnustep-base gettext | |||
python-lxml hotdoc rust-bindgen qt6-base qt6-tools wayland wayland-protocols | |||
debugedit glib2 glib2-devel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disable debug support in AUR packages, instead. We are not e.g. testing that valgrind can perform valgrind on scalapack / wxwidgets-gtk2 so there's no reason to spend longer compiling and produce bigger docker containers.
ci/ciimage/opensuse/install.sh
Outdated
@@ -18,6 +18,7 @@ pkgs=( | |||
boost-devel libboost_date_time-devel libboost_filesystem-devel libboost_locale-devel libboost_system-devel | |||
libboost_test-devel libboost_log-devel libboost_regex-devel | |||
libboost_python3-devel libboost_regex-devel | |||
perl-TimeDate # drop this once obs#1187867 made it into factory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're not trying to update the opensuse image today, only the Arch one, I would suggest letting opensuse fix this bug and having meson pick that fix up next week on the weekly refresh job.
You're not going to fix the opensuse image anyway unless you get dub to work, but that's tracked by #12143
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for debugging the upstream opensuse issue and submitting a fix to them, though. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coincidentally im probably the one at fault, given that I submitted lcov 2.0 in the first place 🙃
I have added a PR to really fix the fedora test at #13436 |
In some cases, there is a need to use a built library as a "source" for another target. Specifically Java JAR might want to contain their own native JNI libraries in order to provide a single file "executable".
767ff81
to
ba76b63
Compare
libgcrypt 1.11.0 on Arch doesnt ship with libgcrypt-config anymore. It only provides libgcrypt.pc.
Parent introduced unit test (9 jni) that depends on at least java 9, thus causing the test to be skipped.
ba76b63
to
2a3d050
Compare
In some cases, there is a need to use a built library as a "source" for another target. Specifically Java JAR might want to contain their own native JNI libraries in order to provide a single file "executable".
I'm kind of surprised this works out of the box without any further changes 🙃