search

NixOS / hydra

Hydra, the Nix-based continuous build system
http://nixos.org/hydra
GNU General Public License v3.0
1.16k stars 298 forks source link

Explicit build products not picked up if they point outside the NAR #865

Open ctheune opened 3 years ago

ctheune commented 3 years ago

I've been debugging an issue with the netboot job (both on hydra.nixos.org and hydra.flyingcircus.io) where the build products are declared in nix-support/hydra-build-products but do not show up in hydra.

I stumbled over PR #827 but @ajs124 says his problem seems to be that the hydra-build-products file is empty. In my case the hydra-build-products file correctly there (I also don't have remote builders involved) and filled.

The issue specifically arises from https://github.com/NixOS/hydra/commit/5b4df3ad5a2b318a29b6bebc3e6be353b8c61d65#diff-6ae5dd4d7cad7e6c0000230c3c51eb5b9bdc92595ccf90d81e6e72544eefe18cR114 where before the change any file available from the store (and AFAICT references to a remote store would have been fetched at that point already).

While I was trying to create a patch that fixes this (by rolling back some of the changes) I stumbled over this comment:

            /* Ensure that the path exists and points into the Nix
               store. */
            // FIXME: should we disallow products referring to other
            // store paths, or that are outside the input closure?

My perspective: yes, we want to be able to reference store paths from the input closure (but I'd also agree that we don't want to refer to things outside the input closure) because otherwise synthetic builds that simply are there to reference other outputs will need to copy things and those could be quite large (netboot in my case requires around 550MiB) and I'd prefer if we didn't have to explicitly copy things around.

So, my patch (https://github.com/NixOS/hydra/compare/master...flyingcircusio:build-products-missing-external-references?expand=1 ) seems to likely be a step in the wrong (long term) direction as @edolstra wanted to get rid of those fs-accessor-based calls ... so either this means we have to retrieve NARs that we refer to in the build products through the same API or extend the store API so we can get access to the hashes (I guess the NARs would need to be transferred in any case so that the generated URLs in hydra to those products will be reachable).

Anyway, the patch could be a stopgap until we resolve the discussion from the comment (and the NAR change seems to have accidentally preempted that discussion) ...

The specific patch doesn't build cleanly yet, but I wanted to open up the discussion here as I have to leave for the week and can continue working on this next week if I get a bit of direction from @edolstra or maybe @grahamc ...

ctheune commented 3 years ago

Gnah. I screwed up my patch. Will provide a fixed version in a minute.

ctheune commented 3 years ago

Ok, the patch now builds cleanly. I can try working with it next week on our instance and maybe get some feedback here until then ...

ctheune commented 3 years ago

Alright. I managed to try out my patch on our hydra and it works: https://hydra.flyingcircus.io/build/80515

I'm going to provide this as a PR for more specific discussions.

ctheune commented 3 years ago

Hmm. Maybe @Ericson2314, @grahamc @Lassulus or @Kloenk want to chip in?

ctheune commented 3 years ago

Just to leave a note here. As I updated our hydra my patch doesn't work cleanly anymore and I don't want to keep updating it. So, as a fix, I started copying the necessary results into the build that merges the kernel + ipxe file for referencing via hydra-build-products.

However: can we talk about making this fail explicitly instead of silently swallowing? Not sure I can provide a fix, but maybe that idea resonates with others here.