Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node2nix ignores package-lock.json on collections of packages when fetching from remote #207

Open
kampka opened this issue Oct 22, 2020 · 6 comments · Fixed by #208
Open

Comments

@kampka
Copy link
Contributor

kampka commented Oct 22, 2020

I have a concrete problem with a package in nixpkgs: https://github.com/NixOS/nixpkgs/tree/master/pkgs/servers/matrix-synapse/matrix-appservice-slack

The package currently fetches the sources from github, compiles the tsc sources and installs the package.
However, node2nix ignores the remote package-lock.json for packages that are defined as a collection in the nixpkgs package.json as per https://github.com/svanderburg/node2nix#deploying-a-collection-of-npm-packages-from-the-npm-registry

This poses a problem when deploying sources that require a correct package-lock.json, as in this case, the dependencies of upstream packages are not compatible (typescript compile) with the dependencies required to build the package.
Thus, bumping the package to version 1.6.0 (latest stable) will fail during npm build.

@svanderburg
Copy link
Owner

@kampka Yesterday, I was trying to develop some test cases so that this kind of change is guarded from potential future breakage.

I wrote two testcases: one that uses a dependency from a local directory (with lock file), and one referring to a git repository (with lock file).

However, what I noticed is that the dependencies in the lock file are ignored. The project simply reevaluates the dependencies provided by the dependency's package.json file and then "freezes" the closure in the project's lock file. In one of my test cases, multiple libraries seem to resolve to their latest versions, instead of older versions that the lock file specifies. So it seems that your theory is not valid.

After studying some NPM docs, I learned that package-lock.json files are not supposed to be used for public libraries (but npm shrinkwrap can). This also explains that if you run npm publish that the package-lock.json file is automatically ignored.

About the matrix-appservice-slack package: I think your modification does seem to fix that package. I believe the incompatibility has a different root cause. Not so long ago, I discovered a subtle flaw in my replicated NPM dependency resolution algorithm -- it tries to mimic NPM's behaviour for reusing dependencies provided by parent node_modules/ folders.

However, my implementation only looks at the name and version attributes. It turns out that more recent versions of NPM also take the origins of the dependency into account, which my implementation ignores.

I believe one of the major reasons that a package may refer to a Git repository for a library dependency (as opposed to the NPM registry) is because that dependency requires some kind of fix/modification. With my current implementation, it may happen that instead of a dependency from Git, a version of a dependency from the NPM registry is used instead (because their name and version are identical).

I'm bit thorn on how to proceed -- I can of course roll this change back, but this will break the matrix-appservice-slack package again.

Implementing a revised dependency resolution algorithm (that also takes the origins of the dependencies into account), is not a trivial modification and will take a bit of time.

I can also for the time being keep your fix included. It will improve compatibility with some packages, but in other cases it might also break things.

@svanderburg svanderburg reopened this Jan 13, 2021
@svanderburg
Copy link
Owner

@kampka I took another look at the matrix-appservice-slack package -- so the summarize: what we want is to deploy a remote development project (development projects are package-lock.json driven), rather than something that is treated as a dependency (for which package-lock.json files are ignored)

Currently, when you write a collections JSON file (a JSON array) then every entry is basically treated the same way as an NPM-dependency and npm install -g: it ignores the package-lock.json file, because this is also what NPM does.

So this explains why after your fix work the package seems to deploy -- because for every local and git dependency the package-lock.json is used all the dependencies and transitive dependencies in the matrix-appservice-slack package are used.

However, the undesired side effect of this change is that also packages with Git dependencies that have a lock file, will have all their "locked dependencies" included. This is not what NPM does, causing differences in the generation process.

I have to look into how I can change this -- what matters for the matrix-appservice-slack package is that only the package-lock.json file of the root project is used. Lock files of all transitive dependencies must be ignored.

I'm thinking about introducing a replacement feature -- giving node2nix the ability to deploy remote development projects. With this feature, we can guarantee the correct generation process.

Moreover, it will also simplify the codebase.

@svanderburg
Copy link
Owner

@kampka I have created a new experimental branch: https://github.com/svanderburg/node2nix/tree/remote-projects with a proof-of-concept implementation that does what I just explained.

I have removed your changes to the Source prototypes that fetch the lock files (because dependencies and transitive dependencies should ignore them). Instead, I made a fetchgit function that can be used to fetch remote development projects.

The idea of this new feature is that you can deploy remote development projects, and for these development projects you can deploy from lock files as well (and in a future revision: also use supplement JSON files etc. if desired).

Normally with a (local) development project, you can generate Nix expressions from a package.json (and optionally a package-lock.json) as follows:

$ node2nix -i package.json -l package-lock.json

The above command expects that the package.json and package-lock.json files are already stored on your local filesystem.

I have added a new option: --git-repository to the CLI that allows you to first fetch the corresponding Git repository and then generate Nix expressions from it. For example, the following command deploys on of my own projects:

$ node2nix -i package.json --git-repository --git-repository git+https://github.com/svanderburg/nijs

After generating the Nix expressions, I can deploy the package as follows:

$ nix-env -f default.nix -iA package

and even spawn a development shell, if desired:

$ nix-shell -A shell

Let me know if this works for you. You should do something similar for the matrix-appservice-slack package (probably you need to use both the --git-repository and -l` options)

The implementation still requires more work before it can be merged into master again.

@svanderburg
Copy link
Owner

@kampka let me know if the feature works for you.

In the meantime I have reverted the lock file inclusion because I ran into a problem with one of my non-public projects.

I intend to release a new version of node2nix very soon. Maybe this feature will go into the next version.

@kampka
Copy link
Contributor Author

kampka commented Jan 27, 2021

@svanderburg I tried to play around with your branch, and it seems to generate correct dependencies.
What strikes me as odd is that I need to provide both the packages package.json and package-lock.json from the remote repository. This seems unnecessary, given that the repo is pulled via --git-repository contains both those files.

@kampka
Copy link
Contributor Author

kampka commented May 31, 2021

@svanderburg Any way we can make progress on the remote-projects branch?
The fact that package-lock.json currently has to be a file that exists on the local file system is a nuisance, but it can certainly be worked around. Otherwise, the branch works fine for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants