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

fix: nodejs: strict-builder: patch shebangs #510

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tgunnoe
Copy link

@tgunnoe tgunnoe commented Apr 26, 2023

For the scenario where the installed node_modules were used for building (nestjs in my situation), the shebangs were not patched for build time in the strict-builder. That lead to no such file or directory in the sandbox.
reference: https://matrix.to/#/!PrIEtcffTLLzUWvJOS:matrix.org/$zslZWYi4HF0xramEQum6xf808-VQbbJIkN4kbgGFqt0?via=nixos.org&via=matrix.org&via=nixos.dev

Also, the bins weren't runnable permissions.

This didn't seem like an issue within the granular builder, but maybe you all would know better than I if a similar fix is needed there as well.

@hsjobeki @DavHau

@DavHau
Copy link
Member

DavHau commented Apr 26, 2023

@hsjobeki I wonder why the we even produce node_modules that contain broken shebangs. Usually patchShebang is executed automatically in the fixup phase of each individual derivation, so there shouldn't be any corrupt shebangs.

@hsjobeki
Copy link
Contributor

hsjobeki commented Apr 26, 2023

Strange. I‘d need to investigate this. questions:

  • (if needed manually) Should „patchShebangs“ be run only on node_modules/.bin
  • Do we really need „+x“ applied to every file in the whole node_modules?
  • Should patch shebangs be in node-modules-tree.nix (where the node_modules are created.

@tgunnoe
Copy link
Author

tgunnoe commented Apr 26, 2023

(if needed manually) Should „patchShebangs“ be run only on node_modules/.bin

It wasn't straightforward to only patch what's in .binwith patchShebangs. I suppose it needs to follow symlinks or something similar. However, I assumed finally that probably everything in node_modules needs to be patched for their scope, but I dont know how much of a performance loss that would entail.

Do we really need „+x“ applied to every file in the whole node_modules?

No, there can be a cleaner operation for the runnables. Any suggestions?

Should patch shebangs be in node-modules-tree.nix (where the node_modules are created.

I can do it there instead. Sorry, still not completely familiar with this builder

@hsjobeki
Copy link
Contributor

hsjobeki commented Apr 26, 2023

Thanks for your efforts. I appreciate it.
If you could move this fix to node-modules-tree.nix and check if your use case still works.
I think we can merge this and add a todo comment for me to come back later.

@tgunnoe
Copy link
Author

tgunnoe commented Apr 26, 2023

I wonder why the we even produce node_modules that contain broken shebangs

Oh, I suppose since the tree is built fully with python, there's not a fixup phase that the derivation goes into at that time. As far as I understand, anyways. I don't know what that would entail to run the nix path substitution from within the python builds

If you could move this fix to node-modules-tree.nix and check if your use case still works.

Sounds good! no problem

@DavHau
Copy link
Member

DavHau commented Apr 27, 2023

Oh, I suppose since the tree is built fully with python

Oh yeah, I forgot about the node-modules-tree.nix

If you could move this fix to node-modules-tree.nix and check if your use case still works.

Note that patchShebangs can only substitute paths in shebangs for known buildInputs. The runCommandLocal which is used to execute the python builder probably needs nodejs as a build input and any other things that are expected to appear in shebangs.

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 this pull request may close these issues.

3 participants