jetbrains: split IDEs into separate nix files & reimplement update scripts#475183
jetbrains: split IDEs into separate nix files & reimplement update scripts#475183MattSturgeon merged 3 commits intoNixOS:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
|
I've not looked too closely, but it seems like most of the Would making I'm also not convinced that refactor needs to be part of this PR. It'd be great if the restructure and update-script changes could be a zero-rebuild PR, either by postponing unrelated refactors or by doing them separately first if they truly are needed. |
69e9f3c to
bcec23b
Compare
|
Good point! I rebased and removed that commit for now, for reference it is backed up here: 9e82055 Edit: Rebuilds are now 0! |
bcec23b to
cf555d4
Compare
cf555d4 to
51e668f
Compare
MattSturgeon
left a comment
There was a problem hiding this comment.
I wasn't able to review the update scripts in detail. I trust they are mostly copied as-is from the old location(s).
| inherit | ||
| pname | ||
| extraLdPath | ||
| jdk | ||
| src | ||
| version | ||
| buildNumber | ||
| wmClass | ||
| product | ||
| productShort | ||
| libdbm | ||
| fsnotifier | ||
| ; |
There was a problem hiding this comment.
Off-topic refactoring suggestion:
Instead of explicitly inheriting everything, use the @args and removeAttrs args excludeDrvArgNames // pattern. This will work well when migrating to lib.extendMkDerivation.
This will:
- preserve attr position information (
inheritcounts as defining a new attribute,//counts as re-using the original attribute) - reframe the args from "explicit inclusion" to "explicit exclusion", which usually makes more sense; most things should be passed through implicitly to mkDerivation. The
excludeDrvArgNameslist highlights any that are not.
(same applies in the base builders)
There was a problem hiding this comment.
Thanks! I'll keep this and your other comments (notably this one) in mind and open a follow-up PR to optimize this further, including switching to lib.extendMkDerivation!
| # Common build overrides, fixes, etc. | ||
| # TODO: These should eventually be moved outside of this file | ||
| pyCharmCommonOverrides = ( |
There was a problem hiding this comment.
How about a pycharm-overrides.nix file with a:
TODO: integrate into
pycharm.nixoncepycharm-community.nixis dropped?
Or will it also apply to pycharm-oss.nix even then?
Either way, I agree it makes more sense for the pycharm package files to import this themselves from a shared location, rather than have it passed in via package args.
An alternative would be for pycharm to define passthru.common-overrides. The others can then access it as jetbrains.pycharm.common-overrides. Pros: you don't need a loose file, cons: you need to add an internal passthru attr.
There was a problem hiding this comment.
Sounds good! If it's okay I would still like to do this in a follow-up PR with the lib.extendMkDerivation switch.
| # See https://www.jetbrains.com/help/pycharm/2022.1/cython-speedups.html | ||
| } | ||
| ); | ||
| patchSharedLibs = lib.optionalString stdenv.hostPlatform.isLinux '' |
There was a problem hiding this comment.
Feels like this could be packaged as a hook, jetbrains.patchSharedLibsHook. That way packages that use it could access it via callPackage args and its definition could be in a dedicated file.
Out of scope for this PR, I think.
| meta = { | ||
| homepage = "https://www.jetbrains.com/pycharm/"; | ||
| description = "Free Python IDE from JetBrains (built from source)"; | ||
| longDescription = "Python IDE with complete set of tools for productive development with Python programming language. In addition, the IDE provides high-class capabilities for professional Web development with Django framework and Google App Engine. It has powerful coding assistance, navigation, a lot of refactoring features, tight integration with various Version Control Systems, Unit testing, powerful all-singing all-dancing Debugger and entire customization. PyCharm is developer driven IDE. It was developed with the aim of providing you almost everything you need for your comfortable and productive development!"; |
There was a problem hiding this comment.
Now we are defining the longDescription manually in nix, it should follow standard one sentence per line formatting (using an indented string '').
Aside: was the longDescription auto-generated by the old update-script? If so, maybe that could be a TODO item for the new script.
There was a problem hiding this comment.
No it wasn't, it was created manually. Those could all use some clean-up in general.
However with multi-line strings this changes whitespaces and causes rebuilds, is it okay I do this in a follow-up PR, potentially together with the lib.extendMkDerivation related refactoring?
There was a problem hiding this comment.
Nothing in meta should cause rebuilds, with a few specific exceptions like meta.mainProgram when using versionCheckHook.
There was a problem hiding this comment.
Unfortunately right now it does. I'll investigate why.
Edit: I'm pretty sure it's the desktop file.
There was a problem hiding this comment.
Ah, that makes sense.
IIUC, using meta.* attrs to construct desktop files (or other build outputs) is considered a bit of an anti-pattern.
It's not really this PRs job to clean that up, but we could:
-
Add a NOTE to each
metablock (kinda repetitive)NOTE: meta attrs are used by the desktop entry, so changing them may cause rebuilds.
-
Fix the formatting and still avoid rebuilds by using
lib.replaceString "\n" " "(and maybelib.trim).
Ideally, this'd be a precursor commit "normalise newlines in desktop entry description", then the "split into separate files" commit comes after with correct formatting from the start.
I appreciate if you don't want to do this and would rather leave a FIXME, but it seems like such a small change.
There was a problem hiding this comment.
I was thinking about this a bit, but I think it's better to do this afterwards. Even if this PR doesn't end up getting merged, I'd still try and keep this PR as minimal as possible with how much it already changes, so that we can focus on improvements separately.
Funnily enough, the lib.replaceString was already there: comment = lib.replaceStrings [ "\n" ] [ " " ] meta.longDescription;. Probably would need some extra trimming, but I haven't tried it.
I added a NOTE to each meta block for now and a FIXME to the linux.nix and README.
There was a problem hiding this comment.
Yeah, addressing this separately is fine.
If it already has a replaceStrings the it's probably a trailing newline from the end of the indented string causing the rebuild, so a trim should solve this.
pkgs/applications/editors/jetbrains/updater/jetbrains_nix_updater/update_src_maven.py
Outdated
Show resolved
Hide resolved
pkgs/applications/editors/jetbrains/updater/jetbrains_nix_updater/util.py
Outdated
Show resolved
Hide resolved
This splits each IDE into its own `.nix` file and refactors all related build machinery to support this. Motivations (in order of priority): - Fixing reviews for update PRs: Currently update PRs get no maintainers assigned, since the `versions.json` was changed. Due to the way CI works, packages outside of `by-name` only have their maintainers added to PRs if the file that contains the derivation (`meta.position`) is changed. - Reducing the size of `default.nix` and making the differences & patches required for each IDE more clear and with that also actually cutting the code into separately maintainable chunks - Overall improving readability and code structure This commit was half-automatically generated by a script I wrote: https://gist.github.com/theCapypara/2939ff9f83a5f30f1d3dff74d725a9bf **Note that this commit itself does not cause any rebuilds. This is important. It is just a refactoring without any functional changes.**
51e668f to
c53edcc
Compare
This adds `passthru.updateScript` to the IDEs for automatic updates and rewrites all existing update scripts into one Python CLI app. This app is re-using most of the existing scripts in some way: - Most of `bin/update_bin.py` has been moved to `updater/jetbrains_nix_updater/fetcher.py` - Most of `source/update.py` has been moved to `updater/jetbrains_nix_updater/update_src.py` - `source/build_maven.py` has been moved almost as-is to `updater/jetbrains_nix_updater/update_src_maven.py` Since we are now no longer using JSON files but Nix files directly, I opted to use a simple "marker" approach for replacing things like versions and URLs in the files. I didn't want to implement complicated regex patterns like the `update-source-version` script does, it seemed overkill and I wanted to keep things as simple as possible. Co-Authored-By: Matt Sturgeon <matt@sturgeon.me.uk>
c53edcc to
55ca18e
Compare
|
I added some TODOs to the README that came from review for this PR. |
e289a35 to
bd6f98b
Compare
MattSturgeon
left a comment
There was a problem hiding this comment.
This is a clear structural improvement.
I'm very confident in the nix changes, especially due to zero rebuilds. I'm less confident reviewing the update scripts, but @theCapypara is probably one of the most familiar with the jetbrains update scripts, so I trust his judgement.
I'd ideally like to have ACK from some jetbrains maintainers, but if no one has any immediate feedback I plan to merge soon to unblock any planned followup work.
| # This actually just ignores a lot of options passed to it... (e.g. buildInputs) | ||
| # - not entirely sure how this hasn't caused big problems yet. |
There was a problem hiding this comment.
I agree, this seems entirely broken. Maybe no one is using the Nixpkgs jetbrains packages on darwin?
Maybe the binary packages get away with missing buildInputs because the dependencies happen to be available impurely on the system?
I'm not really familiar with darwin so I'm just guessing.
Either way, using the }@args: mkDerivation (removeAttrs args excludeAttrs // { pattern,1 or switching to extendMkDerivation (which does that under the hood), will resolve this.
Footnotes
| }; | ||
| aarch64-darwin = { | ||
| url = "https://download.jetbrains.com/aqua/aqua-2024.3.2-aarch64.dmg"; | ||
| sha256 = "43974cdbbb71aaf5bfcfaf2cfd0e69e9920dda3973e64671936c1d52b267494d"; |
There was a problem hiding this comment.
Other files are using SRI hashes, but this still has base64 encoding. Did it get missed when tweaking the update script?
EDIT: ah, I see all these instances are in deprecated packages.
There was a problem hiding this comment.
I missed this, since I just updated the other packages to test my changes at the same time. I guess it doesn't really matter since we'll remove these anyway.
| }; | ||
| aarch64-darwin = { | ||
| url = "https://download.jetbrains.com/writerside/writerside-243.22562.371-aarch64.dmg"; | ||
| sha256 = "9d86ef50b4c6d2a07d236219e9b05c0557241fb017d52ac395719bdb425130f5"; |
| }; | ||
| aarch64-darwin = { | ||
| url = "https://download.jetbrains.com/idea/ideaIC-2025.2.5-aarch64.dmg"; | ||
| sha256 = "52065492d433f0ea9df4debd5f0683154ab4dab5846394cabc8a49903d70e5bc"; |
There was a problem hiding this comment.
Same; maybe this is intentional as a deprecated package no longer being updated?
| }; | ||
| aarch64-darwin = { | ||
| url = "https://download.jetbrains.com/python/pycharm-community-2025.2.5-aarch64.dmg"; | ||
| sha256 = "040a4ed6bb7563972d844c450f615d0d11385e524fbbfdbfc9fc68d78811e994"; |
There was a problem hiding this comment.
Same; also deprecated though
Merging |
This PR majorly changes how Jetbrains IDEs are organized, maintained and updated. To explain my rationale behind this, I'll go through the changes commit-by-commit. It is not possible to split this into multiple PRs (at least not without making IDEs un-updatable in the meantime).
This PRcontains no actual changes to the IDEs or even the core mechanics of the build scripts. It refactors almost all of it, without changing the actual contents of the built packages or the inner logic of the old updater scripts. So there are no rebuilds.
I also added the comments below to the commit descriptions.
jetbrains: split IDEs into separate nix filesThis splits each IDE into its own
.nixfile and refactors all related build machinery to support this.Motivations (in order of priority):
versions.jsonwas changed. Due to the way CI works, packages outside ofby-nameonly have their maintainers added to PRs if the file that contains the derivation (meta.position) is changed.default.nixand making the differences & patches required for each IDE more clear and with that also actually cutting the code into separately maintainable chunksThis commit was half-automatically generated by a script I wrote: https://gist.github.com/theCapypara/2939ff9f83a5f30f1d3dff74d725a9bf
Note that this commit itself does not cause any rebuilds. This is important. It is just a refactoring without any functional changes.
jetbrains: reduce some usage of overrideAttrs
ℹ️ Note: I removed this commit, see comments, it's here for reference: 9e82055
This further improves the previous commit to remove some usage of
overrideAttrsby just addingpost*andpre*attributes tomkJetBrainsProduct. This causes some rebuilds without any actual changes aside from store paths.This is the only commit that should cause some rebuilds (only CLion, Goland, Rider, Rust Rover)
jetbrains: reimplement update scripts
This adds
passthru.updateScriptto the IDEs for automatic updates and rewrites all existing update scripts into one Python CLI app. This app is re-using most of the existing scripts in some way:bin/update_bin.pyhas been moved toupdater/jetbrains_nix_updater/fetcher.pysource/update.pyhas been moved toupdater/jetbrains_nix_updater/update_src.pysource/build_maven.pyhas been moved almost as-is toupdater/jetbrains_nix_updater/update_src_maven.pySince we are now no longer using JSON files but Nix files directly, I opted to use a simple "marker" approach for replacing things like versions and URLs in the files. I didn't want to implement complicated regex patterns like the
update-source-versionscript does, it seemed overkill and I wanted to keep things as simple as possible.Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.