maptool: fix JS (extract application JARs from package) #262650
Conversation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/2803 |
rhendric
force-pushed
the
rhendric/maptool
branch
from
41bc97f
to
4679efa
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/2836 |
| makeWrapper ${jre}/bin/java "$dest"/${binName} \ | ||
| "''${gappsWrapperArgs[@]}" \ | ||
| --prefix LD_LIBRARY_PATH : ${lib.makeLibraryPath [ ffmpeg ]} \ | ||
| --add-flags "${lib.concatStringsSep " " jvmArgs} net.rptools.maptool.client.LaunchInstructions" | ||
|
|
||
| runHook postInstall | ||
| ''; |
There was a problem hiding this comment.
Please get the url via src.url
There was a problem hiding this comment.
I don't know what line this comment is meant to refer to, but please note that src.url points to a single release artifact, not the entire repository like it would if this used fetchFromGitHub or something.
| @@ -13,15 +13,10 @@ | |||
| let | |||
| pname = "maptool"; | |||
| version = "1.13.2"; | |||
| repoBase = "https://github.com/RPTools/${pname}"; | |||
| repoBase = "https://github.com/RPTools/maptool"; | |||
There was a problem hiding this comment.
| repoBase = "https://github.com/RPTools/maptool"; |
There was a problem hiding this comment.
Nope, this deduplicates the base repository URL between the source and the update script.
There was a problem hiding this comment.
You can get this url via src.url. We do not need to cripple the URL.
There was a problem hiding this comment.
Are you aware that the value of src.url is https://github.com/RPTools/maptool/releases/download/1.13.2/maptool-1.13.2-x86_64.pkg.tar.zst?
Are you proposing splitting on / and selecting the first n components, or stripping the last n? Because that seems way less transparent than defining a variable for the URL base. I don't see any problem with this approach.
There was a problem hiding this comment.
Sandro probably meant that the deduplication is unnecessary and that one should simply write url = "https://github.com/RPTools/maptool.git"; instead.
There was a problem hiding this comment.
I don't see how ‘You can get this url via src.url’ could possibly mean that, but... come on, is this a blocker? Does being the maintainer of this package grant me the leeway to take the oh-so-bold position that deduplication is good actually, because in the future I want it to be obvious if the source and the update script URLs drift apart?
Run a grep -rF 'url = "${' on Nixpkgs if you think I'm being some sort of maverick by using this pattern. It's not exactly uncommon. Criticisms of functionality are more than welcome, and violations of the Nixpkgs house coding style also make good feedback, but this is something else altogether and I beg one of you to just click the dang merge button on this months-old PR if this is the only nit left to pick.
There was a problem hiding this comment.
Definitely not a blocker for me. I'll give Sandro 24 hours to react before I merge it, so ping me next night if it's still not merged.
| install -dm755 "$dest" | ||
| makeWrapper ${jre}/bin/java "$dest"/${binName} \ | ||
| "''${gappsWrapperArgs[@]}" \ | ||
| --prefix LD_LIBRARY_PATH : ${lib.makeLibraryPath [ ffmpeg ]} \ |
There was a problem hiding this comment.
Is there an alternative to LD_LIBRARY_PATH?
There was a problem hiding this comment.
This package uses hundreds of pre-compiled JARs, many of which contain binary libraries. The only alternative I can think of would be unpacking those JARs, patchelf-ing each library, and then reassembling the JARs, and I really think using LD_LIBRARY_PATH is less brittle than that.
... instead of using the uber-JAR release. Packing the JARs together causes the JavaScript macro functionality not to work, as the scripting interface uses a ServiceLoader to find language providers via text file resources that share a common name. With one file per JAR, this works great. When multiple files with the same name are in one uber-JAR, all but one of them are shadowed, and the JavaScript language provider was getting lost.
rhendric
force-pushed
the
rhendric/maptool
branch
from
4679efa
to
d768bf6
Compare
rhendric
changed the title
| @@ -13,15 +13,10 @@ | |||
| let | |||
| pname = "maptool"; | |||
| version = "1.13.2"; | |||
| repoBase = "https://github.com/RPTools/${pname}"; | |||
| repoBase = "https://github.com/RPTools/maptool"; | |||
There was a problem hiding this comment.
You can get this url via src.url. We do not need to cripple the URL.
|
Successfully created backport PR for |
Extract application JARs from package instead of using the uber-JAR release.
Packing the JARs together causes the JavaScript macro functionality not to work, as the scripting interface uses a ServiceLoader to find language providers via text file resources that share a common name. With one file per JAR, this works great. When multiple files with the same name are in one uber-JAR, all but one of them are shadowed, and the JavaScript language provider was getting lost.
Description of changes
Resolves #246830.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)