Skip to content
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.

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

fix build against nix 2.7 #1171

Closed
wants to merge 5 commits into from
Closed

Conversation

ajs124
Copy link
Member

@ajs124 ajs124 commented Mar 10, 2022

Includes #1161 (@Ma27)

not particularly well tested, but builds and tests pass (I think)

Ma27 and others added 4 commits March 9, 2022 23:46
@ajs124 ajs124 mentioned this pull request Mar 16, 2022
Comment on lines 160 to 161
auto localStore = state.store.dynamic_pointer_cast<LocalFSStore>();
auto drvPath = localStore->printStorePath(drv->requireDrvPath());
Copy link
Member

Choose a reason for hiding this comment

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

localStore might be null, please do not do this way.

Suggested change
auto localStore = state.store.dynamic_pointer_cast<LocalFSStore>();
auto drvPath = localStore->printStorePath(drv->requireDrvPath());
auto drvPath = state.store->printStorePath(drv->requireDrvPath());

@@ -201,7 +202,6 @@ static void worker(
/* Register the derivation as a GC root. !!! This
registers roots for jobs that we may have already
done. */
auto localStore = state.store.dynamic_pointer_cast<LocalFSStore>();
Copy link
Member

Choose a reason for hiding this comment

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

Please put back

@@ -210,7 +210,7 @@ static void worker(

nlohmann::json out;
for (auto & j : outputs)
out[j.first] = j.second;
out[j.first] = localStore->printStorePath(j.second);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
out[j.first] = localStore->printStorePath(j.second);
out[j.first] = state.store->printStorePath(j.second);

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Deployed to my own Hydra, seems to work fine 👍

@Ericson2314
Copy link
Member

@edolstra Feel comfortable merging this one?

@cole-h
Copy link
Member

cole-h commented Mar 23, 2022

I feel like we should hold off on this, due to the recent issues with Hydra that maybe are connected to the bump to 2.6 (e.g. see the Infra and Infra alerts channels on Matrix, where this has been a ~hot topic).

@Ericson2314
Copy link
Member

Oh, I didn't know about that. Bummer. Perhaps we need a way to disable non-blocking GC until the kinks are worked out? (Note, they could be Hydra side, e.g. a lack of temp roots, too.)

Per #1166 I would say we need a staging branch for every version of Nix newer than the on on master, in addition to Nix HEAD.

So we could revert 2,6 on master, but then revert the revert on staging-nix-2.6, and merge that and this into staging-nix-2.7, and then make staging-nix-head or so on top of that.

@edolstra
Copy link
Member

@cole-h Do you have more info on those issues?

@grahamc
Copy link
Member

grahamc commented Mar 24, 2022

@edolstra could you, @rbvermaa, and I do a call some time to go over them? They're a bit hard to track down and I think we might have an easier time sorting through them together and then writing them down.

@rbvermaa
Copy link
Member

I would prefer it if we can allow Hydra to build against 2.4 and up. We had to revert back to 2.4, because we were getting builds that kept hanging, which significantly disrupted our builds.

@Ericson2314
Copy link
Member

@rbvermaa The Nix internals change too much for that to be possible at this time. But we could keep the branches tied to older Nix around afterwords.

@rbvermaa
Copy link
Member

It seems surprising to me to not support the versions of nix that are provided in NixOS stable channel.

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 25, 2022

@rbvermaa Hydra has always been built against a single version of Nix. The daemon protocol is meant to be interoperable, so other Nix versions for the builders will work. (And I have unmerged PRs to test that interoperability.)

Hydra previously was pinned to random Nix versions. In recent months, The Nix it is pinned to is always stable version of Nix. This is definitely an improvement.

@grahamc grahamc mentioned this pull request Mar 29, 2022
@grahamc
Copy link
Member

grahamc commented Mar 29, 2022

Applied in #1188.

@grahamc grahamc closed this Mar 29, 2022
@ajs124 ajs124 deleted the fix/nix-2.7 branch March 29, 2022 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants