Creating an entire Python package (and package itself) feels way overkill for an update script, which seems (hard to tell from my phone) to be contained in a single Python file. Unless there is library usage I'm not seeing, it would be preferable to condense that down to a single script file if possible.

(Which for the record, I am definitely for using Python for the update script. An entire Python package just feels a bit overkill.)

Will make a more proper review when I'm both on my laptop and feeling better

It made the development easier (since it does have external dependencies, mostly the GitHub library) and means that the linting and type‐checking can be enforced. This caught a bunch of issues while I was working on it. Since it’s used in both OpenJDK and OpenJFX, the latter of which is now in pkgs/by-name and therefore cannot directly access files outside of its own directory, it has to be its own package anyway.

Note that all the actual code is in one file. (I admit that pkgs/by-name/ni/nixpkgs-openjdk-updater/nixpkgs-openjdk-updater/src/nixpkgs_openjdk_updater/__init__.py is an absurd filename, but it matches the conventions for both Nixpkgs and tooling for Python development and packaging, so I think it’s better to be absurd than to try and contort it to go against those.)

I'm pretty sure it's relatively straight forward to make pyproject work in a flat directory, which would be better imo. I'd rather be slightly off of Python standards than have a tiny ancillary package with an absurd amount of boilerplate structure.

Don't know how that's done off of the top of my head, but that's more to do with me not being able to think straight right now than anything else lol. Update script structure isn't a blocker by any means, and it can be cleaned up later.

The way it is set up now allows doing e.g. uv run nixpkgs-openjdk-updater … to test it and using a standard buildPythonApplication. Given the existing by-name constraints requiring a full‐blown package, the only real boilerplate is a couple nested directories. I would prefer to stick with what Python tooling does rather than making sacrifices on the development and testing front in the absence of a strongly compelling reason to the contrary.

A bigger problem is that nixpkgs-vet isn’t happy with my interpolated JSON path. I might have to move things back to one big JSON file and eat the annoying merge conflicts that might result from updates.

Looks like I also have to figure out what’s causing the rebuild.

Okay, hopefully fixed both of those. Naturally it was of course an error in the OpenJDK 8 derivation that I accidentally fixed.

Still showing a rebuild? Will look into it tomorrow.

@ofborg eval

Good idea, I’ve applied the comments. I was working out a clearer approach with less code duplication based on a fold, but then I realized that it’s not clear that the current behaviour is correct in all cases anyway, e.g. if you combine a commit update script that explicitly enumerates the changed files with a silent update script that expects the updater to infer the additional files it changes. Update scripts are a bit of a badly‐composing mess (see e.g. my inability to just reuse finalAttrs.mitmCache.updateScript in OpenJFX), I’m thinking about trying to rationalize the system for 25.05… (But thank you for adding these neat combinators!)

Base branch eval error was fixed.

@ofborg eval

Well, the rebuilds are not benign: they are the canary in the coal mine telling us that overriding openjfx is broken. Packages in the tree rely on that. That’s why I need to choose between the potential fixes I listed.

Well, the rebuilds are not benign: they are the canary in the coal mine telling us that overriding openjfx is broken. Packages in the tree rely on that. That’s why I need to choose between the potential fixes I listed.

Between the 2 options, I think the "Or just fix the few in‐tree users and accept a minor breaking change to the interface" is the most reasonable one.

I’ve renamed the parameter to openjfx_jdk, i.e. “the version of OpenJFX to use with this JDK”, following the naming convention for multi‐version packages and preferring jdk over openjdk like jdk-bootstrap in the vague hopes that there could be uniformity between different possible JDKs in the interface here. I don’t really like that parameter name at all, but I didn’t get enough sleep to think of a better one, the deadline for breaking changes is in about 10 hours, and it’s likely there will be other breaking changes in 25.05 as I do a larger clean‐up, so I think bikeshedding can wait until then unless anyone really hates the name.

This should be ready to merge once ofborg says no rebuilds.

One rebuild for the updater package 🎉 🎉 🎉

This is ready now.

Thanks for the great work again @emilazy.

Thanks for working on this, but I don't see the advantage of rewriting the update script.

What's the painful part of running Java in the update script? The main reason for using Java was to avoid re-implementing the version parse logic. (#306616 (comment)). Also, it looks like the Python version doesn't work as expected: https://nixpkgs-update-logs.nix-community.org/jdk23/2024-10-31.log
It updates the version to refs/tags/jdk-23.0.1+11 (BTW, why not jdk-23.0.1-ga, I find the -ga suffix more user friendly. In my opinion, the build version number should only be used for non-GA versions)

refs/tags/jdk-23.0.1+11 is a correct version to update to, what’s the problem? We deliberately pick the tags corresponding to GA releases but without the -ga suffix so that we have the build number information available. That will allow us to give proper full java -evrsion output matching other Java builds like Temurin and Zulu, which identify their builds with the full version including build number and pass that build number information down to the OpenJDK build process.

The problem with the Java dependency is that having to build an OpenJDK version to update that same OpenJDK version causes problems when adding new versions and causes huge builds when doing it on top of staging. (It also made it basically impossible for me to run on macOS, where our OpenJDK packaging still needs work.) The logic needed changing to include the build number and to handle OpenJFX repositories and I was more comfortable doing it in Python than Java, and @Infinidoge, the other maintainer who has been working on cleaning up our OpenJDK packaging, agreed that it was preferable to have it in that language. Nixpkgs maintainers that aren’t familiar with a given language ecosystem will sometimes have to use and modify these scripts, and Python is the most common language after Bash for tooling tasks like these in the tree. Since the version format is documented as a simple regular expression by OpenJDK, and the modifications to handle the OpenJFX repositories meant that it needed modifying anyway, I don’t think the duplication is a problem. I am not even sure java.lang.Runtime.Version handles the JDK 8 version format, though I admittedly did not check.

the other maintainer who has been working on cleaning up our OpenJDK packaging, agreed that it was preferable to have it in that language
It also made it basically impossible for me to run on macOS

Fair enough, I didn't consider macOS (I don't have a mac, but it'd be nice to improve the macOS build)

refs/tags/jdk-23.0.1+11 is a correct version to update to, what’s the problem?

In that case, something else is going on, or I'm missing something. The update script was executed, but I don't see a PR to update jdk23 to 23.0.1

Yeah, I’d like to get our OpenJDK and OpenJFX packages building on Darwin so that we can use a source‐built JDK on all platforms and probably drop Zulu as redundant.

I already did the update in #351025, which might be it. It’s possible there’s something else going on too, though, since r-ryantm can be a bit finnicky. If we don’t get the next batch of updates automatically I’ll look into it.