Wasn't able to run this locally since my Nix setup is broken (beyond the zshrc stuff), but running on NixOS via Buildkite here: https://buildkite.com/comm/android-build-nix/builds/2#018217ad-a103-48e5-998e-f56e2007d39d

Could you specify what architectures/operating systems this was tested on?

x86_64-linux NixOS, after doing the SDK + NDK installation through android studio (although sdkmanager would likely work as well)

I can't get it to work on

• aarch64 macOS

This likely won't work because ANDROID_SDK_ROOT isn't pointing to anything, I would need to test this before setting it on both platforms

x86 nixOS (emulated via qemu via docker on aarch64 Darwin host):

error: unable to load seccomp BPF program: Invalid argument

This looks related to https://github.com/NixOS/nix/issues/5258, which is a docker-related issue, but the current workaround is to modify nix's behavior.

Harbormaster failed remote builds in B10694: Diff 14726!

This was intermittent network issue while fetching dependencies, restarting fixed it.

Was chatting with @atul and neither of us is really sure what the product impact here is. The team (outside of @jon) doesn't use NixOS, so the only benefits I can see here are:

1. To improve @jon's personal custom workflow. Here, I wonder if we should be spending cycles supporting a custom niche use case, as opposed to having @jon use macOS primarily like the rest of the team
2. To provide an alternative to the CI build that we currently do with Debian. Here, it's not really clear if the change will be that impactful

At a high-level, I think we should be prioritizing Nix work that has an immediate impact on dev velocity and dev experience across all devs on the team. Until the whole team is actively using Nix for the primary development workflow, I'd prefer if we could deprioritize any other Nix work.

To help make the prioritization super clear, here are some clarificiations:

1. NixOS support should be deprioritized in favor of macOS Nix support
2. CI Nix support should be deprioritized in favor of dev environment Nix support
3. Nix as a replacement for Docker should be deprioritized in favor of Nix as a replacement for Homebrew and the dev environment instructions

That said this is a small diff so not opposed to landing it. But unfortunately because it seems to only pertain to a custom workflow of @jon's, @atul's unable to test it...

Spun up a x86 nixOS machine on EC2 to give it a try, and got the following on nix develop:

[root@ip-172-31-26-187:~/comm]# nix develop --extra-experimental-features "flakes nix-command"
error: builder for '/nix/store/hc01f564s3wx40f6fnxi85p0jikjzqj7-arcanist-20220517.drv' failed with exit code 2;
       last 10 log lines:
       > g++ -c -fPIC -Wall -O3 -minline-all-stringops -o parser.yacc.o parser.yacc.cpp
       > parser.y:8:10: fatal error: node_names.hpp: No such file or directory
       >     8 | #include "node_names.hpp"
       >       |          ^~~~~~~~~~~~~~~~
       > compilation terminated.
       > make: *** [Makefile:66: parser.yacc.o] Error 1
       > make: *** Waiting for unfinished jobs....
       > Wrote C++ definition.
       > Wrote PHP definition.
       > make: Leaving directory '/build/source/support/xhpast'
       For full logs, run 'nix log /nix/store/hc01f564s3wx40f6fnxi85p0jikjzqj7-arcanist-20220517.drv'.
error: 1 dependencies of derivation '/nix/store/4r6habzv850108kc5p9g043mb2cmk0s8-nix-shell-env.drv' failed to build

Could definitely be messing something up here, anything else I should try?

Could definitely be messing something up here, anything else I should try?

Interesting... I have the same exact drv, but able to build it fine:

[16:32:47] jon@jon-desktop /home/jon/comm/comm (jonringer/tunnelbroker|REBASE)
$ nix eval .#legacyPackages.x86_64-linux.arcanist.drvPath
"/nix/store/hc01f564s3wx40f6fnxi85p0jikjzqj7-arcanist-20220517.drv"
[16:32:49] jon@jon-desktop /home/jon/comm/comm (jonringer/tunnelbroker|REBASE)
$ nix-build /nix/store/hc01f564s3wx40f6fnxi85p0jikjzqj7-arcanist-20220517.drv
/nix/store/4hqv72jgh77vb51ax9h19mi08dsp33j5-arcanist-20220517

This is likely an issue of the arcanist build itself, in that the build isn't thread safe. I see that they use bison, which means that it's possible that they don't have a build rule for the outputs generated, so a later build step is executing before those files exist. In this case "node_names.hpp".

To attempt this, you can try nix-build /nix/store/hc01f564s3wx40f6fnxi85p0jikjzqj7-arcanist-20220517.drv --cores 1, to make the build single threaded

I did use these changes to do D4448.

I think it's still a move in the right directions, even though the main benefactor is NixOS (which I know is not a primary use case).

As for prioritization, this diff took very little time, but unblocked me for D4448

I did use these changes to do D4448.

I think it's still a move in the right directions, even though the main benefactor is NixOS (which I know is not a primary use case).

As for prioritization, this diff took very little time, but unblocked me for D4448

That's really confusing, since D4448 is landed but this one isn't! Does that mean that D4448 doesn't actually need this diff? Or does that mean that things are in a broken state (for macOS Android builds) until this diff is landed?

1. If D4448 works fine for macOS Android builds without this diff, and this diff is just adding NixOS support, then I don't think it should've been prioritized. (Happy to still land it now that it's up.)
2. On the other hand, if D4448 breaks macOS Android builds without this diff, it shouldn't've been landed before this diff was.

This is likely an issue of the arcanist build itself, in that the build isn't thread safe. I see that they use bison, which means that it's possible that they don't have a build rule for the outputs generated, so a later build step is executing before those files exist. In this case "node_names.hpp".

To attempt this, you can try nix-build /nix/store/hc01f564s3wx40f6fnxi85p0jikjzqj7-arcanist-20220517.drv --cores 1, to make the build single threaded

Is there a way to configure nix-build so that the user doesn't have to manually pass --cores 1 to it every time they build? One thing that worries me about Nix is that it often feels fragile when working with code that doesn't follow its "best practices". The reality is that lots of tools we use will probably not follow Nix "best practices", so it's critical that Nix has ways to configure things to handle those tools well.

Separately, it's a bit confusing to me why we need to be "building" Arcanist at all... in my case it's just a simple interpreted PHP script. Why can't we just use the PHP interpreter here? It feels like scope creep that we're trying to build Arcanist at all.

Does that mean that D4448 doesn't actually need this diff?

For FHS linux distros no. The existing docker CI gate was already handling the Android SDK stuff, so the diff was more or less to align with the CommonCpp being the include path for header files.

Or does that mean that things are in a broken state (for macOS Android builds) until this diff is landed?

They shouldn't be broken, the c++ related changes are reflected in both the cmake and header files. If there was successful workflow before, it will be successful after the diff. I was commenting on that I hadn't tested on mac.

Is there a way to configure nix-build so that the user doesn't have to manually pass --cores 1 to it every time they build?

In /etc/nix/nix.conf, you can state cores = 1. But builds will take a LONG time on average. I can just upstream the pinning in nixpkgs, and later update to the official cache. It's unfortunate that arcanist is in a largely unmaintained status though. https://secure.phabricator.com/D21853

Upstream PR: https://github.com/NixOS/nixpkgs/pull/182364

One thing that worries me about Nix is that it often feels fragile when working with code that doesn't follow its "best practices".

One of the reason's why I've had to make patches to protobuf, fbjni, and soon aws sdk. These usually aren't long term issues, but it's kind of sad how much software "barely works".

Separately, it's a bit confusing to me why we need to be "building" Arcanist at all... in my case it's just a simple interpreted PHP script. Why can't we just use the PHP interpreter here? It feels like scope creep that we're trying to build Arcanist at all.

That's how nix deals with interpreted scripts, it bundles the script and "nixifies" them. This enables the, "just add it to the dev shell, and it just works" workflow and avoids the "also install php 7.0, and place this script somewhere in your PATH" workflow.

Arcanist + phabricator is not the best example either, it's been officially unmaintained since July 2021, only receiving some community activity.

Separately, it's a bit confusing to me why we need to be "building" Arcanist at all... in my case it's just a simple interpreted PHP script.

Is that just an entrypoint? similar to how java usually exports a wrapper script? the arcanist code base is quite extensive: https://github.com/phacility/arcanist

It has about ~120k LoC (C++ and PHP)

Is this ready for me to check out on the nixOS instance and test? Or should we mark it "Changes Planned" for now?

The Node build step takes a long time, so just let me know when it's ready

NixOS specific changes were moved to https://phab.comm.dev/D4602

@atul this is ready to test on any x86_64-linux machine + Nix

@ashoat darwin support: https://phab.comm.dev/D4605

In D4483#131720, @jon wrote:

@atul this is ready to test on any x86_64-linux machine + Nix

Nice! Will check out the commit and give it a shot

Is there no way to use prebuilt binaries for Node? Would that be possible if we weren't limited to Node 16.13?

Would've been good to specify:

This is listed in the nix directions to enable flakes: https://github.com/CommE2E/comm/blob/master/docs/nix_dev_env.md#nix-flakes

This command fails if yarn cleaninstall hasn't been run in the root of the repository first:

That's probably fair, but this is also true in the existing workflow. I'll update the test commands

I'd prefer to take the more supported workflow. Can we please move to installing Arcanist the same way we do in the dev environment instructions? We should avoid the build step, and in general we should avoid picking Nix best practices over a tried and true process.

This would be remedied largely by https://linear.app/comm/issue/ENG-1393/add-new-nix-binary-cache, no one would need to build packages if this was fulfilled.

If the Arcanist install instructions say to install PHP 7 and interpret the PHP, then we should be configuring Nix to install PHP 7 and intrepret the PHP.

Nixpkgs has removed php74 because it's end-of-life soonish (probably sooner than it should though). https://github.com/NixOS/nixpkgs/commit/e91811bbe107b7dad31075254344a3ec57cfa4de

What do you want me to do from here? I can:

• Remove arcanist from dev-shell
• package php74 locally, but that means that everyone will need to build once (until the cache is running). The error from atul was from trying to build a package.
• Wait: I have a PR for nixpkgs to pin it to php8.0, https://github.com/NixOS/nixpkgs/pull/182364

Asking the user to edit /etc/nix/nix.conf to make our dev environment work goes against the whole point of this work, which is to make the dev environment work with a single, simple command.

It's already part of the installation instructions to avoid having to pass --experimental-features "nix-command flakes" all the time. However with cores, I would suggest not doing that, the comm-specific nix cache would make all of the local building largely irrelevant, they would just pull from the comm cache.

Unfortunately I'm still confused. Can you try answering these questions again?

I'm trying to slowly make the dev-shell more inclusive of all the workflows. This is just a small step in that direction. I've been able to build it locally on my NixOS with this diff and the NixOS support diff.

That's really confusing, since D4448 is landed but this one isn't! Does that mean that D4448 doesn't actually need this diff? Or does that mean that things are in a broken state (for macOS Android builds) until this diff is landed?

They are ordered this way in the stack because I needed them to be ordered this way for my nix-based workflow. The diff itself is fine in the existing workflow (e.g. download the sdk + ndk, install jdk manually). As part of the rebasing, I've just been prioritizing "what can go in, thus I'm less likely to cause errors when handling 20+ rebasing commits all the time".

If D4448 works fine for macOS Android builds without this diff, and this diff is just adding NixOS support, then I don't think it should've been prioritized. (Happy to still land it now that it's up.)

Maybe, but it's a major QoL for me. And the code changes took me very little time, and the workflow (with exception of the actual 3 lines of nixos-specific logic) can be extended to any linux distro.

On the other hand, if D4448 breaks macOS Android builds without this diff, it shouldn't've been landed before this diff was.

Agreed, but if there was a passing android build before, it should pass after the diff.

Is there a way to configure nix-build so that the user doesn't have to manually pass --cores 1 to it every time they build?

I mentioned --cores 1 because it lowers the non-determinism of the arcanist build significantly. Re-trying nix develop probably would have also worked. Yes, this is not a great experience, but the appropriate action would be to fix it on the arcanist side, as this is an issue which exists outside of nix and in arcanist.

One thing that worries me about Nix is that it often feels fragile when working with code that doesn't follow its "best practices".

Usually this isn't much of an issue if you just want to use nix for development tools as impure workflows are fine. But it's more of an issue if you want to package something using nix (e.g. nix build .#comm.tunnelbroker). Since the current work is just relating to development QoL this usually isn't much of an issue. Also for the software that is failing at best practices, i've been upstreaming fixes for them. A contributing factor to the current "use a docker dev environment" is likely stemming from the fact that some of these upstreams were broken to begin with, so you couldn't make use of the cmake installation workflow, but instead needed to directly import them as if they were a co-located code base.

The reality is that lots of tools we use will probably not follow Nix "best practices", so it's critical that Nix has ways to configure things to handle those tools well.

For development environments, this shouldn't be much of an issue.

Separately, it's a bit confusing to me why we need to be "building" Arcanist at all...

Since I changed the default php version from what's available in nixpkgs, nix will re-build the package locally. This can be avoided if we setup that nix cache.

in my case it's just a simple interpreted PHP script.

Arcanist is quite a large code base: https://github.com/phacility/arcanist

Why can't we just use the PHP interpreter here?

Because arcanist is more than just a php script

It feels like scope creep that we're trying to build Arcanist at all.

Said another way, nix "derivations" are a merkel tree of all inputs. I changed one of the inputs (e.g. php8.1 -> php8.0), so it needs to build the new derivation. Above I mentioned the three options moving forward.

You should request review again so this ends up in @ashoat's queue

@atul for the darwin issues, I created https://github.com/NixOS/nixos-homepage/issues/876 so nix can at least gracefully handle apple update nukes.

I ran into some issues around updating to 12.5, I was able to get it fixed by doing the following:

sudo rm /etc/{bashrc,zshrc}.backup-before-nix
sh <(curl -L https://nixos.org/nix/install)

Anyways, it seems like there is an option 4 here... just install PHP 8.0 in the Nix config, and use that to interpret Arcanist?

That is the current state. It just happened to fail during a nix develop which triggered a build.

It is a huge problem that it is does not work "out of the box" in Nix without --cores 1

It can work. It worked with me and I have --cores 24 as my default. It's just arcanist doesn't have a strict build DAG which enforces that generated parser files are created before some steps which need them.

Huh? The Nix cache is not relevant here... it appears to me that you made a very opinionated (and expensive) decision to build Arcanist here (a non-standard workflow), and I don't understand the justification at all.

https://github.com/NixOS/nixpkgs/pull/182364 was merged, we should just be able to update the flake

Looks like there is a high cost to packing something using Nix. We should probably continue deferring this work, and instead prioritize using Nix as a development tool. Regarding impure workflows, we should embrace those and focus and getting the dev environment working with nix develop rather trying to use Nix for everything and making everything perfectly pure and Nix-compatible.

Agreed.

@ashoat I created https://phab.comm.dev/D4646 to address arcanist issue. It no longer needs to be built by us. It's available with php80 from nixpkgs.

Talked to @jon offline – we're going to unblock this as soon as he provides detailed step-by-step instructions in the Test Plan for how to test this using macOS

@atul you're still a blocking reviewer. Since it worked on darwin, are you okay with rubber stamping this for now?