3.x fix import fallback

[legobeat]

• fix*yarnpkg-cli): properly handle missing physicalPath in runBinary

https://github.com/yarnpkg/berry/pull/33/files#diff-0b87a3e94c60d5153e56080500921e63b9ce72910785f15fb9ff6d4a389126d9R27

Should the first argument here have been process.execPath? It seems odd to call physicalPath directly only if falsey?

[merceyz]

This is intended for 3.x. I picked the closest base branch as target that I found on the repo.

That would be https://github.com/yarnpkg/berry/tree/cherry-pick/next-release

[merceyz]

Should the first argument here have been process.execPath? It seems odd to call physicalPath directly only if falsey?

Probably but that branch is dead code, physicalPath is never falsy due to this check:

berry/packages/yarnpkg-cli/sources/main.ts

Lines 102 to 105 in d64baeb

	if (!xfs.existsSync(yarnPath)) {
	process.stdout.write(cli.error(new Error(`The "yarn-path" option has been set (in ${configuration.sources.get(`yarnPath`)}), but the specified location doesn't exist (${yarnPath}).`)));
	process.exitCode = 1;
	} else {

[legobeat]

@merceyz The redundant path-checking has been addressed and process exit codes are now ensured to be numbers (preserving existing cases and default of 1).

[legobeat]

Separate but it seems like this is never restored after execution of the binary?

[merceyz]

I'm failing to see what issue this is fixing, could you add a test that fails without these changes or provide the steps to reproduce it?

[merceyz]

If the path is set and is truthy but the file doesn't exist this wont throw the same error it did before.

[legobeat]

The exit code before was a non-number string. This is not valid. Do we actually want to preserve that behavior?

For string type, only integer strings (e.g.,'1') are allowed

https://nodejs.org/api/process.html#processexitcode_1

[legobeat]

(I put this here specifically for consistency with old behavior - otherwise using error.errno, ie -2 would have seemed appropriate)

[arcanis]

Fwiw I'm -1 on merging something in the 3.x branch that isn't a straight cherry-pick from master. I can imagine a change like this changing some edge case behaviour somewhere, and I wouldn't want us to have to deal with additional hotfixes.

@legobeat 4.0 was released in Oct 2023 and the migration from 3.x is reasonably small; why not upgrade?

[legobeat]

4.0 was released in Oct 2023 and the migration from 3.x is reasonably small; why not upgrade?

Let's just say I contribute to repos where this is not under my control, and I believe it's neither here nor there? Would expect many community repos and deployments at large to stay on v3 for a considerable time still and that's not really my crusade.

[arcanis]

and I believe it's neither here nor there

Sure it is. You're asking us to ship code exclusive to the 3.x branch, which we don't dogfood anymore, and possibly affect other 3.x users which may or may not rely on the behaviour you're changing. We need to understand why it's important enough to be backported. "Fixes a vulnerability" is often a yes, "Changes the text color from orange to red" a no.

Would expect many community repos and deployments at large to stay on v3 for a considerable time

The project sometimes releases cherry-picks to older lines if a maintainer is interested in backporting fixes, but we don't promise doing that. I think you'll have a better time properly upgrading the version every once in a while rather than waiting for hotfixes to be backported on older release lines "for a considerable time".

[legobeat]

and possibly affect other 3.x users which may or may not rely on the behaviour you're changing.

This should be the focus. This patch is not primarily intended for my personal benefit.

Separately, I do think there are more out there blocked by #6258 until they can upgrade from v3 to v4 without introducing vulnerability venues through existing legacy dependencies.