fix: propagate --ignore-scripts to yarn v1 dependency bootstrap if enableScripts=false

legobeat commented 2 months ago

What's the problem this PR addresses?

This passes --ignore-scripts for the bootstrapping of Yarn Classic packages if enableScripts==false.

6258

How did you fix it?

Read settings from configuration
Iif configured to disable lifecycle scripts: Extend arguments passed to yarn v1 with --ignore-scripts .

Checklist

[x] I have read the Contributing Guide.

[x] I have set the packages that need to be released for my changes to be effective.
- Note: I marked all as patch to make the check pass locally but I'm not certain they are actually all required. Could use some guidance in case this needs adjusting.
[x] I will check that all automated PR checks pass before the PR gets reviewed.
[ ] Add test coverage for new branch by adding new test-case
- Might need some assistance in adding test. I am not familiar enough with the usage of git workspaces here.

arcanis commented 1 month ago

I'm not 100% convinced this is the right approach. While it makes sense to do that, it would only work for Yarn Classic - not for npm, neither pnpm. It would also only affect one setting.

I don't have a very strong opinion on it, but it feels to me this might be something that should be left to the user.

legobeat commented 1 month ago

I'm not 100% convinced this is the right approach. While it makes sense to do that, it would only work for Yarn Classic - not for npm, neither pnpm. It would also only affect one setting.

I don't have a very strong opinion on it, but it feels to me this might be something that should be left to the user.

Good point that this PR is addressing the Yarn Classic case and is therefore only a partial resolution of #6258 in its current state.

I don't have a very strong opinion on it, but it feels to me this might be something that should be left to the user.

Is that not precisely what enableScripts is intended for?

If the user relies on enableScripts as a security mechanism to disable lifecycle scripts, it's not worth much if all it takes for a malicious/compromised package to unlock its scripts is to ship a lockfile...

(I also believe Yarn Classic has some mechanism to override this via environment variables, in case anyone explicitly wants to e.g. enable install scripts for all dependencies except those with yarn v1 lockfiles in the package root?).

naugtur commented 1 month ago

I would argue (and the file name scriptUtils.ts is kinda supporting me here) that setting enableScripts=false should prevent the whole process of building the external package from happening.

People set enableScripts=false for security reasons, not for speed or convenience
enableScripts=false already breaks more prominent things than this
when enableScripts=false is set the expectation is no scripts would run, which is not the case here - this functionality does run not only for local folders but also git protocol dependencies.

legobeat commented 1 month ago

@naugtur That makes sense. The docs say:

If false, Yarn will not execute the postinstall scripts from third-party packages when installing the project (workspaces will still see their postinstall scripts evaluated, as they're assumed to be safe if you're running an install within them)

This does not seem to include git dependencies.

This is being implemented in #6280 (which would make this PR redundant).

yarnpkg / berry