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.

Sign up for GitHub

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

Jump to bottom

feat(v1/nodejs): add modules nodejs-node-modules and nodejs-devshell #540

Merged
merged 3 commits into from
Jul 4, 2023
Merged

feat(v1/nodejs): add modules nodejs-node-modules and nodejs-devshell #540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef94912
feat(v1): new module nodejs-node-modules
DavHau Jun 29, 2023
7d2ff8d
feat(v1): new modules nodejs-devshell
DavHau Jun 29, 2023
67cf11a
feat(nodejs-node-modules): ensure the .bin also exists in the local n…
DavHau Jul 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions v1/nix/modules/drv-parts/nodejs-devshell/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
{
config,
lib,
dream2nix,
packageSets,
...
}: let
l = lib // builtins;

cfg = config.nodejs-devshell;

nodeModulesDrv = dream2nix.lib.evalModules {
inherit packageSets;
modules = [
dream2nix.modules.drv-parts.nodejs-node-modules
{inherit (config) nodejs-package-lock name version;}
DavHau marked this conversation as resolved.
Show resolved Hide resolved
{mkDerivation.src = l.mkForce null;}
];
};

nodeModulesDir = "${nodeModulesDrv}/lib/node_modules/${config.name}/node_modules";
in {
imports = [
dream2nix.modules.drv-parts.mkDerivation
dream2nix.modules.drv-parts.nodejs-package-lock
];

# rsync the node_modules folder
# - tracks node-modules store path via .dream2nix/.node_modules_id
# - omits copying if store path did not change
# - if store path changes, only replaces updated files
# - rsync can be restarted from any point, if failed or aborted mid execution.
# Options:
# -a -> all files recursive, preserve symlinks, etc.
# --delete -> removes deleted files
# --chmod=+ug+w -> make folder writeable by user+group
mkDerivation.shellHook = ''
ID=${nodeModulesDir}
currID=$(cat .dream2nix/.node_modules_id 2> /dev/null)

mkdir -p .dream2nix
if [[ "$ID" != "$currID" || ! -d "node_modules" ]];
then
${config.deps.rsync}/bin/rsync -a --chmod=ug+w --delete ${nodeModulesDir}/ ./node_modules/
echo -n $ID > .dream2nix/.node_modules_id
echo "Ok: node_modules updated"
fi
'';
}
53 changes: 53 additions & 0 deletions v1/nix/modules/drv-parts/nodejs-node-modules/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
{
config,
lib,
dream2nix,
...
}: let
l = lib // builtins;

cfg = config.nodejs-devshell;
in {
imports = [
dream2nix.modules.drv-parts.mkDerivation
dream2nix.modules.drv-parts.nodejs-package-lock
dream2nix.modules.drv-parts.nodejs-granular
];

mkDerivation = {
dontUnpack = true;
dontPatch = true;
dontBuild = true;
dontInstall = true;
dontFixup = true;
preBuildPhases = l.mkForce [];
preInstallPhases = l.mkForce ["installPhaseNodejsNodeModules"];
};

env = {
# Prepare node_modules installation to $out/lib/node_modules
patchPhaseNodejs = l.mkForce ''
nodeModules=$out/lib/node_modules
mkdir -p $nodeModules/$packageName
cd $nodeModules/$packageName
'';

# copy .bin entries
# from $out/lib/node_modules/.bin
# to $out/lib/node_modules/<package-name>/node_modules/.bin
installPhaseNodejsNodeModules = ''
mkdir -p ./node_modules/.bin
localNodeModules=$nodeModules/$packageName/node_modules
for executablePath in $out/lib/node_modules/.bin/*; do
binaryName=$(basename $executablePath)
target=$(realpath --relative-to=$localNodeModules/.bin $executablePath)
echo linking binary $binaryName to nix store: $target
ln -s $target $localNodeModules/.bin/$binaryName
done
'';
};

nodejs-granular = {
installMethod = "copy";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ftr: I still don't believe that's a good default. It's a necessary workaround for some popular broken packages like webpack, so we need the switch. But default still seems wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends I'd say. Do you want the default setting to just work for all packages. Then copy is your choice.
I'd estimate (without knowing) that 50 % of node packages would fail building with symlink strategy.

Maye we should just ty and then decide based on the amount of failing packages?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where that 50% number comes from? I think it heavily depends on the set of packages you use. Symlinks aren't only an essential component of Posix but also of nix specifically as you all know.

So i think it's fair to consider software which doesn't handle them as broken and just document a setting to override where needed.

Forget wasted disk IO and space, th idea of keeping node_modules writeable to appease broken node_modules means that you can't be sure the node_modules you use is the one you've installed from your lockfile; breaking one of the core-assumptions of dream2nix.

Copy link
Contributor

@hsjobeki hsjobeki Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay from dream2nix perspective I agree.
That implies using npm install --package-lock-only with that flag always. Because npm cannot add new dependencies to the node_modules folder it must write to the lockfile first, which then updates the devShell. (sound reasonable)

We should clearly point it out in the docs then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in the long term, as @DavHau already mentioned we should provide a d2n command line utility that provide a shortcut for such general tasks. (and also don't need to remember)

Copy link
Member

@phaer phaer Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a better cli entrypoint than run scripts like "update-locks" would be a good QoL improvement in the medium term.

Regarding the npm flag: I think that would be ideal if possible and i think floco implies that it's possible? Might be missing something, you are definitely more experienced here!
Otherwise: would it be possible to keep ./node_modules itself as directory, but symlink everything by default (and copy specific packages via override) inside it? Or would it make things worse?

Might need more discussion. In the context of this PR is still don't think we should just set it to "copy" and forget about it. Lets err on the side of loudly breaking builds instead of slower builds which might silently diverge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with symlinks is that they introduce a duality in interpretation. Software can either ignore the fact that a symlink is a symlink and interpret it as file or directory, or alternatively interpret the symlink as such, resolve it, and look at the target, ignoring the fact where it was linked from.

So i think it's fair to consider software which doesn't handle them as broken

The way I see it, a software cannot not handle symlinks, it has to pick one of the interpretations. Not sure if one of them is wrong. It might depend on the use-case.

Both ways of interpreting symlinks result in different perspectives, and both are supported by linux, so I'm not sure if we can declare software as broken just because they picked one of the two interpretations. They might have had their reasons which we don't understand.

Forget wasted disk IO and space, th idea of keeping node_modules writeable to appease broken node_modules means that you can't be sure the node_modules you use is the one you've installed from your lockfile

Two different problems are getting mixed up here:

    1. Resolution errors in nodejs build tools. -> installMethod=copy is the solution (if writable or not doesn't matter)
    1. npm in a dev-shell wants a writable node_modules for some operations.

It's not that we keep node_modules writable because they are broken. They are not. It's just to allow npm commands to work.
We can get rid of a writable node_modules by ensuring that we implement an alternative for the npm commands that would break or decide to not support using npm partially or fully.
Maybe this is easy in the end but requires some studying to be sure that we don't break stuff, so I'd prefer to do this in as a separate project.

In a nutshell, within the node ecosystem most tools have chosen to interpret symlinks not in favor of how we'd like to install node_modules with dream2nix.

I propose we call it not optimal instead of broken.

One of the core ideas of dream2nix is to reduce user intervention to a minimum. I think the best way to achieve this is by prioritizing ecosystem compatibility over performance.

Therefore our defaults should be chosen to maximize compatibility, not performance. Performance is important, but not as much as compatibility.

If someone wants to pick a more performant strategy like installMethod=symlink with the cost of increasing the potential for build failures, then they can do this, but it doesn't seem to be a good default considering our goals.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically installMethod=symlink has been the default for dream2nix. My idea was that we'd patch all problematic tools to deal with it. It resulted in too much overhead for my taste. We'd have to patch several different versions of each problematic tooling etc. I think we could go back to this approach in the future, but right now I don't have the capacity for maintaining all the patches.

Copy link
Member

@phaer phaer Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with symlinks is that they introduce a duality in interpretation. Software can either ignore the fact that a symlink is a symlink and interpret it as file or directory, or alternatively interpret the symlink as such, resolve it, and look at the target, ignoring the fact where it was linked from.

Not sure I understand this correctly, because i think if you "ignore the fact that a symlink is a symlink", then it's always a file - never a directory? In my understanding your latter alternative is favorable in pretty much all cases (exceptions include software which is explicitly designed to manage symlinks). I'd assume the main problem with that in the npm ecosystem would be that ".." wouldn't refer to node_modules anymore after a link is resolved?

Anyway, I believe the thing described above should eventually be possible in dream2nix as floco seems to achieve it already.
But i don't mean to block anything here, feel free to merge ofc. (would have requested changes via Github review in other cases)

Therefore our defaults should be chosen to maximize compatibility, not performance. Performance is important, but not as much as compatibility.

I'd argue, correctness, opt-in compatibality, performance ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh damn symlinks. Even talking about them is confusing. what I meant with "ignore the fact that a symlink is a symlink" is jut looking at it it as it wasn't a symlink and directly reading the content of the file or the directory.

I'd assume the main problem with that in the npm ecosystem would be that ".." wouldn't refer to node_modules anymore after a link is resolved?

Exactly. This is what most build tools do by default and therefore fail.

I'd argue, correctness, opt-in compatibality, performance ;)

Correctness is a difficult term I think.

Compared to what most nodejs tools expect from the node_modules tree, building it using symlinks, is incorrect. Compared to what npm does, it's also incorrect. It depends on the perspective.

In another sense, everything that is not natively defined in nix is not correct as it doesn't specify what exactly is needed to build it.
With dream2nix we build a bridge from the incorrect world to the correct world, so in a sense we directly support incorrectness.

};
}
33 changes: 33 additions & 0 deletions v1/nix/modules/drvs/prettier-devshell/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
lib,
config,
...
}: let
l = lib // builtins;
in {
imports = [
../../drv-parts/nodejs-devshell
];

mkDerivation = {
src = config.deps.fetchFromGitHub {
owner = "davhau";
repo = "prettier";
rev = "2.8.7-package-lock";
sha256 = "sha256-zo+WRV3VHja8/noC+iPydtbte93s5GGc3cYaQgNhlEY=";
};
};

deps = {nixpkgs, ...}: {
inherit
(nixpkgs)
fetchFromGitHub
mkShell
rsync
stdenv
;
};

name = l.mkForce "prettier";
version = l.mkForce "2.8.7";
}
32 changes: 32 additions & 0 deletions v1/nix/modules/drvs/prettier-node-modules/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
lib,
config,
...
}: let
l = lib // builtins;
in {
imports = [
../../drv-parts/nodejs-node-modules
];

mkDerivation = {
src = config.deps.fetchFromGitHub {
owner = "davhau";
repo = "prettier";
rev = "2.8.7-package-lock";
sha256 = "sha256-zo+WRV3VHja8/noC+iPydtbte93s5GGc3cYaQgNhlEY=";
};
};

deps = {nixpkgs, ...}: {
inherit
(nixpkgs)
fetchFromGitHub
mkShell
stdenv
;
};

name = l.mkForce "prettier";
version = l.mkForce "2.8.7";
}
Loading