Pkg integration #29

tegefaulkes · 2021-07-21T06:33:02Z

Description

This PR is to test using vercel/pkg with native modules.
It imports the following node modules:

level
utp-native
threads.js

It should be able to build via pkg and nix-build method.

Issues Fixed

Fixes Integrate a Node native module like utp-native or leveljs in order to demonstrate vercel/pkg #27

Tasks

import, build and test level
import, build and test utp-native
import, build and test threads

Final checklist

Related issues and PRs:

src/bin/worker.ts

utils.nix

tegefaulkes · 2021-07-22T03:38:52Z

Completed most of the checklist, The last thing to do is check of the GH releases works properly with the changes.
Depending on the output we may have to zip the builds now since they are directories instead of just the executable.

src/bin/typescript-demo-lib.ts

CMCDragonkai · 2021-07-22T03:41:24Z

release.nix

@@ -59,13 +65,16 @@ let
      PKG_IGNORE_TAG = 1;
      buildPhase = ''
        cp ${./package.json} package.json
+        mkdir -p output/prebuilds


Does this need output/prebuilds? Why not just ./prebuilds? The ./ is the working directory of the build. As long as this directory doesn't conflict with anything in the ./ which I believe is the produced source files from the upstream derivations.

Remember this is an overridden derivation, the $src has already unpacked into the current directory.

When I use ./ instead of a temp directory, the result included all of the src files.
I also feel that creating and using a temp directory makes the process more explicit.

Is that because of:

--public-packages "*" \

Is the * picking up everything?

If you must use a a special directory for this. I'd say use pkg as the special directory for vercel/pkg related building. Not output which is confusing with $out.

Ok, I can change it to pkg.
as for --public-packages "*" \ is just flagging everything is a public package since non public packages are included as bytecode by default.. This breaks with the --no-bytecode flag.

Using pkg 5.3.1 doesn't fix the issue for leveldown prebuilds, neither does adding them as assets.
this seems to be an issue with pkg itself, I've posted an issue at vercel/pkg#1264.

release.nix

CMCDragonkai · 2021-07-22T03:42:48Z

release.nix

      '';
      dontFixup = true;
    };
  buildExe = arch:
    stdenv.mkDerivation rec {
-      name = "${utils.basename}-${version}-win32-${arch}.exe";
+      name = "${utils.basename}-${version}-win32-${arch}";


Why change this? The name of the windows executable has an extension of .exe.

The output is a directory now that includes the .exe

You should leave it as the actual executable. Is there other files necessary for the actual $out?

This is not a nix package, so the executable as $out is totally fine.

Plus this actually is integrated into the Gitlab CI/CD which expects a single file as $out. Changing it to a directory adds extra work to do there.

We need to have the output as a directory since we need to include the prebuilds for leveldown.
we can have it as a zip file if needed.

Can this be resolved now back to the way it was?

CMCDragonkai · 2021-07-22T03:44:02Z

Is there a way for you test the CI/CD execution of your changes.

You just need to go into the gitlab yaml file and enable this for your branch as well.

Then go to https://gitlab.com/MatrixAI/open-source/typescript-demo-lib/-/pipelines to manually trigger it. Or https://gitlab.com/MatrixAI/open-source/typescript-demo-lib/-/jobs.

README.md

pkgs.nix

CMCDragonkai · 2021-07-22T04:16:33Z

Is there a way for you test the CI/CD execution of your changes.

You just need to go into the gitlab yaml file and enable this for your branch as well.

Then go to https://gitlab.com/MatrixAI/open-source/typescript-demo-lib/-/pipelines to manually trigger it. Or https://gitlab.com/MatrixAI/open-source/typescript-demo-lib/-/jobs.

Oh if you do this, make sure to switch it back later, we only want builds to occur on master, not on every branch.

CMCDragonkai · 2021-07-22T04:31:35Z

We need `--no-bytecode` because without this flag it doesn't work inside Nix and it isn't deterministic. So up to your decision. As I said if you really need a directory, try `pkg` as the directory name to avoid confusion.

…

On 7/22/21 2:28 PM, Brian Botha wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In release.nix <#29 (comment)>: > @@ -59,13 +65,16 @@ let PKG_IGNORE_TAG = 1; buildPhase = '' cp ${./package.json} package.json + mkdir -p output/prebuilds Ok, I can change it to pkg. as for |--public-packages "*" \| is just flagging everything is a public package since non public packages are included as bytecode by default.. This breaks with the |--no-bytecode| flag. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#29 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHJ6LPSIJSE7VDU7XRLTY6M7PANCNFSM5AXJKZ6Q>.

CMCDragonkai · 2021-07-22T06:30:56Z

You mean nixpkgs commit from master? Does this all work with just the revision provided by nixpkgs? NixOS/nixpkgs@3d64b60 The above is the just the one that brought in `pkg`.

…

On 7/22/21 4:28 PM, Brian Botha wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In pkgs.nix <#29 (comment)>: > @@ -1,5 +1,6 @@ import ( # let rev = "60cce7e5e1fdf62421ef6d4184ee399b46209366"; in - let rev = "04a48a2fcd2370f4e8a8c9fd3394531ccdb30f1e"; in + # let rev = "04a48a2fcd2370f4e8a8c9fd3394531ccdb30f1e"; in + let rev = "53caacaf56640d04180775aee016d2f16d6f083c"; in I picked the latest pkgs commit when I started on this issue. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#29 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHITJMVXE5N4SW42KT3TY63A5ANCNFSM5AXJKZ6Q>.

tegefaulkes · 2021-08-03T02:01:53Z

I have created an upstram PR addressing the issue with the leveldown warnings.
vercel/pkg#1273

tegefaulkes · 2021-08-03T02:11:04Z

When this is done, the changes for js-polykey are not far behind. the only complication would be the jose/jwt throwing warnings. But I've added them as an asset and it seems to be working with manual testing e.g. starting agent + starting a session.

CMCDragonkai · 2021-08-03T12:10:27Z

I'm checking out your change to vercel/pkg now.

Firstly:

git fetch origin pull/1273/head:pr-1273
git checkout pr-1273
# setup nix-shell here
npm install
npm run build
# the build outputs to ./lib-es5, however the executable requires the node interpreter
node ./lib-es5/bin.js  --help

The output:


  pkg [options] <input>

  Options:

    -h, --help           output usage information
    -v, --version        output pkg version
    -t, --targets        comma-separated list of targets (see examples)
    -c, --config         package.json or any json file with top-level config
    --options            bake v8 options into executable to run with them on
    -o, --output         output file name or template for several files
    --out-path           path to save output one or more executables
    -d, --debug          show more information during packaging process [off]
    -b, --build          don't download prebuilt base binaries, build them
    --public             speed up and disclose the sources of top-level project
    --public-packages    force specified packages to be considered public
    --no-bytecode        skip bytecode generation and include source files as plain js
    -C, --compress       [default=None] compression algorithm = Brotli or GZip

  Examples:

  – Makes executables for Linux, macOS and Windows
    $ pkg index.js
  – Takes package.json from cwd and follows 'bin' entry
    $ pkg .
  – Makes executable for particular target machine
    $ pkg -t node14-win-arm64 index.js
  – Makes executables for target machines of your choice
    $ pkg -t node12-linux,node14-linux,node14-win index.js
  – Bakes '--expose-gc' and '--max-heap-size=34' into executable
    $ pkg --options "expose-gc,max-heap-size=34" index.js
  – Consider packageA and packageB to be public
    $ pkg --public-packages "packageA,packageB" index.js
  – Consider all packages to be public
    $ pkg --public-packages "*" index.js
  – Bakes '--expose-gc' into executable
    $ pkg --options expose-gc index.js
  – reduce size of the data packed inside the executable with GZip
    $ pkg --compress GZip index.js

I'm going to assume you manage to make it work.. so he main thing is that we need to get node2nix to work against your fork's branch.

CMCDragonkai · 2021-08-03T12:13:12Z

@tegefaulkes In the future for this sort of work you would want to create the forks under MatrixAI group in github rather than your personal account: https://github.com/tegefaulkes/pkg

It makes it easier for us to maintain forks and maintain overrides in our nixpkgs overlay.

So to start I'm going to fork from vercel/pkg and pull your branch in there https://github.com/MatrixAI/pkg.

CMCDragonkai · 2021-08-03T12:17:15Z

The other really important thing is to always branch off from main or master when creating a PR. This way you can have a main that syncs up with upstream, while creating a feature branch to be PRed into upstream main.

So for now I'll just create a feature branch off main and do the same as what you did.

CMCDragonkai · 2021-08-03T12:21:28Z

This is now done here: https://github.com/MatrixAI/pkg/tree/leveldown

The steps were:

git remote add matrixai git@github.com:MatrixAI/pkg.git
git checkout main
git checkout -b leveldown
# make the change
git add -A
git commit -m 'Removes dictionary override for bundling leveldown - Fixes vercel#1264'
git push matrixai

So now:

»» ~/Projects/pkg
 ♜ git branch -vv                                                                                                           (leveldown) pts/3 22:21:32
  * leveldown 63367b0 Removes dictionary override for bundling leveldown - Fixes vercel#1264
    main      09c3f21 [origin/main] 5.3.1
    pr-1273   6ad2e53 Fixes #1264. Removes dictionary override for bundling leveldown. changes from PR #837 makes this unnecessary now.

This repository and branch will now be the target of our Nix override.

CMCDragonkai · 2021-08-03T12:32:55Z

The nix override work in our overlay is located here: https://gitlab.com/MatrixAI/Engineering/nixpkgs-overlay/-/merge_requests/15

tegefaulkes · 2021-08-04T07:12:40Z

Ok, should be working now. Last thing is to check if the github CI still works.

pkgs.nix

CMCDragonkai · 2021-08-04T07:20:22Z

I don't think anything needs to change in our .gitlab-ci.yml file. It should work fine now.

So please fix up my comments, ensure any linting is done, and also clean up the README and format it and squash all the commits.

tegefaulkes · 2021-08-04T08:12:15Z

Misclicked closed. My bad.

tegefaulkes · 2021-08-04T08:55:27Z

Failing on tests. I'll look it over and fix it up tomorrow.
after that it should be ready to merge.

tegefaulkes · 2021-08-05T04:20:30Z

Checks are passing, looks like it's working. this should be good for a merge after a review. @CMCDragonkai

tegefaulkes · 2021-08-05T04:23:12Z

Squashed.

tegefaulkes · 2021-08-05T04:42:10Z

CMCDragonkai · 2021-08-05T05:21:03Z

Just fixed up a few things:

Removed useless comments from pkgs.nix
Formatted README.md with spacing between the code blocks and text
Restoring the mocks should happen at the end of each test
Indentation issues in release.nix
Cleaned up commit message

CMCDragonkai · 2021-08-05T05:21:30Z

I noticed you also enabled source map for this repo. This is a good idea for a library. For a end-user application, not sure if it is relevant.

tegefaulkes · 2021-08-05T05:24:14Z

I noticed you also enabled source map for this repo. This is a good idea for a library. For a end-user application, not sure if it is relevant.

Ah, jest was throwing some minor errors without it. It's not needed I think.

CMCDragonkai · 2021-08-05T05:24:21Z

You should also always test: nix-build ./release.nix --attr application.

Also we now have a custom override for our pkg here, this would have to done for all the repos using pkg.

Until upstream merges. Our overlay can also have this change applied too in the mean time.

CMCDragonkai · 2021-08-05T05:28:11Z

The application build is currently failing due to the missing node-gyp-build command. It needs to be available to the derivation. We've dealt with this before.

CMCDragonkai · 2021-08-05T05:41:19Z

So it turns out that what's required is that:

     node2nixProd = (
  -    (import (node2nixDrv false) { inherit pkgs nodejs; }).shell.override {
  +    (import (node2nixDrv false) { inherit pkgs nodejs; }).shell.override (attrs: {
  +      buildInputs = attrs.buildInputs ++ [ nodePackages.node-gyp-build ];
         dontNpmInstall = true;
  -    }
  +    })

The override can also take a function which takes the attrs, originally this was not the case. And there was a difference between override and overrideAttrs.

Interestingly you cannot make this work if you use overrideAttrs.

CMCDragonkai · 2021-08-05T05:48:25Z

Now I find that you need node-gyp-build for both node2nixProd and node2nixDev.

This makes sense as the node2nixProd will only install production dependencies, whereas node2nixDev will install both production and development dependencies. So if there's an override, both require the node-gyp-build to be available.

Now that override and overrideAttrs both take a function with attrs, I wonder if this is because override delegates to overrideAttrs if it's an attribute function. Because nixpkgs upstream says that override is meant to override the function parameters. But this doesn't make any sense if there is access to existing attributes.

CMCDragonkai · 2021-08-05T05:53:23Z

Yea I checked override being able to take attrs and applying something like buildInputs is specific to node2nix.

… from source and removes pkg warnings

CMCDragonkai · 2021-08-05T06:18:43Z

Regarding override issues: svanderburg/node2nix#178

I've changed to using override instead of overrideAttrs for both node2nixDev and node2nixProd.

Regarding the build warnings of creating a homeless shelter that's cause pkg is attempting to download prebuilt-binaries but instead building from source should be possible.

This is solved with this environment variable in the release builds:

      # ensure that native modules are built from source
      npm_config_build_from_source = "true";

Regarding the application build, both dev and prod require the node-gyp-build due to leveldown. So this will be important for any future native modules we are adding in, the build system will become ever more complex. So it's ideal we document what each thing is for.

@tegefaulkes still has to fix up the executable itself, the arguments to ./result/bin/typescript-demo-lib should be all optional. I think some arguments were introduced by @emmacasolin before where there was no defaulting value for them. Right now not passing it the right set of args results in weird errors.

This is also important for the docker output which doesn't pass any parameters to the command. It will look like something is broken without this being fixed.

CMCDragonkai · 2021-08-05T06:29:45Z

My last commit ensures parity between nix-shell and nix-build. This should ensure that pkg being used inside nix-shell is the same as that which is used for the release. These changes will need to be applied to js-polykey too.

At the final point make sure to test all builds, application, docker, and the executables. And then merging time!