nix-community / napalm

Support for building npm packages in Nix and lightweight npm registry [maintainer=?]
MIT License
104 stars 18 forks source link

Insert `npmCommands` directly into the build shell #16

Closed paluh closed 2 years ago

paluh commented 4 years ago

Thanks a lot for building this tool!

I've encountered some problems during my project packaging - simple patching command like sed failed when it was included in the npmCommand. It seems that the problem is related to the fact that currently commands are passed and used as shell variables by napalm. The simplest example of a similar failing flow could be:

#!/usr/bin/env bash

COMMAND='sed -i "s/x/y/" "file"'
$COMMAND

The above fails with sed: -e expression #1, char 1: unknown command: `"' even though sed command is correct by itself (and can be run direcc I'm not sure but probably arguments are passed altogether with quotes into the sed program when we execute it by variable expansion.

This simple change is enough to fix the problem by avoiding variable usage during npmCommand execution.

Do you think that my approach is correct and I can provide a PR with this change?

nmattia commented 4 years ago

Uh! Everyday I learn something new about bash. Yes, that would make a lot of sense!

paluh commented 4 years ago

Uh! Everyday I learn something new about bash.

Yeah. In my case it is more like "#!/bashing head against the wall" than learning ;-)

I was thinking a bit about my PR and I want to discuss different approach.

What do you think?

jtojnar commented 2 years ago

This was fixed https://github.com/nix-community/napalm/pull/38, the npmCommands can be either string or list of string (which will be joined by newlines) and interpolated directly into the buildPhase.