Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

feat: add an option for turning off Node.js hijacking #1344

Closed
wants to merge 3 commits into from

Conversation

zkochan
Copy link

@zkochan zkochan commented Oct 22, 2021

close #897

@zkochan
Copy link
Author

zkochan commented Oct 22, 2021

What is the right way to make this configurable?

@jesec
Copy link
Contributor

jesec commented Oct 23, 2021

We already have an env variable for that: PKG_EXECPATH=PKG_INVOKE_NODEJS.

@zkochan
Copy link
Author

zkochan commented Nov 1, 2021

Does it mean that the issue #897 can be solved by simply running PKG_EXECPATH=PKG_INVOKE_NODEJS pkg app.js ?

@jesec
Copy link
Contributor

jesec commented Nov 1, 2021

Does it mean that the issue #897 can be solved by simply running PKG_EXECPATH=PKG_INVOKE_NODEJS pkg app.js ?

If you run a pkg executable with PKG_EXECPATH=PKG_INVOKE_NODEJS, it would behave the same as a plain node. That may or may not be the behavior you want.

@zkochan
Copy link
Author

zkochan commented Nov 24, 2021

Why was it closed? Is #897 fixed?

@robertsLando
Copy link
Contributor

Like @jesec said you should get the same result using PKG_EXECPATH env var set to PKG_INVOKE_NODEJS

@zkochan
Copy link
Author

zkochan commented Nov 24, 2021

For sure not the same result because see my change at line 1919

@robertsLando
Copy link
Contributor

You should use an env var so to make it configurable

@robertsLando robertsLando reopened this Nov 24, 2021
@zkochan
Copy link
Author

zkochan commented Nov 24, 2021

ok

@zkochan
Copy link
Author

zkochan commented Dec 19, 2021

This is not doing what I need.

This solution works if I set the env variable, when running the compiled binary. What I want is to build the binary with this setting on.

@zkochan
Copy link
Author

zkochan commented Dec 19, 2021

Probably I need to add a new argument here

pkg/lib/packer.ts

Lines 161 to 175 in f081c63

const prelude =
`return (function (REQUIRE_COMMON, VIRTUAL_FILESYSTEM, DEFAULT_ENTRYPOINT, SYMLINKS, DICT, DOCOMPRESS) {
${bootstrapText}${
log.debugMode ? diagnosticText : ''
}\n})(function (exports) {\n${commonText}\n},\n` +
`%VIRTUAL_FILESYSTEM%` +
`\n,\n` +
`%DEFAULT_ENTRYPOINT%` +
`\n,\n` +
`%SYMLINKS%` +
'\n,\n' +
'%DICT%' +
'\n,\n' +
'%DOCOMPRESS%' +
`\n);`;

@zkochan
Copy link
Author

zkochan commented Jan 13, 2022

Could someone look into this please?

Copy link
Contributor

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

Looks good from a code side. Dunno if @jesec agree

Copy link
Contributor

@jesec jesec left a comment

Choose a reason for hiding this comment

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

This looks categorically wrong.

It does not make sense to embed MOCK_NODE into the executable.

What are you trying to do here? Do you want pkg to extract a regular Node.js runtime from the patched binary? If that's the case:

  • why not just download from nodejs.org?
  • why not put the condition at L66 to revert all patches?
  • why don't you skip all other pkg procedures?
  • we use CLI arguments not env variables for configurations of pkg itself.

@edvald
Copy link

edvald commented Feb 24, 2022

Just FYI, here's how we solved this for ourselves with a patch: #897 (comment)

For clarity, the exact issue for us is the patching of child_process. We needed a way to explicitly disable part of it (at least in lieu of a more general solution which I couldn't come up with myself), so that e.g. scripts run in child processes created by a pkg'd binary could successfully call the parent pkg'd binary. The goal is not to run node directly, which I believe is already sorted with the PKG_EXECPATH=PKG_INVOKE_NODEJS option.

@github-actions
Copy link

This pull-request is stale because it has been open 90 days with no activity. Remove the stale label or comment or this will be closed in 5 days. To ignore this pull-request entirely you can add the no-stale label

@github-actions github-actions bot added the Stale label Jun 15, 2022
@github-actions
Copy link

This pull-request is now closed due to inactivity, you can of course reopen or reference this pull-request if you see fit.

@github-actions github-actions bot closed this Jun 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Command failed: Executing pkg cli from the child process of pkg cli getting failed.
4 participants