Skip to content
This repository has been archived by the owner on May 20, 2021. It is now read-only.

Changes to avoid rebuilds of pkgConfig.postInstall code on each nix-build #161

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Nov 12, 2020

Note: @lheckemann and I developed this for a customer project, so please keep in mind that this is currently far from ready and has some downsides/regressions I'll mention below.

However, this approach works surprisingly well, so we decided to file a (draft)-PR against this repository to discuss if this is integratable into the upstream yarn2nix. We are well-aware though that getting this mergable is not an easy task, so we are already "prepared" to maintain those changes in transloadit/yarn2nix if needed.

Also, when reviewing, please look at the changes except for 9694f00 (which basically re-adds the Nix code of yarn2nix to the repository). This is needed since the yarn2nix subtree in nixpkgs actually contains all yarn2nix sources, but has a dev script to sync that code with nix-community/yarn2nix, so we decided to develop in a fork of this repository.


tl;dr

Usually an expression like this is used when building a yarn project with dependencies that have e.g. C bindings:

with import <nixpkgs> {};
let
  nodeHeaders = fetchzip {
    name = "node-v${nodejs.version}-headers";
    url = "https://nodejs.org/download/release/v${nodejs.version}/node-v${nodejs.version}-headers.tar.gz";
    sha256 = "sha256-sASVCHD8e9P1VPOVLBHpY5wuyhBJvO0qe9bUDj8k3UU=";
  };
in mkYarnModules {
  name = "foo";
  pname = "foo";
  version = "0.0.0";
  packageJSON = ./package.json;
  yarnLock = ./yarn.lock;
  yarnNix = ./yarn.nix;
  pkgConfig.sharp = {
    buildInputs = [ nodePackages.node-gyp python3 pkgconfig vips ];
    postInstall = ''
      node-gyp --nodedir=${nodeHeaders} rebuild
    '';
  };
}

This kind of expression has the downside however that you always need to rebuild the C bindings which slows down CI and dev cycles (if yarn2nix is used for development).

These changes basically move the build scripts defined in pkgConfig into their own derivations and ensure that all required npm dependencies are available as well. Due to that, the time for a recompile is saved.

Downsides

In the current state of the branch, the following features are not available:

  • Right now it's always necessary to generate the yarn.nix before running nix-build. This is needed since we have to fetch metadata about transitive dependencies from either the registry itself or the package.json of a git repository. That kind of information is needed to make sure that only needed dependencies are available in a derivation to build a NPM package with an extra build script in pkgConfig.*.

  • I had to change the format of yarn.nix a bit, so existing files would break.

Ma27 and others added 14 commits October 12, 2020 17:01
This reverts commit 1c903af.

`nixpkgs` only copies the files from `nix-community/yarn2nix`. Also, we
need those to track our own changes on `yarn2nix`.
…postInstall`

Especially when using `yarn2nix` in a dev-workflow, the rebuilds can be
fairly time-consuming (and thus slowing down the dev-cycles) due to
rebuilds of e.g. C bindings with `node-gyp` in user-defined
`postInstall`-hooks.

This patch aims to work around it by moving those build processes into
their own derivations and copying their `$out` into the final
`node_modules/`-store-path.

However, this improvement comes with a few non-trivial changes and
drawbacks[1]:

* Those post-install hooks require their dependencies from
  `package.json` in their build environment. To make those available,
  it's needed to write those into `yarn.nix` by resolving those from the
  previously parsed `yarn.lock`. This has to happen in `yarn2nix` itself
  to avoid the need for even more IFD.

* To make sure that each derivation has the correct transitive
  dependencies available it's needed to write *all* version constraints
  that point to a single tarball from the Yarn registry into `yarn.nix`.

  PLEASE note that this bullet-point and the previous one require a
  non-trivial change to the format of `yarn.nix`.

* `yarn2nix` has `node-fetch` as additional dependency which is needed
  to fetch all dependencies of packages defined in `pkgConfig` to make
  sure those exist in their own derivation. For git dependencies the
  `package.json` is fetched from the pre-downloaded store-path in the
  runtime of `yarn2nix`. Because of that, `yarn2nix` must be executed
  *before* running `mkYarnPackage` in `nix-build`.

* FOR NOW it's not possible to build define build-scripts for transitive
  dependencies.

[1] And thus it's questionable if it'll ever land in upstream
   `yarn2nix`.
Unfortunately, this change requires a previously generated `yarn.nix`.
The reason for this is that metadata from all dependencies needs to be
fetched to create dedicated build-envs for dependencies defined in
`pkgConfig.*`.
To make sure that we don't DDoS the Yarn registry (and get an ECONNRESET
after several requests).
Otherwise this will only work in combination with nix-prefetch-git,
which _doesn't_ prefetch submodules by default (unlike
pkgs.fetchgit). We may need to change this so that both use submodules
if we ever encounter a package that won't build without submodules, or
add an option for it…
Not sure exactly how yarn does it, but getting weird cases as errors
is nicer than if it blows up in weird ways :)
This shouldn't make a difference in practice, since this happens in
the nix build directory which should be -x for others. Nevertheless,
setting a+w is something that's only a good idea in very rare cases,
so let's not normalise it ;)
@lheckemann
Copy link
Contributor

We should probably fix the tests ^^

@Ma27
Copy link
Member Author

Ma27 commented Dec 21, 2020

We should probably fix the tests ^^

I'd like to get some feedback first: also, I removed some features here, so this needs some more discussion before getting the testsuite running again.

@Ma27
Copy link
Member Author

Ma27 commented Jan 3, 2021

cc @Lassulus should I base this off nixpkgs then (re NixOS/nixpkgs#108138)?

Ma27 and others added 5 commits November 2, 2021 22:45
This is because GitHub has deprecated the unsecured `git://` access[1].
By regenerating our internal `yarn.nix` I confirmed that there are no
hash-changes, so this should be fully backwards-compatible.

[1] https://github.blog/2021-09-01-improving-git-protocol-security-github/
generateNix: force using https URLs
…f needed

It's known that using workspaces usually produces a slightly different
`node_modules/`[1]. While this used to be no big deal for us, we actually
triggered a case where "dependency hoisting" — the process of moving
dependencies of workspaces up in the final `node_modules/`-tree —
becomes a problem, basically issue 5705[2].

The problem was triggered by the following - simplified - dependency
tree:

    <root>
    |- commander (6.1)
    |- mocha (3)
    |  `- commander (2.9)
    `- sql-generate (1.5)
       `- commander (2.9)

`yarn2nix` created a workspace of the package `root` with
`node_modules/` containing `mocha`/`sql-generate`. In the `yarn
install` operation these are all placed into a single `node_modules/` in
`$out`, however `commander` from 2.9 is preferred over 6.1:

    $ yarn why commander
    [...]
    => Found "[email protected]"
    info Has been hoisted to "commander"
    info Reasons this module exists
       - "workspace-aggregator-dc20c757-df45-41d7-bdba-927d267212a3" depends on it
       - Hoisted from "_project_#root#mocha#commander"
       - Hoisted from "_project_#root#sql-generate#commander"
    => Found "root#[email protected]"
    info This module exists because "_project_#root" depends on it.

Even specifying `nohoist` for both dependencies inside the temporary
`package.json` that's built by `yarn2nix` doesn't solve the problem
since `yarn` still doesn't put the newer `[email protected]` into
`$out/node_modules/`, I don't really know why, could be a `yarn`-bug
though.

Anyways, we're not even using the workspace feature and since it's known
to produce different results, it's IMHO easier to just disable the
entire feature if no `workspaces` are defined in `package.json`.

[1] https://classic.yarnpkg.com/lang/en/docs/workspaces/#toc-limitations-caveats
[2] yarnpkg/yarn#5705
…uired

mkYarnModules: only run `yarn install` inside a workspace-structure if needed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants