Skip to content

jetbrains: switch builder to use extendMkDerivation pattern #478575

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.

Sign up for GitHub

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

Merged
MattSturgeon merged 2 commits into from
Feb 4, 2026
Merged

jetbrains: switch builder to use extendMkDerivation pattern #478575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f55d624
jetbrains: switch builder to use `extendMkDerivation` pattern
theCapypara Jan 9, 2026
a6276de
jetbrains: refactor some builder attributes
theCapypara Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 47 additions & 46 deletions pkgs/applications/editors/jetbrains/builder/darwin.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,57 +1,58 @@
# Darwin-specific base builder.
# TODO:
# 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.

{
lib,
stdenvNoCC,
undmg,
...
}:

{
meta,
pname,
product,
productShort,
src,
version,
passthru,

plugins ? [ ],
excludeDrvArgNames,
...
}:

let
loname = lib.toLower productShort;
in
stdenvNoCC.mkDerivation {
inherit
pname
src
version
plugins
passthru
;
meta = meta // {
mainProgram = loname;
};
desktopName = product;
dontFixup = true;
installPhase = ''
runHook preInstall
APP_DIR="$out/Applications/${product}.app"
mkdir -p "$APP_DIR"
cp -Tr *.app "$APP_DIR"
mkdir -p "$out/bin"
cat << EOF > "$out/bin/${loname}"
#!${stdenvNoCC.shell}
open -na '$APP_DIR' --args "\$@"
EOF
chmod +x "$out/bin/${loname}"
runHook postInstall
'';
nativeBuildInputs = [ undmg ];
sourceRoot = ".";
lib.extendMkDerivation {
inherit excludeDrvArgNames;

constructDrv = stdenvNoCC.mkDerivation;

extendDrvArgs =
finalAttrs:
{
product,
productShort ? product,

nativeBuildInputs ? [ ],
meta ? { },
...
}:

let
loname = lib.toLower productShort;
in
{
desktopName = product;

dontFixup = true;

installPhase = ''
runHook preInstall
APP_DIR="$out/Applications/${product}.app"
mkdir -p "$APP_DIR"
cp -Tr *.app "$APP_DIR"
mkdir -p "$out/bin"
cat << EOF > "$out/bin/${loname}"
#!${stdenvNoCC.shell}
open -na '$APP_DIR' --args "\$@"
EOF
chmod +x "$out/bin/${loname}"
runHook postInstall
'';

nativeBuildInputs = nativeBuildInputs ++ [ undmg ];

sourceRoot = ".";

meta = meta // {
mainProgram = loname;
};
};
}
108 changes: 45 additions & 63 deletions pkgs/applications/editors/jetbrains/builder/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,82 +6,64 @@
callPackage,

jdk,
fontconfig,
libGL,
libX11,

vmopts ? null,
forceWayland ? false,
}:
let
baseBuilder = if stdenv.hostPlatform.isDarwin then ./darwin.nix else ./linux.nix;
mkJetBrainsProductCore = callPackage baseBuilder { inherit vmopts; };
in
# Makes a JetBrains IDE
{
pname,
src,
version,
buildNumber,
wmClass,
product,
productShort ? product,
meta,

libdbm,
fsnotifier,
lib.extendMkDerivation {
constructDrv = callPackage baseBuilder {
inherit vmopts jdk forceWayland;
Copy link
Contributor

@MattSturgeon MattSturgeon Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these feel like things that should be derivationArgs, rather than callPackage/override args. That way they become extensible via overrideAttrs and accessible via finalAttrs.

That refactor will (of course) cause rebuilds, and may be tangential to this PR anyway.

Copy link
Member Author

@theCapypara theCapypara Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly didn't want to break any external uses of these, but I also thought it might make more sense to have these as derivationArgs.

I would suggest to rethink these three in a follow-up PR. I also really don't like how jdk is just an internal alias basically, I kinda want to get rid of that.

# Args to not pass to mkDerivation in the base builders. Since both get the same args
# passed in, both have the same list of args to ignore, even if they don't both use
# all of them.
excludeDrvArgNames = [
"product"
"productShort"
"buildNumber"
"wmClass"
"libdbm"
"fsnotifier"
"extraLdPath"
"extraWrapperArgs"
];
Comment on lines +20 to +32
Copy link
Contributor

@MattSturgeon MattSturgeon Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. This intuitively feels a bit ugly, but I get why you've taken this approach... I currently don't have a better suggestion.

If the goal is being DRY, you either do this or have both implementations import ./exclude-drv-arg-names.nix. If the goal is to be explicit about which args each implementation expects, then calling code would need to apply args conditionally.

Maybe related:

This also makes me question: does the current structure (common builder -> calls platform base builder) make more sense than the alternative (platform builder -> calls common base builder)... What do you think?

Copy link
Member Author

@theCapypara theCapypara Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regading your quote: I think which order we do this in kinda makes no difference, since we'll end up with the Darwin builder getting most of these attrs, even though it doesn't use them, regardless, unless we would specify separate drvs for Linux and Darwin for all IDEs, which doesn't make any sense really.

I'll think about this a bit, because I also kinda don't like that there isn't one place that documents the arguments. I guess having the excludeDrvArgNames here in this file at least allows seeing all of them in one file and we could even add documentation comments for them here as well.

I couldn't really find any example of how this is solved elsewhere, the pattern of needing to have a completely different base drv for Darwin and Linux seems to be somewhat unique.

We could maybe put all the Linux-specific attributes into a sub-attrset linux but this seems very confusing and may cause issues in the future, if we need some of them for the Darwin builder as well.

};

extraWrapperArgs ? [ ],
extraLdPath ? [ ],
buildInputs ? [ ],
passthru ? { },
}:
mkJetBrainsProductCore {
inherit
pname
extraLdPath
jdk
src
version
buildNumber
wmClass
product
productShort
libdbm
fsnotifier
;
extendDrvArgs =
Copy link
Contributor

@MattSturgeon MattSturgeon Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless some of these are intended to be passed to mkDerivation, they should probably be listed here in this function's excludeDrvArgNames.

Otherwise, if they are used by the baseBuilder, they probably shouldn't be declared args here; it's usually cleaner if only one layer handles a specific arg.

This also makes me question: does the current structure (common builder -> calls platform base builder) make more sense than the alternative (platform builder -> calls common base builder)... What do you think?

Copy link
Member Author

@theCapypara theCapypara Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all of them that are listed here are also used here? Or did I miss one?

Copy link
Contributor

@MattSturgeon MattSturgeon Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I misread the diff, but I couldn't see an excludeDrvArgNames in this file.

Copy link
Member Author

@theCapypara theCapypara Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay, I misunderstood you. But if I add them here, won't they also not be passed to the Linux and Darwin builders?

Copy link
Contributor

@MattSturgeon MattSturgeon Jan 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In part, that's kinda my point. Are these args handled here, by the base builder, or both?

IMO, whichever function is using the args should be the one that a) declares the arg and b) decides whether to propagate or exclude it from the call to its underlying builder.

Copy link
Member Author

@theCapypara theCapypara Jan 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wmClass was still in the args by accident, all others are used here. All in the excludeDrvArgNames passed to the base builder are used by the linux base builder.

# NOTE: See linux.nix and darwin.nix for additional specific arguments
finalAttrs:
{
buildNumber,
product,

buildInputs =
buildInputs
++ [ stdenv.cc.cc ]
++ lib.optionals stdenv.hostPlatform.isLinux [
fontconfig
libGL
libX11
];
libdbm,
fsnotifier,

extraWrapperArgs =
extraWrapperArgs
++ lib.optionals (stdenv.hostPlatform.isLinux && forceWayland) [
''--add-flags "\''${WAYLAND_DISPLAY:+-Dawt.toolkit.name=WLToolkit}"''
];
meta ? { },
passthru ? { },
...
}:
{
passthru = passthru // {
inherit
buildNumber
product
libdbm
fsnotifier
;
Comment on lines +51 to +56
Copy link
Contributor

@MattSturgeon MattSturgeon Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this layer, these arguments are only used to populate passthru. I understand the motivation to avoid duplicating the passthru wiring in both linux.nix and darwin.nix, but it does mean this wrapper needs to be aware of arguments it otherwise doesn't consume. That's a bit of an abstraction leak: ideally each layer would fully own the arguments it accepts.

That said, this isn't something introduced by this change. I'm happy to defer that to a follow-up.

Stepping back, builder/default.nix itself is fairly thin, and it's worth reconsidering whether this level of indirection is buying us much at all.


passthru = lib.recursiveUpdate passthru {
inherit
buildNumber
product
libdbm
fsnotifier
;
updateScript = ../updater/main.py;

updateScript = ../updater/main.py;
tests = {
plugins = callPackage ../plugins/tests.nix { ide = finalAttrs.finalPackage; };
};
};

tests = {
plugins = callPackage ../plugins/tests.nix { ideName = pname; };
meta = meta // {
teams = [ lib.teams.jetbrains ];
};
};
};

meta = meta // {
teams = [ lib.teams.jetbrains ];
};
}
Loading
Loading