nix-community / yarn2nix

Generate nix expressions from a yarn.lock file [maintainer=???]
GNU General Public License v3.0
123 stars 61 forks source link

Build workspace dependencies #64

Closed nicknovitski closed 6 years ago

nicknovitski commented 6 years ago

Consider workspace dependencies which are transpiled or processed in some way. Currently, you have to either always remember to run those builds outside of nix before building their depending package in nix and include the built files in the nix source instead of filtering them as you would normally, leading to many unnecessary rebuilds and bloated derivations, or do what I'm trying, which is even worse:

yarn2nix.mkYarnPackage {
# ...
        postConfigure = ''
          for f in ${lib.concatStringsSep " " (builtins.attrNames workspaceDependencies)}; do
            echo $f
            pushd "node_modules/$f"
            yarn install # TODO: do anything but this
            popd
          done
        '';
}

Currently yarn takes the former approach of doing nothing and leaving it up to us, and I think we can do better.

So here's one way: add an attribute called module that duplicates the configure and build steps, but has an install step that copies the built folder as-is rather than a properly nix-y set of outputs.

nicknovitski commented 6 years ago

I'm currently testing this with a small private workspace. It's a bit dicey since it also needs #61 to work, but I think I have it. Everything is building successfully, but there are issues. I'm noticing things taking much longer to build, I believe not just from the building and installing, but from copying so many things around. Also executable files in workspace dependencies are losing their +x. I think both of those can be fixed with doing the linking right.

nicknovitski commented 6 years ago

I think that's it.

Most of the source of complexity turned out to be scoped directories. In brief, when it comes time to link either a workspace dependency or target package @foo/bar, we want to ensure that if the scope directory node_modules/@foo exists, it is a directory of symlinks rather than a symlink to a directory.

nicknovitski commented 6 years ago

Rebased this branch too.

zimbatm commented 6 years ago

and now it needs a rebase again :)

nicknovitski commented 6 years ago

Apparently my uniqueByPackageName function wasn't working, so I did as we discussed and replaced it with adding -f to a few link calls. So packages which end up in workspaceDependenciesTransitive multiple times will get linked multiple times, but I'm not sure when that would matter. Maybe if you had two different packages with the same pname.

e: possibly also there would be strange errors if a workspace had multiple versions of the same package. I haven't tested that.

I'm a little concerned about workspaceDependenciesTransitive: isn't it only getting dependencies down one level, rather than recursing to arbitrary depth?

zimbatm commented 6 years ago

did you try to use pkgs.symlinkJoin? It seems like you are re-implementing a similar mechanism

nicknovitski commented 6 years ago

I hadn't heard of it, but I'll look into it. Especially since i've noticed that I'm handling node_modules/.bin symlinks wrong somehow: some of them are things like jest -> ../../deps/node_modules/myWorkspaceDependency/node_modules/jest/bin/jest.js.

zimbatm commented 6 years ago

Yeah, I am not super confident in all the changes that you made (sorry!) because the workspace feature now makes it too complex for me to understand what is going on. I think we'll need a good testing round before a new release can be made.

zimbatm commented 6 years ago

Some times it's best to merge PRs and discover issues after the fact :)

nicknovitski commented 6 years ago

No problem, I've been thinking that I should add some tests of more complicated workspaces.

zimbatm commented 6 years ago

Okay, let me know when this PR is ready

nicknovitski commented 6 years ago

I thought of a much simpler approach: add a distPhase and just use the tarball for workspaceDependencies.

nicknovitski commented 6 years ago

I think this is ready.

I believe the .bin problem I mentioned stems from the initial workspaces pr; mkYarnModules copies the package.json files of workspace dependencies into subdirectories of deps, and then runs yarn install, which may create node_modules directories in those subdirectories, and then deletes the deps directory entirely, including any dependencies yarn placed there. The problem I noticed was links in the root node_modules/.bin pointing to the deleted directory.

nightkr commented 6 years ago

I'm a little concerned about workspaceDependenciesTransitive: isn't it only getting dependencies down one level, rather than recursing to arbitrary depth?

A package's exposed workspaceDependencies contains all transitive dependencies, since apart from resolution you'll pretty much never care about whether a dependency is direct or not.