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

Adds support for running native binaries without JS intermediaries #5508

Merged
merged 8 commits into from Jun 20, 2023

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Jun 17, 2023

What's the problem this PR addresses?

Yarn currently cannot run native binaries without going through a JS jumper script. Other package managers don't have this restriction, and it makes the yarn run overhead worse on some scripts for little reasons - or doesn't work at all when packages don't use jumper scripts.

Fixes #882

How did you fix it?

Two mechanisms are used:

  • First we check for the binary extension
  • Then we check its magic number

If one of the two match a predetermined list, the binary is spawned without going through Node. This ensures that this logic is called only when the binary is truly a native binary, and will not affect the behaviour of other existing scripts.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

Presumably we'll need to do this for the following locations as well:

await Promise.all(
Array.from(await getWorkspaceAccessibleBinaries(workspace), ([binaryName, [, binaryPath]]) =>
makePathWrapper(binFolder, toFilename(binaryName), process.execPath, [binaryPath]),
),
);

await Promise.all(
Array.from(await getPackageAccessibleBinaries(locator, {project}), ([binaryName, [, binaryPath]]) =>
makePathWrapper(binFolder, toFilename(binaryName), process.execPath, [binaryPath]),
),
);

await Promise.all(
Array.from(packageAccessibleBinaries!, ([binaryName, [, binaryPath]]) =>
makePathWrapper(env.BERRY_BIN_FOLDER as PortablePath, toFilename(binaryName), process.execPath, [binaryPath]),
),
);

packages/plugin-pnp/sources/jsInstallUtils.ts Outdated Show resolved Hide resolved
@arcanis arcanis requested a review from merceyz June 18, 2023 21:33
@merceyz
Copy link
Member

merceyz commented Jun 19, 2023

There are some issues that this probably fixes (for example #882), could you check and link to them so they're closed?

@belgattitude
Copy link

There are some issues that this probably fixes (for example #882), could you check and link to them so they're closed?

Not sure about this PR, but from the title it might fix some turborepo hacks: https://github.com/vercel/turbo/blob/main/packages/turbo/install.js#L190

So it might also relate to

@arcanis arcanis merged commit 3626ea2 into master Jun 20, 2023
19 checks passed
@arcanis arcanis deleted the mael/native-binaries-support branch June 20, 2023 11:46
@jmatsushita
Copy link

jmatsushita commented Jun 29, 2023

I've tried yarn set version 4.0.0-rc.47 with expo@9.0.0-beta.0 in a monorepo but it doesn't fix it.

> yarn workspace client expo info                                                                                                       .../node_modules/expo/bin/cli:3
# Forward all arguments to the local CLI (@expo/cli).
^

SyntaxError: Invalid or unexpected token
    at internalCompileFunction (node:internal/vm:73:18)
    at wrapSafe (node:internal/modules/cjs/loader:1176:20)
    at Module._compile (node:internal/modules/cjs/loader:1218:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:23:47

Node.js v18.16.0

@arcanis
Copy link
Member Author

arcanis commented Jun 29, 2023

This fix is only about native binaries. From your logs, expo is a shellscripts, which aren't allowed in bin at the moment for the reasons outlined in #882 (comment).

@MorisR
Copy link

MorisR commented Jul 20, 2023

Will the changes be also added to yarn 3?
we're still stuck on node v16 and yarn 4 requires node v18

merceyz pushed a commit that referenced this pull request Jul 20, 2023
…5508)

**What's the problem this PR addresses?**

Yarn currently cannot run native binaries without going through a JS
jumper script. Other package managers don't have this restriction, and
it makes the `yarn run` overhead worse on some scripts for little
reasons - or doesn't work at all when packages don't use jumper scripts.

**How did you fix it?**

Two mechanisms are used:

- First we check for the binary extension
- Then we check its magic number

If one of the two match a predetermined list, the binary is spawned
without going through Node. This ensures that this logic is called only
when the binary is truly a native binary, and will not affect the
behaviour of other existing scripts.

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
@merceyz
Copy link
Member

merceyz commented Jul 20, 2023

Yes, I've backported it now so it will be in the next release.

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.

[Bug] SyntaxError when running a "bin" file that isn't authored in JS
5 participants