Page MenuHomePhabricator

[Nix] Update nixpkgs pin
ClosedPublic

Authored by jon on Jul 26 2022, 6:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jun 14, 8:41 AM
Unknown Object (File)
Mon, Jun 10, 7:25 AM
Unknown Object (File)
Sat, Jun 8, 8:43 AM
Unknown Object (File)
May 15 2024, 5:59 PM
Unknown Object (File)
May 15 2024, 5:59 PM
Unknown Object (File)
May 15 2024, 5:59 PM
Unknown Object (File)
May 15 2024, 5:59 PM
Unknown Object (File)
May 15 2024, 5:52 PM

Details

Summary

Update the dev shell.

This also includes the changes from
https://github.com/NixOS/nixpkgs/pull/182364 which fixes arcanist.

Arcanist is passing on official nix cache:
https://hydra.nixos.org/eval/1773081?filter=arcanist&compare=1773037&full=#tabs-still-succeed
so it's available for all platforms

The pruned node patch is only relevant for environments which are missing
$HOME (e.g. nix's sandbox builds), but shouldn't affect dev workflow. Also,
may not be relevant, as patch fails to apply as it's "already applied".

Test Plan

nix develop

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat requested changes to this revision.Jul 27 2022, 6:16 AM

As discussed extensively in D4483, I think we should be interpreting Arcanist. It's great that the Nix cache now has a binary of Arcanist, but we still aren't able to consistently build Arcanist because, as @jon mentioned on D4483:

arcanist doesn't have a strict build DAG which enforces that generated parser files are created before some steps which need them.

It seems to me that it's possible that in the future, Nix will stop having a binary of Arcanist that works with our config.

It also seems to me that using a "built" version of Arcanist is a poorly supported workflow of a poorly supported library.

I continue to be confused why we aren't simply using an interpreted version of Arcanist, and I wish I didn't have to keep repeating this feedback.

nix/overlay.nix
34 ↗(On Diff #14999)

Just for the record, "16.14 now requires explicit awaits on all imports" is not accurate

This revision now requires changes to proceed.Jul 27 2022, 6:16 AM

It seems to me that it's possible that in the future, Nix will stop having a binary of Arcanist that works with our config.

It's using the latest from the upstream.

It also seems to me that using a "built" version of Arcanist is a poorly supported workflow of a poorly supported library.

This is how nix creates packages which can use tooling which is separate from the host machine. It resolves all of the dependencies to exact versions.

Also, doing some more digging, the "build" portion of the arcanist package is related to it building phage as well. I can do some work to make the two more separate.

$ cat /nix/store/qmrk84w344rkmsd6ibywf3f0nvrqnhkq-arcanist-20220517/bin/arc
#!/nix/store/qm2lv1gpbyn0rsfai40cbvj3h4gz69yc-bash-5.1-p16/bin/bash -e
export PATH='/nix/store/z9q1zmzaxyxdlcgj1hnwvq5j183x891f-php-with-extensions-8.0.21/bin:/nix/store/jzrbfkww0f5s4fadc4apisnna0h2fklm-which-2.21/bin'${PATH:+':'}$PATH
exec /nix/store/z9q1zmzaxyxdlcgj1hnwvq5j183x891f-php-with-extensions-8.0.21/bin/php /nix/store/qmrk84w344rkmsd6ibywf3f0nvrqnhkq-arcanist-20220517/libexec/arcanist/bin/arc "$@"

I continue to be confused why we aren't simply using an interpreted version of Arcanist, and I wish I didn't have to keep repeating this feedback.

I'm confused as well. Nix doesn't make assumptions about there being a php interpreter on the host machine. The arcanist package uses the very specific version which it gets passed. The exec /nix/store/z9q1zmzaxyxdlcgj1hnwvq5j183x891f-php-with-extensions-8.0.21/bin/php /nix/store/qmrk84w344rkmsd6ibywf3f0nvrqnhkq-arcanist-20220517/libexec/arcanist/bin/arc "$@" line is nix executing the script with the intended php interpreter.

nix/overlay.nix
34 ↗(On Diff #14999)

What would you have instead?

Looks like I might be confused about what's being built vs. interpreted here. Let's talk more in our 1:1 in an hour

nix/overlay.nix
34 ↗(On Diff #14999)
ashoat requested changes to this revision.Jul 27 2022, 1:29 PM
  1. Let's separate the nixpkgs pin change
  2. Let's stop using the Arcanist config on nixpkgs as it's doing stuff we don't need/want
    • Instead we can introduce our own, simple Arcanist config that doesn't do any build steps
    • And later we can upstream this to nixpkgs
nix/overlay.nix
45–46 ↗(On Diff #14999)
This revision now requires changes to proceed.Jul 27 2022, 1:29 PM
jon added inline comments.
nix/overlay.nix
34 ↗(On Diff #14999)

Scope changes to just the nixpkgs update and node fix

jon retitled this revision from [Nix] Update nixpkgs pin, fix arcanist to [Nix] Update nixpkgs pin.Jul 27 2022, 6:13 PM

The comment is super helpful

nix/overlay.nix
51–54 ↗(On Diff #15031)

Two questions:

  1. Are both of these patches necessary for Nix support? Or are they basically security fixes?
  2. Have these patches been upstreamed in newer versions of Node?
This revision is now accepted and ready to land.Jul 27 2022, 7:37 PM
jon marked an inline comment as done.
jon added inline comments.
nix/overlay.nix
51–54 ↗(On Diff #15031)

Are both of these patches necessary for Nix support?

First patch is necessary for darwin build. Second patch doesn't affect the development environment; it is only necessary when using 16.15 and wanting to run node instead an environment which doesn't have HOME set to a directory which exists.

So we want this first, but don't need the second (but nix attempting to apply it will fail)

Or are they basically security fixes?

No, first is build time fix for darwin, second is runtime fix in certain envrionments (e.g. during a nix build).

Have these patches been upstreamed in newer versions of Node?

The first patch is pretty nixpkgs specific and likely not to be upstreamed (nixpkgs is using a slightly stale version of apple_sdk because bumping apple_sdk is a herculean effort). Nixpkgs has the current apple_sdk available, but it's still not currently the default. When the default has been bumped, then the first patch will likely not be needed as well. Second patch is already upstreamed, and will be removed when bumping to 16.17

For context:

@ashoat I know you accepted it, but just wanted to make sure the explanation is fine f or now

Yes it is!! Thank you for explaining.

This revision is now accepted and ready to land.Jul 28 2022, 11:24 PM
This revision was automatically updated to reflect the committed changes.