Handle file: dependencies #18

balsoft · 2020-04-17T23:42:25Z

Ligo's website uses a file: dependency syntax in package.json. We need to handle that. It turned out to be pretty simple really.

zhenyavinogradov · 2020-05-01T04:51:54Z

Why does the test fail?

balsoft · 2020-05-15T16:03:02Z

Because I'm using builtins.filterSource, and I probably shouldn't. I'll try to figure it out on Monday.

balsoft · 2020-05-19T11:25:24Z

@zhenyavinogradov please review again, tests work now

zhenyavinogradov · 2020-05-20T12:54:55Z

default.nix

        echo 'building npm cache'
        chmod u+w ./package-lock.json
        NODE_PATH=${npmModules} node ${./mknpmcache.js} ${cacheInput "npm-cache-input.json" lock}

        echo 'building node_modules'
        npm $npmFlags ci
        patchShebangs ./node_modules/
+        ${concatMapStringsSep "\n" (dep: "cp -r ${dep}/node_modules/* node_modules") peerDependencies}


I'm not sure I understand the logic for doing this. AFAIU this adds dependencies of local packages to node_modules, but I think that we should respect the behavior of npm install, and if it does not include these dependencies, we should not include them either

But if we don't include those dependencies, the resulting package won't work.

This is a good question: should we mimic the behaviour of npm even if it means more work needs to be done manually, or do we go for "least effort" approach (as I have done here)?

Does that mean the package won't work if it's built with pure npm? I think we don't have to support a case like this then

Yes, it will require additional commands if built with pure npm (npm install complains about peer dependencies and tells you how to install them manually). I don't know what's the reasoning for this. If there is a compelling reason not to automatically resolve peer dependencies this way, I'd be happy to remove this logic. However, if this npm behaviour is for historic reasons only, I think we should keep it the way it currently is becuase this is simply more user-friendly and simplifies the nix expressions needed to build the package.

That’s not for historic reasons, it’s actually the opposite, npm < 3 used to install peer dependencies automatically, now it doesn’t. See npm/npm#6565.

What I’m somewhat confused about is that peer dependencies seem to be some special weird thing meant for plugins or something like this (I have no idea). These are not the same as local (file:) dependencies, are they?

Peer dependencies are transitive dependencies of local dependencies IIUC.

Reference on peer dependencies that gets me confused: https://nodejs.org/en/blog/npm/peer-dependencies/

Oh, ok, so I need to RTFM before writing code as it turns out.

So the peer dependency change doesn't have to do anything with the main premise of this PR -- handling file: dependencies. It just so happens that one of the local dependencies of LIGO's website has peer dependencies that aren't a dependency of the website itself.

Whether my logic of automatically installing peer deps is correct or not is a complex question, because it appears that the issues you've mentioned is to do with npm link and other development-time commands that don't interest us much because nix-npm-buildpackage is targeting CIs.

So, IIUC, in this specific case the best solution would be to revert the peer dependency logic you added, and then... um... fix the build by either fixing the dependency that needs a peer for some reason, or adding the peer dependency to package.yaml of the repo?

(I’m really with Zhenya on not doing what npm doesn’t do. I don’t really understand peer dependencies, but it kinda sounds like one’s project should declare peer dependencies of its dependencies explicitly?)

kirelagin · 2020-05-20T12:52:16Z

default.nix

@@ -117,44 +124,53 @@ with stdenv.lib; let
 in rec {
  mkNodeModules = { src, extraEnvVars ? {} }:
    let
+      localDeps = map dirOfLocal (builtins.filter isLocal (builtins.attrValues (lock.dependencies)));


That’s not very... functional? Instead of matching, filtering, matching, mapping, you could just mapMaybe once (drop isLocal, make dirOfLocal return a “maybe”).

(I’m not sure if there is mapMaybe or catMaybes in the nixpkgs library, but it should be there... in the meantime you can just return a list of 0/1-length and concatMap.)

kirelagin · 2020-05-20T13:03:13Z

default.nix

@@ -90,6 +95,8 @@ with stdenv.lib; let

  commonBuildInputs = [ _nodejs makeWrapper ];  # TODO: git?

+  normalizeName = builtins.replaceStrings [ "@" "." "/" ] [ "_" "_" "_" ];


Would be nice to have some documentation (or comment) for this. Or maybe a library function.

Anyway, this set of symbols looks somewhat arbitrary to me. According to this you might need to escape way more chars than this.

Those are the characters that can appear in javascript package name, but can't appear in nix path name.

Still would be nice to have some documentation.

I found this and it says that the package name can only contain “url-safe” characters. I’m not sure what this means, but I suppose, they refer to the “unreserved characters” from the URL RFC, namely:

unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"

which means that the only problematic one is ~ (which you did not include in your list). (Note that both Nix and npm allow ., but not as the first char.) @ and / in your list, I suppose, come from the scoped packages that have the form @<scope>/<package> and, according to the doc the restrictions for scope names are the same.

tldr; basically, it looks like you need to replace your list with just [~, /] and remove leading @.

(Also, tbh, I would probably replace both ~ and / with - rather than _ for readability reasons.)

kirelagin · 2020-05-20T13:05:49Z

default.nix


        mkdir $out
-        mv ./node_modules $out/
+        cp -r ./node_modules $out/


Because node_modules can contain read-only peer dependencies now, and in that case it can't be moved.

zhenyavinogradov · 2020-05-20T13:28:39Z

Can you please also add a simple test case for this, so we can be sure that we don't break it in the future?

zhenyavinogradov · 2020-05-21T04:40:55Z

default.nix

-      packageLockJson = filteredSrc + "/package-lock.json";
+
+      packageJson = src + "/package.json";
+      packageLockJson = src + "/package-lock.json";


This path will change every time anything in src changes, causing everything to rebuild. As I understand the problem is that now filteredSrc depends on the contents of package-lock.json, causing infinite recursion. In that case we can create another filter expression which contains only package.json and package-lock.json, read package-lock.json from there, and then use it to construct a filter which will include local dependencies

Oh, good point.

@balsoft Do you plan to address this in this PR or would you like to create a separate issue?

I think it's a separate issue.

Why not do it in this PR?

Maybe fixed it here, please take a look

Why not do it in this PR?

Because it has the potential for postponing the merging of this PR forever.

In my opinion caching node_modules is more important than file: dependency support. @balsoft I can help you with finishing this PR if you want

There're only minor fixes left as I see

zhenyavinogradov · 2020-05-27T05:20:37Z

default.nix

+      metaSrc = cleanSourceWith {
+        src = src;
+        filter = path: type: elem (getRelativePath path) [ "package.json" "package-lock.json" ];
+      };


filteredSrc also has a check for canCleanSource to support the situation when src is a nix store path not produced by cleanSourceWith, it should be done here as well. Also, to avoid repetition, you can make filteredSrc a function which accepts localDeps as a parameter and use filteredSrc [] for metaSrc

zhenyavinogradov · 2020-05-27T05:27:41Z

default.nix

          cleanedSource = cleanSourceWith {
            src = src;
-            filter = path: type: elem (getRelativePath path) usedPaths;
+            filter = path: type: any (x: elem x usedPathsRec) (dirOfRec (getRelativePath path));


I just noticed this, but I think any should not be used here, because it will include all parent directories of a dependency, while we only want the exact directory of the dependency to be included. For example, if we have dependency file:./deps/foo, and there also exists ./deps/extra-stuff directory which is not used as a dependency, it will still be included in the filtered source

angerman · 2022-06-17T09:25:11Z

Just ran into this issue; are we still planning on merging this feature?

balsoft requested a review from zhenyavinogradov April 30, 2020 15:31

balsoft requested a review from a team May 19, 2020 13:58

zhenyavinogradov reviewed May 20, 2020

View reviewed changes

kirelagin reviewed May 20, 2020

View reviewed changes

zhenyavinogradov reviewed May 21, 2020

View reviewed changes

balsoft requested review from kirelagin and zhenyavinogradov May 21, 2020 09:50

balsoft force-pushed the balsoft/local-deps branch from f2107f6 to 9fd293f Compare May 26, 2020 12:20

zhenyavinogradov reviewed May 27, 2020

View reviewed changes

balsoft added 6 commits September 22, 2020 00:40

Handle file: dependencies

ff30d83

Make local deps work with peer dependencies

271f07d

Use nixpkgs cleanSource instead of filterSource

64233a6

Remove peer dependency logic

bd74fa6

Add a test for local dependencies

93d11dc

Don't depend on all of src for meta

c4d5cf3

balsoft force-pushed the balsoft/local-deps branch from 9fd293f to c4d5cf3 Compare September 21, 2020 21:42

angerman mentioned this pull request Jun 24, 2022

copy deps folder #50

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle file: dependencies #18

Handle file: dependencies #18

balsoft commented Apr 17, 2020

zhenyavinogradov commented May 1, 2020

balsoft commented May 15, 2020

balsoft commented May 19, 2020

zhenyavinogradov May 20, 2020

balsoft May 20, 2020

zhenyavinogradov May 20, 2020 •

edited

balsoft May 20, 2020

kirelagin May 20, 2020

balsoft May 20, 2020

kirelagin May 20, 2020

balsoft May 20, 2020

kirelagin May 20, 2020

kirelagin May 20, 2020

kirelagin May 20, 2020

kirelagin May 20, 2020

balsoft May 20, 2020

kirelagin May 20, 2020 •

edited

kirelagin May 20, 2020 •

edited

kirelagin May 20, 2020

balsoft May 20, 2020

zhenyavinogradov commented May 20, 2020

zhenyavinogradov May 21, 2020

balsoft May 21, 2020

kirelagin May 26, 2020

balsoft May 26, 2020

zhenyavinogradov May 26, 2020

balsoft May 26, 2020

kirelagin Jul 21, 2020

zhenyavinogradov Jul 21, 2020

zhenyavinogradov Jul 21, 2020

zhenyavinogradov May 27, 2020

zhenyavinogradov May 27, 2020

angerman commented Jun 17, 2022

		@@ -90,6 +95,8 @@ with stdenv.lib; let

		commonBuildInputs = [ _nodejs makeWrapper ]; # TODO: git?

		normalizeName = builtins.replaceStrings [ "@" "." "/" ] [ "_" "_" "_" ];

Handle file: dependencies #18

Are you sure you want to change the base?

Handle file: dependencies #18

Conversation

balsoft commented Apr 17, 2020

zhenyavinogradov commented May 1, 2020

balsoft commented May 15, 2020

balsoft commented May 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenyavinogradov May 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirelagin May 20, 2020 • edited

Choose a reason for hiding this comment

kirelagin May 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenyavinogradov commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angerman commented Jun 17, 2022

zhenyavinogradov May 20, 2020 •

edited

kirelagin May 20, 2020 •

edited

kirelagin May 20, 2020 •

edited