jdk{8,11,17,23,24}: always enable __structuredAttrs; cleanup by limwa · Pull Request #425773 · NixOS/nixpkgs

limwa · 2025-07-16T12:24:06Z

Re-enables __structuredAttrs in jdk8.

Because the getAllOutputNames function returns different results depending on the value of __structuredAttrs, and the LIBDIRS variable needs to be constructed in a given order, the build for jdk8 fails with __structuredAttrs = true. The outputs should be patched separately to avoid having a cycle between outputs.

As such, I've simplified the postFixup hook to use autoPatchelf individually on each output. Using autoPatchelf retains the old behavior and introduces other checks such as erroring when a library dependency cannot be satisfied.

Relevant issue: #425323 (comment)
Follow up to: #425561

Things done

Add a 👍 reaction to pull requests you find important.

limwa · 2025-07-16T12:29:07Z

Things to consider:

should we update mkDerivation to retain the old getAllOutputNames behavior? This would trigger a rebuild for all packages in nixpkgs with __structuredAttrs = true...
should we rework the postFixup hook?
to reduce rebuilds, I can change the derivation to only use concatStringsSep on JDK8. Do I do this?

Infinidoge · 2025-07-16T12:33:18Z

Hello mass rebuild. Ideally the root cause for why there is a difference in the function before/after structured attrs would be sorted out without a universe rebuild, however if no solution can be found then I think it is worth doing, as this is a huge oversight.

limwa · 2025-07-16T12:35:31Z

Hey there! I'll change the code to only apply the workaround for JDK8, that should reduce rebuilds significantly!

limwa · 2025-07-16T13:04:07Z

Alright, I've changed the code and rebuilds are now at 101-500.

Infinidoge · 2025-07-16T13:32:18Z

That's a lot better, good catch

symphorien · 2025-07-16T17:45:14Z

in my opinion it is better to target staging and change for the better code now. Conditionals to prevent a mass rebuild like that are only useful to get a change quicker to master until it can be cleaned up in a mass rebuild, but there is no rush to enable debuginfo for jdk8.

limwa · 2025-07-16T18:12:24Z

in my opinion it is better to target staging and change for the better code now. Conditionals to prevent a mass rebuild like that are only useful to get a change quicker to master until it can be cleaned up in a mass rebuild, but there is no rush to enable debuginfo for jdk8.

I don't really have an opinion on this, I'm open to whatever is preferred. If we go through with the mass rebuild, do you think it would be better to do this for all derivations with __structuredAttrs = true (i.e. put these changes into "mkDerivation") instead? Or do we just do it for this derivation?

symphorien · 2025-07-16T21:17:30Z

depends how you feel about fixing all occurences in nixpkgs :) but a change local to openjdk is fine by me.

limwa · 2025-07-16T23:37:39Z

depends how you feel about fixing all occurences in nixpkgs :) but a change local to openjdk is fine by me.

My "best fix" atm is to add another env variable in make-derivation.nix that has the order of the outputs. getAllOutputNames would use this variable instead of the outputs variable. So, in theory, the fix would be only a few lines and quite easy to implement. The problem is it would rebuild all derivations with __structuredAttrs = true and all other derivations that rely on those (so, in practice, a universe rebuild).

The problem with that approach is that derivations that use $outputs would not be fixed. Maybe we could overwrite the outputs array with the correct order right after sourcing .attrs.sh?

nyabinary · 2025-07-17T00:36:54Z

+1 to the doing the code and targeting for staging instead :P better than a hack tbh

limwa · 2025-07-17T22:46:48Z

Turns out autoPatchelf can be used to dramatically simplify the postFixup hook.
I don't think it can get simpler than this... The PR is ready!

wolfgangwalther · 2025-07-19T17:03:00Z

pkgs/development/compilers/openjdk/generic.nix

Came here because I wanted to see how you dealt with getAllOutputNames.. glad to see you just removed it. Will have to keep thinking about this elsewhere, whether we can fix the order of that somehow.

If I were to fix getAllOutputNames, it would be something like this #425773 (comment). Like the comment says, it only addresses getAllOutputNames and scripts using $outputs or ${outputs[@]} would not be fixed.

Also, in response to #425561 (comment), it was said that relying on the order of getAllOutputNames is not correct, which I tend to agree with. Imo, getAllOutputNames should only be used for applying some steps equally to all outputs, where order does not matter.

I also searched Nixpkgs for occurrences of getAllOutputNames, $outputs, ${outputs[*]}, ${outputs[@]} and there were not that many occurrences. And the places where those were being used either the order was not relevant or there was already some fix in place to ensure the order was correct (look at expect-fail.sh). Only some JDK (jdk8, jetbrains-jdk) derivations were expecting the order to be correct and those are already being fixed.

In the end, if a fix for getAllOoutputNames is provided, I think it should be temporary. More (and clearer) documentation is needed to $outputs on the nix side and to getAllOutputNames on Nixpkgs side.

If I were to fix getAllOutputNames, it would be something like this #425773 (comment).

I'd do it differently: I'd just sort the outputs both with and without structuredAttrs alphabetically. That's consistent and rather simple to do.

It implies the same, though: Scripts should not rely on a specific order of these.

@wolfgangwalther it appears this code in multiple-outputs.sh that relies on outputs having a fixed order when returned by getAllOutputNames

nixpkgs/pkgs/build-support/setup-hooks/multiple-outputs.sh

Lines 186 to 189 in 54066a5

local outputFirst

for outputFirst in $(getAllOutputNames); do

break

done

Thanks for finding this. I added it as a note to the tracking issue in #205690.

limwa · 2025-07-21T08:03:46Z

I just saw the review comments on #350736. Would you like me to address some of those (removing strings with just comments, fixing disallowedReferences for JDK8, ...) or do I just leave the PR as-is?

nyabinary · 2025-07-23T01:03:23Z

I just saw the review comments on #350736. Would you like me to address some of those (removing strings with just comments, fixing disallowedReferences for JDK8, ...) or do I just leave the PR as-is?

You can honestly probably do it and then add ;cleanup to it (do it in a different commit though)

limwa · 2025-07-23T13:26:33Z

pkgs/development/compilers/openjdk/generic.nix

patchShebangs replaces /bin/bash automatically

limwa · 2025-07-23T13:27:32Z

pkgs/development/compilers/openjdk/generic.nix

It appears setJavaClassPath is also propagated for jdk8, although it is done in the installPhase

limwa · 2025-07-23T13:28:59Z

pkgs/development/compilers/openjdk/generic.nix

Previously, the build jobs were set based on the available cores on the machine and not the NIX_BUILD_CORES variable

limwa · 2025-07-23T13:30:13Z

pkgs/development/compilers/openjdk/generic.nix

I removed these two lines and the build was still successful, so I figured I could remove them. Should I revert?

limwa · 2025-07-23T13:31:01Z

pkgs/development/compilers/openjdk/generic.nix

This is automatically set based on propagatedBuildInputs

limwa · 2025-07-23T13:31:28Z

pkgs/development/compilers/openjdk/generic.nix

Left a comment because I didn't know if this was correct or not

limwa · 2025-07-23T13:34:59Z

Did the cleanup and left some comments explaining some of the weirder changes. Other than those, the cleanup consisted of:

moving comments out of hooks to prevent rebuilds due to comments
move inputs that are always used together
use more common constructs (e.g lib.optionals vs ${if atLeast17 then "configurePlatforms" else null} = ...)

Built all jdk derivations and everything seems to be okay.

EDIT: it's the first time I'm doing anything like this,.so I'm especially open to suggestions! Thanks!

symphorien · 2025-07-25T21:17:00Z

I will merge this soon unless somebody speaks up.

symphorien · 2025-07-27T09:32:12Z

thank you for your persistence, this kind of change is not easy.

nix-owners bot requested review from FliegendeWurst, Infinidoge, chayleaf, edwtjo and tomodachi94 July 16, 2025 12:30

nixpkgs-ci bot added the 9.needs: reviewer This PR currently has no reviewers requested and needs attention. label Jul 16, 2025

limwa force-pushed the fix/enable-structured-attrs-jdk8 branch from f80d3c7 to 6f6098a Compare July 16, 2025 12:54

limwa force-pushed the fix/enable-structured-attrs-jdk8 branch from 6f6098a to f89e9d8 Compare July 16, 2025 13:01

symphorien mentioned this pull request Jul 16, 2025

jetbrains-jdk: fix with structured attrs #425529

Merged

13 tasks

limwa force-pushed the fix/enable-structured-attrs-jdk8 branch from f89e9d8 to 4d187ee Compare July 17, 2025 02:57

limwa changed the base branch from master to staging July 17, 2025 02:57

nixpkgs-ci bot closed this Jul 17, 2025

limwa force-pushed the fix/enable-structured-attrs-jdk8 branch 4 times, most recently from b4bab36 to 9891244 Compare July 17, 2025 22:34

wolfgangwalther requested review from wolfgangwalther and removed request for wolfgangwalther July 19, 2025 17:00

wolfgangwalther reviewed Jul 19, 2025

View reviewed changes

limwa force-pushed the fix/enable-structured-attrs-jdk8 branch from 9891244 to 1149111 Compare July 23, 2025 13:25

limwa commented Jul 23, 2025

View reviewed changes

pkgs/development/compilers/openjdk/generic.nix Outdated

Copy link

Contributor Author

limwa Jul 23, 2025

Choose a reason for hiding this comment

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

patchShebangs replaces /bin/bash automatically

limwa commented Jul 23, 2025

View reviewed changes

limwa changed the title ~~jdk{8,11,17,23,24}: always enable __structuredAttrs~~ jdk{8,11,17,23,24}: always enable __structuredAttrs; cleanup Jul 23, 2025

nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 24, 2025

limwa added 2 commits July 25, 2025 18:40

jdk{8,11,17,23,24}: always enable __structuredAttrs

8f70ff8

jdk{8,11,17,23,24}: cleanup

1bc0ebe

limwa force-pushed the fix/enable-structured-attrs-jdk8 branch from 1149111 to 1bc0ebe Compare July 25, 2025 17:44

nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 25, 2025

symphorien approved these changes Jul 25, 2025

View reviewed changes

nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jul 25, 2025

symphorien merged commit 29c0b0f into NixOS:staging Jul 27, 2025
26 of 28 checks passed

limwa deleted the fix/enable-structured-attrs-jdk8 branch November 25, 2025 23:11

	local outputFirst
	for outputFirst in $(getAllOutputNames); do
	break
	done

Uh oh!

Conversation

limwa commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

limwa commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Infinidoge commented Jul 16, 2025

Uh oh!

limwa commented Jul 16, 2025

Uh oh!

limwa commented Jul 16, 2025

Uh oh!

Infinidoge commented Jul 16, 2025

Uh oh!

symphorien commented Jul 16, 2025

Uh oh!

limwa commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

symphorien commented Jul 16, 2025

Uh oh!

limwa commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nyabinary commented Jul 17, 2025

Uh oh!

limwa commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limwa Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limwa Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limwa commented Jul 21, 2025

Uh oh!

nyabinary commented Jul 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limwa commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

symphorien commented Jul 25, 2025

Uh oh!

Uh oh!

symphorien commented Jul 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

limwa commented Jul 16, 2025 •

edited

Loading

limwa commented Jul 16, 2025 •

edited

Loading

limwa commented Jul 16, 2025 •

edited

Loading

limwa commented Jul 16, 2025 •

edited

Loading

limwa Jul 19, 2025 •

edited

Loading

limwa Jul 23, 2025 •

edited

Loading

limwa commented Jul 23, 2025 •

edited

Loading