Flakes Implementation

[brynblack]

Description

This pull request adds flake support to Polykey-CLI, replacing the traditional "legacy" build system of Nix. It simplifies a lot of the Nix build codebase, and increases reproducability and reliability.

Tasks

• 1. Configure flake
• 2. Fix overlay deployment
• 3. Test through nix-build, overlay, flake build, flake overlay, and CI
• 4. Figure out how to get the npmDepsHash into the flake
• 5. Discuss whether we should keep or remove the old deployment system
• 6. Implement remaining minor changes
• 7. Fix CI dry run
• 8. Re-add release.nix style package output (vercel/pkg, binary output format should be the same)
• 9. Minor revisions

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[ghost]

👇 Click on the image for a new way to code review

Legend

[CMCDragonkai]

There's some conflicts with staging.

But some notes:

1. So we are getting rid of default.nix and shell.nix and fully moving to nix build and nix develop.
2. The nixpkgs-matrix can refer to our flakes using just builtins.getFlake this is a new primitive that supports protocol URIs like github:... or flakes:...
3. If flakes:... is more generic, change to using this.

The nixpkgs upstream may move to accommodate more out-of-tree packaging for scalability reasons - the future is decentralized.

[CMCDragonkai]

Also everything in our utils.nix should be factored out into nixpkgs-matrix! So it can be shared between all of our projects!

[brynblack]

This CI dry-run issue is very annoying. I don't understand why the runner doesn't have access to the tmp directory, given its being manually set to the correct permissions within the flake.nix. I saw some discussion about it, but there doesn't seem to be one clear solution.

[CMCDragonkai]

This CI dry-run issue is very annoying. I don't understand why the runner doesn't have access to the tmp directory, given its being manually set to the correct permissions within the flake.nix. I saw some discussion about it, but there doesn't seem to be one clear solution.

Check if tmp actually exists. Run file /tmp or ls /. Run them both beforehand or stat.

[brynblack]

Details

docker run -it --entrypoint /bin/bash c4d61a3b707b
bash-5.2# git clone https://github.com/MatrixAI/Polykey-CLI
Cloning into 'Polykey-CLI'...
remote: Enumerating objects: 2440, done.
remote: Counting objects: 100% (1066/1066), done.
remote: Compressing objects: 100% (316/316), done.
remote: Total 2440 (delta 797), reused 930 (delta 724), pack-reused 1374
Receiving objects: 100% (2440/2440), 2.34 MiB | 6.09 MiB/s, done.
Resolving deltas: 100% (1815/1815), done.
bash-5.2# cd /Polykey-CLI
bash-5.2# git checkout feature-flakes
branch 'feature-flakes' set up to track 'origin/feature-flakes'.
Switched to a new branch 'feature-flakes'
bash-5.2# nix build .#docker
error:
… while calling the 'derivationStrict' builtin

     at /builtin/derivation.nix:9:12: (source not available)

   … while evaluating derivation 'docker-image-polykey-cli-0.1.4.tar.gz'
     whose name attribute is located at /nix/store/f6zwsqxlr744fg8mmgdlq3qi3r1nlk3y-source/pkgs/stdenv/generic/make-derivation.nix:303:7

   … while evaluating attribute 'baseJson' of derivation 'docker-image-polykey-cli-0.1.4.tar.gz'

     at /nix/store/f6zwsqxlr744fg8mmgdlq3qi3r1nlk3y-source/pkgs/build-support/docker/default.nix:598:18:

      597|           imageTag = if tag == null then "" else tag;
      598|           inherit fromImage baseJson;
         |                  ^
      599|           layerClosure = writeReferencesToFile layer;

   (stack trace truncated; use '--show-trace' to show the full trace)

   error: program '/etc/nix/upload-to-cache.sh' failed with exit code 1

bash-5.2# echo "" > /etc/nix/upload-to-cache.sh
bash-5.2# nix build .#docker
bash-5.2# nix develop
Entering "polykey-cli"
bash: ./.env: No such file or directory

mkdir --parents "$(pwd)/tmp"

export PATH="$(pwd)/dist/bin:$(npm root)/.bin:$PATH"

npm install --ignore-scripts
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE package: '@matrixai/[email protected]',
npm WARN EBADENGINE required: { msvs: '2019', node: '^20.5.1' },
npm WARN EBADENGINE current: { node: 'v18.17.1', npm: '9.6.7' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE package: '@matrixai/[email protected]',
npm WARN EBADENGINE required: { msvs: '2019', node: '^20.5.1' },
npm WARN EBADENGINE current: { node: 'v18.17.1', npm: '9.6.7' }
npm WARN EBADENGINE }

up to date, audited 741 packages in 680ms

168 packages are looking for funding
run npm fund for details

found 0 vulnerabilities

set +v
bash-5.2# nix build .#docker
bash-5.2#

[CMCDragonkai]

Check the internals of upload to cache. It is supposed to upload to S3, nothing to do with nix I think.

[CMCDragonkai]

Fast check found an error there.

bin/utils › encodeEscaped should encode all escapable characters
    Property failed after 50 tests
    { seed: 1318872640, path: "49:2:0:15:12", endOnFailure: true }
    Counterexample: ["\\r"]
    Shrunk 4 time(s)
    Got error: expect(received).toBe(expected) // Object.is equality
    Expected: "\\r"
    "
      261 |     fc.assert(
      262 |       fc.property(stringWithNonPrintableCharsArb, (value) => {
    > 263 |         expect(binUtils.decodeEscaped(binUtils.encodeEscaped(value))).toBe(
          |                                                                       ^
      264 |           value,
      265 |         );
      266 |       }),
      at toBe (tests/utils.test.ts:263:71)
      at Property.predicate (node_modules/fast-check/lib/check/property/Property.js:17:86)
      at Property.run (node_modules/fast-check/lib/check/property/Property.generic.js:49:33)
      at runIt (node_modules/fast-check/lib/check/runner/Runner.js:21:30)
      at check (node_modules/fast-check/lib/check/runner/Runner.js:73:11)
      at Object.assert (node_modules/fast-check/lib/check/runner/Runner.js:77:17)
      at Object.assert (tests/utils.test.ts:261:8)
      Hint: Enable verbose mode in order to have the list of all failing values encountered during the run
      at buildError (node_modules/fast-check/lib/check/runner/utils/RunDetailsFormatter.js:131:15)
      at throwIfFailed (node_modules/fast-check/lib/check/runner/utils/RunDetailsFormatter.js:143:11)
      at reportRunDetails (node_modules/fast-check/lib/check/runner/utils/RunDetailsFormatter.js:156:16)
      at Object.assert (node_modules/fast-check/lib/check/runner/Runner.js:81:52)
      at Object.assert (tests/utils.test.ts:261:8)

@amydevs @tegefaulkes

[tegefaulkes]

There's been temperamental failures of some formatting tests since the main release work a while back. Not part of the scope here so it can be ignored. But needs to be addresses eventuallly.

[tegefaulkes]

Seems like I'm taking this over and getting it merged today. Just need to sanity check all the jobs work and the build outputs.

[CMCDragonkai]

Yes for staging CI jobs too, only can be tested once merged.

[tegefaulkes]

I've found some minor things with just getting my bearings.

1. It's possible to run the flake without installing it with nix run .#polykey-cli But in this case it's broken in nixos. It's running the linux packaged output. I think it should be running the application build in this case. OR we fix the linux build to work with the nix system. I think we're missing a patchelf to make it work?
2. While we have an application version defined. its not actually exposed so we can run or build the application version.
3. After adding the application version I can see that it builds fine, but when running it with nix run it's trying to run nixstorepath/bin/polykey-cli but the executables are only aliased as pk or polykey. Adding polykey-cli to this does get it to work.

I'm going to test the other build outputs.

[CMCDragonkai]

My thinking is that there are 2 kinds of builds:

1. A nix-like app build which is just a JS app that depends on nodejs.
2. A packaged ELF.

When building polykey-cli by itself it should be 1. this keeps it the way it used to be - and thus we consider that to be polykey-cli closure. The 2. is the thing that goes into the GitHub releases.

The polykey-cli should not be the "alias" here. How does that work? You should be producing just the standard package.json specified binaries so you can use just pk or polykey. The name of the flake output should have no bearing on the binaries.

[CMCDragonkai]

See https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-run - I think we're missing a attribute to be used to make 3. work with just pk or the more official polykey. It explains how to make nix run run a specific binary.

[CMCDragonkai]

[CMCDragonkai]

We wouldn't want to switch pname because it needs to be used by the buildNpmPackage to avoid org-scoping. This was the case before the flakes and still will be due to us continuing to use the same derivation abstraction.

Instead to override it, use meta.mainProgram.

It might work within the buildNpmPackage, but if it doesn't then you just need to apply it on the outside with an override.

[CMCDragonkai]

Something like:

{ buildNpmPackage, ... }:

buildNpmPackage rec {
  meta = {
    # Other meta attributes like description, homepage, license, etc.
    mainProgram = "polykey";
  };
}

[CMCDragonkai]

Naming of all release artifacts should be similar?

[CMCDragonkai]

PR will need to be squashed and ticked off above.

[tegefaulkes]

nix build can build multiple outputs at once like before. But there is a problem, it doesn't output the paths it built like it used too.

This is going to break the build jobs.

[tegefaulkes]

Just need to add --print-out-paths flag to get the previous behaviour.

[tegefaulkes]

I've fixed up the npmDepsHash.js script. I'm resolving all the above comments since they all relate to the same thing.

I've done the following.

1. Using execSync now
2. Same as before, error is caught and warned but ultimately ignored so non nix systems still work.
3. The exec'ed command is just prefetch-npm-deps ./package-lock.json > npmDepsHash This cuts out all of the processing in the script. Though I need to fix that really quick.

[tegefaulkes]

Everything has been addressed and the CI has passed for the feature branch jobs. I'll move on to merging this and then testing the rest of the CI.

If we run into any more CI job problems I'll fix them in the staging branch.

[tegefaulkes]

I've squashed everything. I'm going to do a quick once over and merge.

[CMCDragonkai]

I'd suggest changing to execFileSync when executing single programs.

[brynblack]

Okay I think I'm happy with everything that's changed in the flakes implementation, I like how its now got a clear separation between the executable package and the Nix specific package that can be ran. Although I do wish we didn't have to have such a separation, if and/or when we do get Nix execution for builds working using the bundled package I think we can remove some complexity here. Otherwise, its good.