Skip to content

fix(lifecycle): skip verify for install hooks#8957

Merged
zkochan merged 20 commits intomainfrom
verify-deps-before-run-preinstall-infinite-loop-8954
Jan 10, 2025
Merged

fix(lifecycle): skip verify for install hooks#8957
zkochan merged 20 commits intomainfrom
verify-deps-before-run-preinstall-infinite-loop-8954

Conversation

@KSXGitHub
Copy link
Copy Markdown
Contributor

@KSXGitHub KSXGitHub commented Jan 9, 2025 •
edited
Loading

Fixes #8954

@KSXGitHub KSXGitHub force-pushed the verify-deps-before-run-preinstall-infinite-loop-8954 branch from 0b3f89b to 832412d Compare January 10, 2025 02:26
@KSXGitHub KSXGitHub force-pushed the verify-deps-before-run-preinstall-infinite-loop-8954 branch from 832412d to 80aaf9e Compare January 10, 2025 02:34
@KSXGitHub KSXGitHub marked this pull request as ready for review January 10, 2025 03:10
@KSXGitHub KSXGitHub requested a review from zkochan as a code owner January 10, 2025 03:10
@zkochan
Copy link
Copy Markdown
Member

zkochan commented Jan 10, 2025

We already have a lot of env variables set for scripts, can't we just use them? For instance, check for the existence of the npm_lifecycle_script env variable. If it exists, the process is executed from inside a script.

@KSXGitHub KSXGitHub marked this pull request as draft January 10, 2025 10:37
@KSXGitHub KSXGitHub marked this pull request as ready for review January 10, 2025 18:05
@KSXGitHub
Copy link
Copy Markdown
Contributor Author

KSXGitHub commented Jan 10, 2025

@zkochan I have changed the code into checking for npm_lifecycle_event. But it only skip on npm_lifecycle_event that are install hooks because a blanket skip doesn't work (it disables the entire verify-deps-before-run feature). Also, I keep the old env around because pnpm exec needs it to not repeat check.

KSXGitHub
KSXGitHub commented Jan 10, 2025
View reviewed changes
KSXGitHub
KSXGitHub commented Jan 10, 2025
View reviewed changes
@zkochan
Copy link
Copy Markdown
Member

zkochan commented Jan 10, 2025

I see, but then I am not sure npm_lifecycle_event is needed. The previous approach was fine.

zkochan
zkochan reviewed Jan 10, 2025
View reviewed changes
exec/plugin-commands-script-runners/src/shouldRunCheck.ts Outdated
// are likely to also execute other `pnpm run`.
// We don't want this potentially expensive check to repeat.
// The solution is to use an env key to disable the check.
export const SKIP_ENV_KEY = 'pnpm_run_skip_deps_check'
Copy link
Copy Markdown
Member

@zkochan zkochan Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we could avoid the additional env variable by setting this env variable instead:

npm_config_verify_deps_before_run=false

I think this will work unless someone explicitly sets the verify-deps-before-run setting via CLI options. However, in that case it is probably expected that we don't skip the check.

Copy link
Copy Markdown
Contributor Author

@KSXGitHub KSXGitHub Jan 10, 2025

Choose a reason for hiding this comment

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

Replacing it breaks some tests related to nested scripts. It probably means that it fails to skip check in some situations.

The code is in branch replace-custom-env-with-npm-config-env. The commit is 907b58a.

@KSXGitHub
Copy link
Copy Markdown
Contributor Author

KSXGitHub commented Jan 10, 2025

I think it's fine. The previous version of shouldRunCheck made a wrong assumption (or rather, an assumption was right but become wrong in v10) that params[0] is a script name (i.e. a key in the "scripts" object). Now the new version of shouldRunCheck just reads npm_lifecycle_event for a script name.

@KSXGitHub KSXGitHub requested a review from zkochan January 10, 2025 19:54
@zkochan
Squashed commit of the following:
4105974 
commit 49656eccb106c58ed7fa88d3aa7806bbb3410835
Author: Zoltan Kochan <[email protected]>
Date:   Fri Jan 10 21:50:00 2025 +0100

    test: fix

commit 4a2a961
Author: khai96_ <[email protected]>
Date:   Sat Jan 11 03:04:09 2025 +0700

    refactor: remove unnecessary restore step

commit 343459e
Author: khai96_ <[email protected]>
Date:   Sat Jan 11 02:59:32 2025 +0700

    test: fix

commit 907b58a
Author: khai96_ <[email protected]>
Date:   Sat Jan 11 02:50:30 2025 +0700

    refactor: replace env
zkochan
zkochan reviewed Jan 10, 2025
View reviewed changes
exec/plugin-commands-script-runners/src/shouldRunCheck.ts Outdated
]

export const shouldRunCheck = (env: Env, scriptName: string): boolean => !env[SKIP_ENV_KEY] && !SCRIPTS_TO_SKIP.includes(scriptName)
export const shouldRunCheck = (env: Env): boolean => !EVENTS_TO_SKIP.includes(env.npm_lifecycle_event)
Copy link
Copy Markdown
Member

@zkochan zkochan Jan 10, 2025

Choose a reason for hiding this comment

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

Isn't it fine to do this

Suggested change
export const shouldRunCheck = (env: Env): boolean => !EVENTS_TO_SKIP.includes(env.npm_lifecycle_event)
export const shouldRunCheck = (env: Env): boolean => env.npm_lifecycle_event != null

We won't ever verify deps in nested scripts anyway (because we set verify-deps-before-run to false) and npm_lifecycle_event isn't defined when you run the first pnpm run

@zkochan
Copy link
Copy Markdown
Member

zkochan commented Jan 10, 2025

blanket skip doesn't work (it disables the entire verify-deps-before-run feature)

that's probably because the tests are running inside pnpm test. We should be able to fix that.

@zkochan zkochan merged commit c96eb2b into main Jan 10, 2025
@zkochan zkochan deleted the verify-deps-before-run-preinstall-infinite-loop-8954 branch January 10, 2025 22:14
@zkochan zkochan mentioned this pull request Jan 11, 2025
Closed
4 tasks
aklkv added a commit to aklkv/ember-a11y-refocus that referenced this pull request Jan 27, 2025
@aklkv
fix: pnpm verify-deps-before-run=install flag
cbbb518 
ref: pnpm/pnpm#8957
@aklkv aklkv mentioned this pull request Jan 27, 2025
Merged
MelSumner pushed a commit to ember-a11y/ember-a11y-refocus that referenced this pull request Jun 12, 2025
@aklkv
fix: pnpm verify-deps-before-run=install flag (#398)
2594cde 
ref: pnpm/pnpm#8957
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zkochan zkochan zkochan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

install scripts that run other scripts can cause an infinite loop with verify-deps-before-run=install

2 participants

@KSXGitHub @zkochan