@emilazy you did some major refactoring recently to openjdk. Do you have time to have a look here? Should only be a small change, but a major rebuild

I don’t really understand the patch. Why do we need to remove references here?

Also, we should not propagate the jdk-bootstrap vs. jdk-bootstrap' error here. disallowedReferences should just be fixed.

I don’t really understand the patch. Why do we need to remove references here?

Because with ca derivations enabled by default, there is this reference which isn't removed

Also, we should not propagate the jdk-bootstrap vs. jdk-bootstrap' error here. disallowedReferences should just be fixed.

This is a bit above my knowledge. I did take a look there and am not that confident to change that. There seems to be only a few derivations which need that reference removed by hand (see the linked issue or #297466) and without this fix, openjdk will fail for everyone trying ca derivations for their config

Because with ca derivations enabled by default, there is this reference which isn't removed

What leaves a reference to the bootstrap JDK? There shouldn’t be any present, so having to strip them out seems concerning.

This is a bit above my knowledge. I did take a look there and am not that confident to change that. There seems to be only a few derivations which need that reference removed by hand (see the linked issue or #297466) and without this fix, openjdk will fail for everyone trying ca derivations for their config

The current state is incorrect and does not prevent references to the correct bootstrap package on JDK 8. I only kept it when merging the packages into one file to avoid rebuilds. If we’re touching the disallowed references code, we should use jdk-bootstrap' consistently.

Because with ca derivations enabled by default, there is this reference which isn't removed

What leaves a reference to the bootstrap JDK? There shouldn’t be any present, so having to strip them out seems concerning.

The same thing that is required to have disallowedReferences here in the first place.

This is a bit above my knowledge. I did take a look there and am not that confident to change that. There seems to be only a few derivations which need that reference removed by hand (see the linked issue or #297466) and without this fix, openjdk will fail for everyone trying ca derivations for their config

The current state is incorrect and does not prevent references to the correct bootstrap package on JDK 8. I only kept it when merging the packages into one file to avoid rebuilds. If we’re touching the disallowed references code, we should use jdk-bootstrap' consistently.

Ah, I missunderstood. I thought you were asking me to fix disallowedReferences in stdenv.mkDerivation (which would fix this and others like these, but is, like I said, above my understanding). If I now understand correctly, the following diff to this PR is what you are asking?:

diff --git a/pkgs/development/compilers/openjdk/generic.nix b/pkgs/development/compilers/openjdk/generic.nix
index 3238dc521561..ff5371882d8e 100644
--- a/pkgs/development/compilers/openjdk/generic.nix
+++ b/pkgs/development/compilers/openjdk/generic.nix
@@ -580,8 +580,6 @@ stdenv.mkDerivation (finalAttrs: {
           mkdir -p $out/nix-support
           #TODO or printWords?  cf https://github.com/NixOS/nixpkgs/pull/27427#issuecomment-317293040
           echo -n "${setJavaClassPath}" > $out/nix-support/propagated-build-inputs
-          # remove references also in CA-derivations
-          find "$out" -type f -exec remove-references-to -t ${jdk-bootstrap'} '{}' +
         ''
       else
         ''
@@ -590,8 +588,6 @@ stdenv.mkDerivation (finalAttrs: {
           # properly.
           mkdir -p $jre/nix-support
           printWords ${setJavaClassPath} > $jre/nix-support/propagated-build-inputs
-          # remove references also in CA-derivations
-          find "$out" -type f -exec remove-references-to -t ${jdk-bootstrap} '{}' +
         ''
     )
     + ''
@@ -601,6 +597,8 @@ stdenv.mkDerivation (finalAttrs: {
       cat <<EOF > $out/nix-support/setup-hook
       if [ -z "\''${JAVA_HOME-}" ]; then export JAVA_HOME=$out/lib/openjdk; fi
       EOF
+      # remove references also in CA-derivations
+      find "$out" -type f -exec remove-references-to -t ${jdk-bootstrap'} '{}' +
     '';

   postFixup = ''
@@ -626,7 +624,7 @@ stdenv.mkDerivation (finalAttrs: {

   # TODO: The OpenJDK 8 derivation got this wrong.
   disallowedReferences = [
-    (if atLeast11 then jdk-bootstrap' else jdk-bootstrap)
+    jdk-bootstrap'
   ];

   passthru =

The same thing that is required to have disallowedReferences here in the first place.

The disallowedReferences here doesn’t change anything about the derivation outputs. It just implements an assertion that the resulting package doesn’t carry an unnecessary reference to the bootstrap JDK. If it does when ca-derivations are turned on, that seems to point to a problem that we shouldn’t just hack around?

The same thing that is required to have disallowedReferences here in the first place.

The disallowedReferences here doesn’t change anything about the derivation outputs. It just implements an assertion that the resulting package doesn’t carry an unnecessary reference to the bootstrap JDK. If it does when ca-derivations are turned on, that seems to point to a problem that we shouldn’t just hack around?

Why do you think it is an assertion? If I read #211783 correctly, it actually removes the reference completely from the output. Following up on that NixOS/nix#4629 (comment) states that the disallowedReferences will not work for ca derivation. The comment from the closed PR which tried to fix it #214044 states

# We exclude Content Addressed derivations as they need to instantiate
# the store path after the derivation is built. Thus discarding
# context does not help in getting rid of the reference.

As @thufschmitt said in NixOS/nix#4629 (comment) this is probably a wontfix in stdenv.mkDerivation

Ergo this needs the removeReferencesTo fix

My understanding of that PR is that it just addresses an infelicity where disallowedReferences pulls in a build‐time dependency. Rather, the issue with #214044 is just that the hack we use to fix it breaks with ca-derivations.

I believe this patch is incorrect; it would mean that the disallowedReferences check would never trigger, even when we want it to. The other ones that have already been merged should probably be reverted. I do not think the correct fix is to sprinkle a workaround like this in every package that uses disallowedReferences across the tree for the sake of an experimental feature with known issues; even if we were to do that, it would probably make more sense to just conditionalize disallowedReferences on CA derivations rather than making it non‐functional. We should fix it at the source in Nix like @thufschmitt suggested in #214044 (comment) or work around it at a lower level like @trofi’s #214044.

I believe this patch is incorrect; it would mean that the disallowedReferences check would never trigger, even when we want it to.

I still don't believe it to be a check but an actual removal of build-outputs from the output. Just not for ca-derivations

I do not think the correct fix is to sprinkle a workaround like this in every package that uses disallowedReferences across the tree for the sake of an experimental feature with known issues;

Agreed. Although I believe ca-derivations to be an important experimental feature, which we should try to make it work. The sprinkles are rather few anyway.

If #214044 actually fixes the issue, I would rather have that in nixpkgs as well. @thufschmitt and @Ericson2314 what is your take on this?

I still don't believe it to be a check but an actual removal of build-outputs from the output. Just not for ca-derivations

No:

yuyuko:~
❭ nix build --impure --expr 'with import (builtins.getFlake "nixpkgs") { }; runCommand "test" { disallowedReferences = [ hello ]; } "echo ${hello} > $out"'
error: output '/nix/store/9w54wmf6pxxz9hf3ddzz50wdmb5ypjvc-test' is not allowed to refer to the following paths:
         /nix/store/ihc48rywbj8ymmjdb2gypyfzhw8yf8zq-hello-2.12.1

Agreed. Although I believe ca-derivations to be an important experimental feature, which we should try to make it work. The sprinkles are rather few anyway.

They are sprinkles that subvert the very purpose of the mechanism they are trying to fix. If you add remove-references-to on top of disallowedReferences, it’s the same as just removing the disallowedReferences, except with more code and more confusion. And we should not remove disallowedReferences checks just because CA derivations exposes a very broken Nix design decision that we haven’t worked around. At most, we could conditionalize disallowedReferences on CA derivations, but if we have to do that it should be done in stdenv.mkDerivation. I strongly believe the previous PRs should be reverted, as they are misleading and break disallowedReferences.

Thanks for the example and the clarification. This did help a lot.
What do you think of an revival of #214044 and a tracking issue to remove the other remove-reference-to fixes?

Yeah, I think that sounds reasonable. The stdenv workaround is sort of weird and broken in the way that things were before we added that code in the first place, but it shouldn’t make things actively worse and it seems an acceptable compromise for testing CA derivations. As far as a tracking issue goes, are there more PRs than #350137 and #297466 that would need undoing?

Yeah, I think that sounds reasonable. The stdenv workaround is sort of weird and broken in the way that things were before we added that code in the first place, but it shouldn’t make things actively worse and it seems an acceptable compromise for testing CA derivations.

Ok, cool.

As far as a tracking issue goes, are there more PRs than #350137 and #297466 that would need undoing?

I don't know, I'll look into it tomorrow 👍

Let's do #214044 instead, please.

I believe this patch is incorrect; it would mean that the disallowedReferences check would never trigger, even when we want it to.

@emilazy I have found 65 19 references in nixpkgs with rg "(disallowedReferences)|(remove-ereference-to)" --stats rg --stats "disallowedReferences" $(rg "remove-references-to" -l) where disallowedReferences and remove-ereferences-to are used together. From a first glance it hasn't anything to do with CA-derivations but seems to be a common theme. But if I understood you correctly, they should never be used together for the same references, right?

Take gnuradio for example

and

So, what should we do about those?

For example in #49856 both (removeReferencesTo and disallowedReferences) were added to reduce closure size. If I understand you correctly, we should just have one or the other?
If so: There is no mention of disallowedReferences in the nixpkgs manual. We should add a best practice to it (Well..What is the best practice here?)

I think that they can make sense in conjunction sometimes: you want to enforce that nobody accidentally makes a reference leak, and there are also known references you want to remove to achieve that. If you strip references from every single file in the output, then the check can never break. But if it’s only one or two files it can be a sensible combination. Even if you’re stripping all references, maybe you’re worried about someone doing something in postFixup or something that undoes it? But generally it doesn’t make too much sense in that case, I think.

In the calibre case, if you used disallowedReferences without removeReferencesTo, the package build would fail. They strip the references from $out/lib/calibre/calibre/plugins/podofo.so because that’s what’s known to have such references; adding `disallowedReferences provides an assertion that more references don’t creep in through other files.

So I am only really concerned with packages that had disallowedReferences to express a property like “no trace of the bootstrap compiler remains in these outputs; the resulting OpenJDK does not encode references to the bootstrap tools”, and then get a sledgehammer remove-references-to applied that means that, were any such reference to creep in, we wouldn’t notice. AFAIK there’s only a couple of ca-derivations PRs that have been merged that did that, as that’s presumably the only motivation anyone has had to add these redundant commands, so those are my main concern.

I still don’t fully understand where the ca-derivations error comes from or why this stripping fixes it, but @trofi’s solution worries me a lot less :)

Yeah, this goes a little bit above my understanding, too :-D Thanks for your explanation about the differences and the usecases for disallowedReferences and removeReferencesTo
@trofi solution seems to work, so I created #359318
I searched through the PRs and the nixpkgs files, but I haven't found more than those.

BTW, I forgot to mention that disallowedReferences is documented in the Nix manual. Possibly we should point to it from the Nixpkgs manual.