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

gradle: add setup hook #272380

Merged
merged 25 commits into from
Jul 14, 2024
Merged

gradle: add setup hook #272380

merged 25 commits into from
Jul 14, 2024

Conversation

chayleaf
Copy link
Contributor

@chayleaf chayleaf commented Dec 5, 2023

Description of changes

  1. This adds previously missing Gradle lockfiles... although the official lockfile format is very bad - so instead we MITM Gradle with a caching proxy. Trust me, I've tried - MITMing Gradle is the only solution that consistently works, because Java devs have no idea what lockfiles are supposed to do (for starters, Gradle lockfiles don't always lock dependencies).
  2. Said MITM caching proxy is added. Right now it's custom made for Gradle, but in theory it could be used for other software, to get rid of FODs. It offers basic configuration via regex.
  3. A downside is the fact those "lockfiles" (meaning a JSON record of all files fetched by Gradle) now have to be stored in nixpkgs. I think this is much better than FODs anyway, and I've added an update script to generate them.
  4. This PR also converts all packages that depend on Gradle to use the setup hook

In turn, this solves the following longstanding Gradle+Nix issues:

  • Package aliases like okio -> okio-jvm now work with no manual workarounds
  • SNAPSHOT dependencies don't have to be manually pinned - the "lockfile" takes care of it (because maven repo metadata is autogenerated based on the lockfiles' contents)
  • FODs aren't used anymore, letting Nix fetch the files. This should increase the level of deduplication
  • Many Gradle scripts download stuff from random sources - that can be included in the lockfile. That said, there's no logic to exclude URLs from the cache currently.
  • Using different dependencies (from different repos) with the same name and version number is now possible (yes, it is something Gradle actually does at times)
  • No code duplication is necessary anymore (currently there are long "deps" FODs for each Gradle app)

Open questions

It seems there are no major issues left?

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@liff
Copy link
Contributor

liff commented Dec 6, 2023

It would be great to get rid of # perl code mavenizes pathes indeed 😀

although the official lockfile format is very bad

Are there some improvements that could be suggested to the upstream, or is the format hopelessly bad?

As the lockfiles' size is quite significant, is there a way to make it noticeably smaller (i.e. 30%+)? Maybe a postprocessing script to put deps with a common prefix (e.g. from the same Gradle repo) to the same attrset?

Would this sort of change be simple enough to implement in the proxy directly? Change the type of Pages to

struct Pages(BTreeMap<String, BTreeMap<String, Page>>);

so that the first level is the URL scheme+authority:

{
 "https://jcenter.bintray.com": {
  "com/google/code/findbugs/jsr305/3.0.2/jsr305-3.0.2.jar": {
   "sha256": "dmrSoHg/JoeWLIrXTO7MOKKLn3Ki0IXuQ4t4E+ko0Mc="
  },
  "com/google/code/findbugs/jsr305/3.0.2/jsr305-3.0.2.pom": {
   "sha256": "GYidvfGyVLJgGl7mRbgUepdGRIgil2hMeYr+XWPXjf4="
  },
  "com/google/code/gson/gson-parent/2.9.1/gson-parent-2.9.1.pom": {
   "sha256": "fKCEXnNoVhjePka9NDTQOko3PVIPq5OmgDGK1sjLKnk="
  }
 }
}

In this tiny sample it saves about 8%, which is pretty far from the desired 30%+ 😦

@linsui
Copy link
Contributor

linsui commented Dec 6, 2023

Then maybe use the gradle coordination:

{
 "https://jcenter.bintray.com": {
  "com.google.code:findbugs.jsr305:3.0.2": {
   "jar": "dmrSoHg/JoeWLIrXTO7MOKKLn3Ki0IXuQ4t4E+ko0Mc=",
   "pom": "GYidvfGyVLJgGl7mRbgUepdGRIgil2hMeYr+XWPXjf4="
  }
 }
}

@lorenzleutgeb
Copy link
Member

Related to #151524

@chayleaf
Copy link
Contributor Author

chayleaf commented Dec 6, 2023

Are there some improvements that could be suggested to the upstream, or is the format hopelessly bad?

Here's my list of issues:

  1. it generates a lockfile tree, not a single lockfile. Each build.gradle produces 2 lockfiles (for the build script and for the project itself) and 2 "dependency verification" hash lists, and there may be multiple build.gradle files in a project. This means there can be lots of duplication
  2. it doesn't store which repo dependencies come from, which actually makes it useless, not only for us to fetch deps from, but for Gradle itself, because different parts of a project may use the same dependency version from different repos, and it may have a different hash (that's not theoretical, it happens in practice, and Gradle docs suggest manual workarounds involving editing the "dependency verification" lockfiles)
  3. It doesn't pin SNAPSHOT deps on purpose, pretty horrible huh?
  4. There's no native way to bundle a project's dependencies. What we're doing right now is a massive hack - and it shows, because many manual workarounds are needed, like okio->okio-jvm or kotlin-stdlib-common->kotlin-stdlib redirections. "The Gradle Way" is copying the entire GRADLE_USER_HOME... which isn't reproducible at all
  5. Since Gradle is Turing-complete, so are the dependency lists - and each separate configuration produces a different set of lockfiles. What constitutes as "separate" depends on the project, but it's common to have dependencies depend on the target platform if they contain native libs - well, that's on Java devs for relying on jars instead of building stuff from source.
  6. Maybe for the aforementioned reasons, maybe because it adds seemingly useless overhead, nobody actually uses lockfiles outside of locked down enterprise contexts
  7. Many projects (like Ghidra or some Signal-related project I don't remember the name of) have additional parts of the build script fetch stuff from the internet.

At this point, isn't it normal to give up on Gradle lockfiles? It seems like the entire ecosystem is built in a way as to be as non-reproducible as possible. From what little interaction I had with upstream, it doesn't seem like they'd be very willing to improve it either, but you're welcome to try; I'm not a Java dev, I'm just a nixpkgs contributor, I don't want to waste my effort on proposing changes to people who probably don't have the time to look into why they're necessary.

Good thing is the MITM cache, with some modifications (to add more configuration), can actually be used for other build systems that currently use FODs, so I think this way of doing it is both pragmatic and useful in the long run. Even if there isn't an http(s) proxy setting, we can hijack /etc/ssl (or whatever other paths) and use a proxifier, which should defeat even the most uncooperating build systems. As I already mentioned, this is far from ideal, but it's pragmatic.

@chayleaf
Copy link
Contributor Author

chayleaf commented Dec 8, 2023

Then maybe use the gradle coordination:

This gives an improvement of 46.123%, down to 100kb for this particular deps.json! It would be possible to squeeze another 0.3% if snapshots didn't exist, but whatever, that doesn't matter much.

@chayleaf chayleaf force-pushed the gradle2 branch 3 times, most recently from 263a26d to faacf14 Compare December 9, 2023 00:17
@chayleaf
Copy link
Contributor Author

chayleaf commented Dec 9, 2023

I think I got this in a shape suitable for most Gradle derivations currently in nixpkgs, should I try converting more derivations to the setup hook? I'm slightly familiar with them from my previous attempt at creating a builder for Gradle. If possible, I'd like to get a review of the public interface (what you see in the latest commit, the attr names and such) before continuing.

Also, should I undo some deps.json optimizations to have slightly smaller diffs (which would add up to 5% to deps.json size), or is raw size the priority? reverted some optimizations, but brought a little more improvements

@chayleaf chayleaf force-pushed the gradle2 branch 3 times, most recently from c6f85f4 to cd5e909 Compare December 10, 2023 00:58
@ofborg ofborg bot requested review from expipiplus1 and Ma27 December 10, 2023 06:53
@chayleaf chayleaf force-pushed the gradle2 branch 3 times, most recently from ae1da4f to 5fed667 Compare December 10, 2023 09:05
@GetPsyched
Copy link
Member

Not necessarily as part of this PR, but I wonder if there is a plan to have a buildGradlePackage wrapper at some point?

@chayleaf
Copy link
Contributor Author

chayleaf commented Jul 12, 2024

I don't see a point in that, look at e.g. the key package diff, it destroys all the annoying Gradle-specific overhead that was previously there, and mitmCache highlights that this is very much not an official Gradle lockfile but rather something hack-ish

@GetPsyched
Copy link
Member

Good point; not adding a wrapper exposes the hacky behaviour. Nevermind.

@chayleaf
Copy link
Contributor Author

Result of nixpkgs-review pr 272380 run on x86_64-linux 1

8 packages marked as broken and skipped:
  • corretto19
  • corretto19.debug
  • javaPackages.openjfx15
  • javaPackages.openjfx19
  • javaPackages.openjfx20
  • openjfx15
  • openjfx19
  • openjfx20
81 packages built:
  • apkleaks
  • apkleaks.dist
  • apksigcopier
  • apksigcopier.dist
  • apksigner
  • armitage
  • autopsy
  • bisq-desktop
  • bluej
  • cie-middleware-linux
  • corretto11
  • corretto11.debug
  • corretto17
  • corretto17.debug
  • corretto21
  • corretto21.debug
  • cryptomator
  • diffoscope
  • diffoscope.dist
  • diffoscope.man
  • experienced-pixel-dungeon
  • fastddsgen
  • fdroidserver
  • fdroidserver.dist
  • freenet
  • freeplane
  • ganttproject-bin
  • ghidra
  • ghidra-extensions.gnudisassembler
  • ghidra-extensions.machinelearning
  • ghidra-extensions.sleighdevtools
  • gradle (gradle_8)
  • gradle-unwrapped (gradle_8-unwrapped)
  • gradle_6
  • gradle_6-unwrapped
  • gradle_7
  • gradle_7-unwrapped
  • greenfoot
  • gscan2pdf
  • gscan2pdf.man
  • jabref
  • jadx
  • openjfx11 (javaPackages.openjfx11)
  • openjfx (javaPackages.openjfx17 ,openjfx17)
  • openjfx21 (javaPackages.openjfx21)
  • openjfx22 (javaPackages.openjfx22)
  • jd-gui
  • jextract
  • jextract-21
  • key
  • kotlin-language-server
  • maptool
  • mcaselector
  • microsoft-identity-broker
  • mindustry
  • mindustry-server
  • mindustry-wayland
  • mitm-cache
  • moneydance
  • mucommander
  • ns-usbloader
  • olvid
  • pattypan
  • pdf-sign
  • pdfchain
  • pdfsam-basic
  • pdftk
  • pidginPackages.purple-signald
  • quark-goldleaf
  • rat-king-adventure
  • rkpd2
  • scenebuilder
  • scenic-view
  • shattered-pixel-dungeon
  • shorter-pixel-dungeon
  • signald
  • signaturepdf
  • sparrow
  • sparrow-unwrapped
  • stirling-pdf
  • summoning-pixel-dungeon

@Atemu Atemu merged commit 476b450 into NixOS:master Jul 14, 2024
22 of 23 checks passed
@Atemu
Copy link
Member

Atemu commented Jul 14, 2024

Thanks a bunch for your work @chayleaf :)

@infinisil
Copy link
Member

For the pkgs/by-name check failure, this would've been an option (cc @roberth). Not a big deal though, the CI failure pointed out that it wouldn't break other PRs:

This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

@philiptaron
Copy link
Contributor

philiptaron commented Jul 22, 2024

This PR broke evaluation of nixpkgs for Nix 2.3.18.

syntax error, unexpected '.', at /nix/store/mbq17diwa38x96apmjp8maap2qf1yk01-source/pkgs/games/shattered-pixel-dungeon/generic.nix:56:54

I made a fix in PR #329212.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-buildproxy-reproducible-http-https-responder-in-sandboxed-nix-builds/40081/12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.