This repository has been archived by the owner on May 20, 2021. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 59
Changes to avoid rebuilds of pkgConfig.postInstall
code on each nix-build
#161
Draft
Ma27
wants to merge
19
commits into
nix-community:master
Choose a base branch
from
transloadit:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 ;)
Avoid rebuilds
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. |
cc @Lassulus should I base this off nixpkgs then (re NixOS/nixpkgs#108138)? |
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 "commander@2.9.0" 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#commander@6.2.1" 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 `commander@6.1` 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.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 theyarn2nix
subtree innixpkgs
actually contains allyarn2nix
sources, but has a dev script to sync that code withnix-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: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 runningnix-build
. This is needed since we have to fetch metadata about transitive dependencies from either the registry itself or thepackage.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 inpkgConfig.*
.I had to change the format of
yarn.nix
a bit, so existing files would break.