pnpm / supi

Fast, disk space efficient installation engine. Used by pnpm
MIT License
24 stars 5 forks source link

fix: pass unsafePerm to lifecycle call #26

Closed etamponi closed 6 years ago

etamponi commented 6 years ago

This fixes https://github.com/pnpm/pnpm/issues/963

zkochan commented 6 years ago

Please add a test for this use case to

https://github.com/pnpm/supi/blob/master/test/install/lifecycleScripts.ts

etamponi commented 6 years ago

Here you go :) this puts us in par with npm: scripts will be run by non-root users, and root will run scripts only if --unsafe-perm is set. This also requires no additional changes to @zkochan/lifecycle

Do you still want to cover this with a test?

zkochan commented 6 years ago

Thanks!

Yeah, a test is still needed. We don't want to have regressions.

etamponi commented 6 years ago

Is the update available already?

Il 24 dic 2017 15:14, "Zoltan Kochan" notifications@github.com ha scritto:

@zkochan commented on this pull request.

In src/api/extendOptions.ts https://github.com/pnpm/supi/pull/26#discussion_r158602994:

@@ -83,5 +83,12 @@ export default async (opts?: SupiOptions): Promise => { extendedOpts.prefix = path.join(extendedOpts.prefix, subfolder) } extendedOpts.rawNpmConfig['registry'] = extendedOpts.registry

  • if (!extendedOpts.rawNpmConfig['unsafe-perm']) {
  • extendedOpts.rawNpmConfig['unsafe-perm'] = process.platform === 'win32' ||

I added unsafePerm to the options pnpm/types@a35d2a3 https://github.com/pnpm/types/commit/a35d2a3376b9ec211b4ffb9a602237d67eeea6ef

So just update @pnpn/types and use it as a regular option.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pnpm/supi/pull/26#pullrequestreview-85442499, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjUNEatgr2FtWcIlofu9TTNfSapiy3Lks5tDlwqgaJpZM4RLL5x .

etamponi commented 6 years ago

Tests added :)

etamponi commented 6 years ago

Do you want me to consolidate this PR into a single commit?

zkochan commented 6 years ago

yes. That would be great.

you can rewrite this branch and it will update the CR

etamponi commented 6 years ago

Here you go :)