-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
gradle: auto-detect native version and patch file-events #277721
Conversation
@lorenzleutgeb sorry to bother, but pinging in case you have time to take a peek. |
daec92d
to
f7c0dfd
Compare
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.
Builds and works on my machine. Thanks! I suggest to remove some redundancy (DRY, see comment) but could be merged as is.
popd | ||
done | ||
|
||
# The file-events library _seems_ to follow the native-platform version, but | ||
# we won’t assume that. | ||
fileEventsJar=$(basename $(ls -1 $out/lib/gradle/lib/file-events-*.jar | grep -Ev '.-(linux|osx|freebsd|windows)-')) |
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.
Maybe define a function to get the version like this? It seems to be the same code in lines 81-83 above.
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.
Added a helper and switched to using autoPatchelf, since it does exactly what we need here.
f7c0dfd
to
1bb4d47
Compare
1bb4d47
to
c326ee4
Compare
c326ee4
to
8e9523e
Compare
8e9523e
to
5cfb74b
Compare
Result of 3 packages marked as broken and skipped:
11 packages failed to build:
65 packages built:
|
SuccessesResults Failures
|
@ofborg build gscan2pdf bisq-desktop corretto11 corretto17 corretto19 ganttproject-bin moneydance |
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.
I had a look at the code and tried it out and it looks great 👍️
I also grepped through all jar
files in the latest gradle distribution (gradle-8.7-bin.zip
) and found some other native libraries - not sure if we should/must patch them as well:
libjansi.so
injansi-1.18.jar
andkotlin-compiler-embeddable-1.9.22.jar
liblz4-java.so
inkotlin-compiler-embeddable-1.9.22.jar
I haven't had any issues so far without it being patched 🤷
shift | ||
local candidates="$@" | ||
|
||
jar="$(basename $(ls -1 "$@" | grep -Ev '.-(linux|osx|freebsd|windows)-'))" |
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.
I think we can simply rely on sort
for this, no?
jar="$(basename $(ls -1 "$@" | grep -Ev '.-(linux|osx|freebsd|windows)-'))" | |
jar="$(basename -a $candidates | sort | head -n1)" |
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.
Applied.
popd | ||
autoPatchelfInJar \ | ||
$out/lib/gradle/lib/native-platform-linux-${arch}$variant-''${nativeVersion}.jar \ | ||
"${stdenv.cc.cc.lib}/lib64:${lib.makeLibraryPath [ stdenv.cc.cc ncurses5 ncurses6 ]}" |
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.
I don't know too much about elf patching - but this looks reasonable to me🤷
mv native-platform-linux-${arch}$variant-${nativeVersion}.jar $out/lib/gradle/lib/ | ||
popd | ||
autoPatchelfInJar \ | ||
$out/lib/gradle/lib/native-platform-linux-${arch}$variant-''${nativeVersion}.jar \ |
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.
Just for the record - this is "just" patching the linux version (as it was done before).
As others told me, the resulting nix package works on macOS too
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.
Yes, patchelf is a Linux-only thing, AFAIK.
As far as I can tell, |
5cfb74b
to
4a7a7ab
Compare
I'd like to get this in but am a little hesitant because of the build failures. Re-running nixpkgs-review at the moment, it's not done but
Edit: Results are in. I don't really understand what's the problem with Result of 8 packages marked as broken and skipped:
7 packages failed to build:
71 packages built:
|
4a7a7ab
to
9e7243c
Compare
Will need a rebase if still desired. |
Instead of having to know the version of native-platform at the Nix level, we use the file `lib/native-platform-<version>.jar` to figure out the version of the library JAR to patch.
It depends on libstdc++, so add that to its RPATH.
9e7243c
to
bd244e7
Compare
@adamcstephens rebased, thanks. |
Description of changes
Drop the requirement to specify the version of the native-platform library at Nix level. We use the non-platform-specific
native-platform-<version>.jar
to resolve the version of that library at fixup.Hoping that we might be able to use
nix-update-script
to automate updates, but have not tested that yet.I also noticed there is a bundled file-events library that requires libstdc++, so added a similar patch for that.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.