Paths

Table of Contentst

[Nix] Update nixpkgs pin
ClosedPublic
Actions

Authored by • jon on Jul 26 2022, 6:46 PM.

Details

Reviewers

varun
ashoat

Commits

rCOMM9fbc556bb448: [Nix] Update nixpkgs pin

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

• jon created this revision.Jul 26 2022, 6:46 PM

Herald added subscribers: • abosh, atul, • adrian, tomek. · View Herald TranscriptJul 26 2022, 6:46 PM

• jon mentioned this in D4483: [Nix] Support building Android through gradle on Linux.Jul 26 2022, 6:47 PM

Harbormaster completed remote builds in B10900: Diff 14999.Jul 26 2022, 6:59 PM

• jon requested review of this revision.Jul 26 2022, 6:59 PM

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?

• jon requested review of this revision.Jul 27 2022, 11:54 AM

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)	See ENG-929

Let's separate the nixpkgs pin change
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 mentioned this in D4660: [Nix] Rephrase motivation behind pinning to better reflect upstream.Jul 27 2022, 5:13 PM

• jon marked an inline comment as done.Jul 27 2022, 5:48 PM

• jon added inline comments.

nix/overlay.nix
34 ↗	(On Diff #14999)	Addressed in https://phab.comm.dev/D4660

Scope changes to just the nixpkgs update and node fix

• jon marked 2 inline comments as done.Jul 27 2022, 6:00 PM

• jon added a child revision: D4661: [Nix] Remove arcanist override of php8.0.Jul 27 2022, 6:10 PM

• jon added a parent revision: D4604: [Nix] Add support for tunnelbroker build.

• jon edited parent revisions, added: D4660: [Nix] Rephrase motivation behind pinning to better reflect upstream; removed: D4604: [Nix] Add support for tunnelbroker build.

Harbormaster completed remote builds in B10925: Diff 15031.Jul 27 2022, 6:12 PM

• 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: Are both of these patches necessary for Nix support? Or are they basically security fixes? 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 requested review of this revision.Jul 28 2022, 12:01 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: patch 1 patch 2