jetbrains: switch builder to use extendMkDerivation pattern#478575
jetbrains: switch builder to use extendMkDerivation pattern#478575MattSturgeon merged 2 commits intoNixOS:masterfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
MattSturgeon
left a comment
There was a problem hiding this comment.
Looks like a much cleaner implementation overall!
I've scrolled through the diff a few times and left a mixture of design questions/comments and impl nitpicks. I'll stop scrolling now, to avoid overwhelming you with too much review spam. 🙈
| meta = meta // { | ||
| teams = [ lib.teams.jetbrains ]; | ||
| }; | ||
| meta = (lib.optionalAttrs (args ? meta) args.meta) // { |
There was a problem hiding this comment.
Same feedback(s) as the other meta
| # 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" | ||
| "wmClass" | ||
| "libdbm" | ||
| "fsnotifier" | ||
| "extraLdPath" | ||
| "extraWrapperArgs" | ||
| ]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| mkPyCharmProduct, | ||
| mkJetBrainsSource, | ||
| pyCharmCommonOverrides, | ||
| ... |
There was a problem hiding this comment.
These are callPackage args, right? Why the ...?
There was a problem hiding this comment.
It doesn't use mkJetBrainsProduct anymore and I didn't want to introduce an extra function just for PyCharm in the default.nix, so it also gets mkPyCharmProduct passed. Happy for any suggestions to make this cleaner!
There was a problem hiding this comment.
I'll probably need to clone this locally to fully understand, but my instinct is that unnecessary ... is a code smell. How come mkJetBrainsProduct and mkPyCharmProduct get supplied unconditionally?
| fsnotifier, | ||
| pyCharmCommonOverrides, | ||
| musl, | ||
| ... |
dfb133f to
8347f2b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
The fact that there are now more rebuilds than before really confuses me. I can't really figure out why? I guess maybe because of the default attrs and how they are merged? Or some interactions with the overrides in the IDEs? But then why is |
|
@ofborg build jetbrains.idea-oss.tests.plugins.stored-correctly |
|
Testing locally, I was able to avoid rebuilds for diff --git a/pkgs/applications/editors/jetbrains/builder/default.nix b/pkgs/applications/editors/jetbrains/builder/default.nix
index c0bf69d462c2..b710bb9cc0d9 100644
--- a/pkgs/applications/editors/jetbrains/builder/default.nix
+++ b/pkgs/applications/editors/jetbrains/builder/default.nix
@@ -23,6 +23,7 @@ lib.extendMkDerivation {
excludeDrvArgNames = [
"product"
"productShort"
+ "buildNumber"
"wmClass"
"libdbm"
"fsnotifier"
diff --git a/pkgs/applications/editors/jetbrains/builder/linux.nix b/pkgs/applications/editors/jetbrains/builder/linux.nix
index 76a08a4220a9..5194df00de23 100644
--- a/pkgs/applications/editors/jetbrains/builder/linux.nix
+++ b/pkgs/applications/editors/jetbrains/builder/linux.nix
@@ -81,21 +81,19 @@ lib.extendMkDerivation {
{
inherit desktopItem vmoptsIDE vmoptsFile;
- buildInputs = [
+ buildInputs = buildInputs ++ [
stdenv.cc.cc
fontconfig
libGL
libX11
- ]
- ++ buildInputs;
+ ];
- nativeBuildInputs = [
+ nativeBuildInputs = nativeBuildInputs ++ [
makeWrapper
patchelf
unzip
autoPatchelfHook
- ]
- ++ nativeBuildInputs;
+ ];
postPatch = ''
rm -rf jbr
Maybe this is enough for all IDEs? I'm not suggesting that avoiding all rebuilds should be the end goal, but it does make the initial switch easier to review 🙂 To fully take advantage of fixpoint-args args, making them more accessible from |
|
Thanks! I will test later, this may get rid of all rebuilds. If it does, I'll add additional commits to reduce the list of excluded attrs again, we'll have to see what makes sense there. This will cause rebuilds again, but we'll know why. Everything else would then go in follow-up PRs. |
8347f2b to
25cff2b
Compare
|
Alright, got rid of all rebuilds now except for So we can now talk about changing Edit: On Darwin there are more rebuilds: |
25cff2b to
b4f6a47
Compare
b4f6a47 to
02c853a
Compare
69f3bca to
4264290
Compare
|
I now made it so that the first commit does not cause any rebuilds* (see the fact that the labels changed to "rebuild-*: 0". I'll now push a second commit that removes the unused I also had to disable the *This first commit allegedly removes 6 packages, see the diff generated by CI: {"added":[],"changed":["release-checks"],"rebuilds":["release-checks"],"removed":["jetbrains.idea.aarch64-darwin","jetbrains.phpstorm.aarch64-darwin","jetbrains.pycharm.aarch64-darwin","jetbrains.rider.aarch64-darwin","jetbrains.ruby-mine.aarch64-darwin","jetbrains.webstorm.aarch64-darwin"]}It doesn't actually remove these. |
Odd. Testing locally, all of these are still present. Not sure why the eval comparison would find them missing. |
This commit does not cause any rebuilds.
69f3bca to
a6276de
Compare
MattSturgeon
left a comment
There was a problem hiding this comment.
Let's get this merged to unblock further refactoring. LGTM!
| inherit | ||
| buildNumber | ||
| product | ||
| libdbm | ||
| fsnotifier | ||
| ; |
There was a problem hiding this comment.
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.
|
For the record, I'm still not sure why the eval comparison thinks these packages have been removed; they are still there: Added packages (0)Removed packages (6)Changed packages (9)cc @NixOS/nixpkgs-ci |
|
@MattSturgeon Seems that the packages got removed on Darwin because (somehow) musl got added as a dependency. So CI was right. Edit: Yeah I somehow managed to miss the buildInputs of these packages, they need to be marked as Linux-only, since before they also effectively were (since the Darwin builder ignored them. Will fix later today.) |
Follow-up to #475183
I recommend disabling whitespaces for reviewing.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.