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

fix: a modified patch should update the deps on install #4918

Merged
merged 5 commits into from
Jun 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/strange-peas-tease.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"@pnpm/build-modules": patch
"@pnpm/core": patch
"@pnpm/headless": patch
"@pnpm/lockfile-types": patch
"@pnpm/resolve-dependencies": patch
---

Update the dependencies when a patch file is modified.
5 changes: 5 additions & 0 deletions .changeset/two-seals-add.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@pnpm/types": minor
---

Add PatchFile type.
7 changes: 2 additions & 5 deletions packages/build-modules/src/buildSequence.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import graphSequencer from '@pnpm/graph-sequencer'
import { PackageManifest } from '@pnpm/types'
import { PackageManifest, PatchFile } from '@pnpm/types'
import filter from 'ramda/src/filter'

export interface DependenciesGraphNode {
Expand All @@ -16,10 +16,7 @@ export interface DependenciesGraphNode {
optionalDependencies: Set<string>
// eslint-disable-next-line @typescript-eslint/no-explicit-any
requiresBuild?: boolean | any // this is a durty workaround added in https://github.com/pnpm/pnpm/pull/4898
patchFile?: {
hash: string
path: string
}
patchFile?: PatchFile
}

export interface DependenciesGraph {
Expand Down
1 change: 1 addition & 0 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"@pnpm/calc-dep-state": "workspace:3.0.0",
"@pnpm/constants": "workspace:6.1.0",
"@pnpm/core-loggers": "workspace:7.0.4",
"@pnpm/crypto.base32-hash": "workspace:1.0.0",
"@pnpm/error": "workspace:3.0.1",
"@pnpm/filter-lockfile": "workspace:6.0.7",
"@pnpm/get-context": "workspace:6.2.1",
Expand Down
37 changes: 33 additions & 4 deletions packages/core/src/install/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
stageLogger,
summaryLogger,
} from '@pnpm/core-loggers'
import { createBase32HashFromFile } from '@pnpm/crypto.base32-hash'
import PnpmError from '@pnpm/error'
import getContext, { PnpmContext, ProjectOptions } from '@pnpm/get-context'
import headless, { Project } from '@pnpm/headless'
Expand All @@ -26,6 +27,7 @@ import {
writeLockfiles,
writeWantedLockfile,
cleanGitBranchLockfiles,
PatchFile,
} from '@pnpm/lockfile-file'
import { writePnpFile } from '@pnpm/lockfile-to-pnp'
import { extendProjectsWithTargetDirs } from '@pnpm/lockfile-utils'
Expand Down Expand Up @@ -234,21 +236,30 @@ export async function mutateModules (
)
}
const packageExtensionsChecksum = isEmpty(opts.packageExtensions ?? {}) ? undefined : createObjectChecksum(opts.packageExtensions!)
const patchedDependencies = opts.ignorePackageManifest
? ctx.wantedLockfile.patchedDependencies
: (opts.patchedDependencies ? await calcPatchHashes(opts.patchedDependencies, opts.lockfileDir) : {})
const patchedDependenciesWithResolvedPath = patchedDependencies
? fromPairs(Object.entries(patchedDependencies).map(([key, patchFile]) => [key, {
hash: patchFile.hash,
path: path.join(opts.lockfileDir, patchFile.path),
}]))
: undefined
let needsFullResolution = !maybeOpts.ignorePackageManifest &&
lockfileIsUpToDate(ctx.wantedLockfile, {
overrides: opts.overrides,
neverBuiltDependencies: opts.neverBuiltDependencies,
onlyBuiltDependencies: opts.onlyBuiltDependencies,
packageExtensionsChecksum,
patchedDependencies: opts.patchedDependencies,
patchedDependencies,
}) ||
opts.fixLockfile
if (needsFullResolution) {
ctx.wantedLockfile.overrides = opts.overrides
ctx.wantedLockfile.neverBuiltDependencies = opts.neverBuiltDependencies
ctx.wantedLockfile.onlyBuiltDependencies = opts.onlyBuiltDependencies
ctx.wantedLockfile.packageExtensionsChecksum = packageExtensionsChecksum
ctx.wantedLockfile.patchedDependencies = opts.patchedDependencies
ctx.wantedLockfile.patchedDependencies = patchedDependencies
}
const frozenLockfile = opts.frozenLockfile ||
opts.frozenLockfileIfExists && ctx.existsWantedLockfile
Expand Down Expand Up @@ -296,6 +307,7 @@ export async function mutateModules (
nodeVersion: opts.nodeVersion,
pnpmVersion: opts.packageManager.name === 'pnpm' ? opts.packageManager.version : '',
},
patchedDependencies: patchedDependenciesWithResolvedPath,
projects: ctx.projects as Project[],
prunedAt: ctx.modulesFile?.prunedAt,
pruneVirtualStore,
Expand Down Expand Up @@ -479,12 +491,28 @@ export async function mutateModules (
pruneVirtualStore,
scriptsOpts,
updateLockfileMinorVersion: true,
patchedDependencies: patchedDependenciesWithResolvedPath,
})

return result.projects
}
}

async function calcPatchHashes (patches: Record<string, string>, lockfileDir: string) {
return fromPairs(await Promise.all(
Object.entries(patches).map(async ([key, patchFileRelativePath]) => {
const patchFilePath = path.join(lockfileDir, patchFileRelativePath)
return [
key,
{
hash: await createBase32HashFromFile(patchFilePath),
path: patchFileRelativePath,
},
]
})
))
}

function lockfileIsUpToDate (
lockfile: Lockfile,
{
Expand All @@ -498,7 +526,7 @@ function lockfileIsUpToDate (
onlyBuiltDependencies?: string[]
overrides?: Record<string, string>
packageExtensionsChecksum?: string
patchedDependencies?: Record<string, string>
patchedDependencies?: Record<string, PatchFile>
}) {
return !equals(lockfile.overrides ?? {}, overrides ?? {}) ||
!equals((lockfile.neverBuiltDependencies ?? []).sort(), (neverBuiltDependencies ?? []).sort()) ||
Expand Down Expand Up @@ -652,7 +680,8 @@ interface InstallFunctionResult {
type InstallFunction = (
projects: ImporterToUpdate[],
ctx: PnpmContext<DependenciesMutation>,
opts: StrictInstallOptions & {
opts: Omit<StrictInstallOptions, 'patchedDependencies'> & {
patchedDependencies?: Record<string, PatchFile>
makePartialCurrentLockfile: boolean
needsFullResolution: boolean
neverBuiltDependencies?: string[]
Expand Down
35 changes: 34 additions & 1 deletion packages/core/test/install/patch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,12 @@ test('patch package', async () => {

const patchFileHash = 'jnbpamcxayl5i4ehrkoext3any'
const lockfile = await project.readLockfile()
expect(lockfile.patchedDependencies).toStrictEqual(patchedDependencies)
expect(lockfile.patchedDependencies).toStrictEqual({
'is-positive@1.0.0': {
path: patchedDependencies['is-positive@1.0.0'],
hash: patchFileHash,
},
})
expect(lockfile.packages[`/is-positive/1.0.0_${patchFileHash}`]).toBeTruthy()

const filesIndexFile = path.join(opts.storeDir, 'files/c7/1ccf199e0fdae37aad13946b937d67bcd35fa111b84d21b3a19439cfdc2812c5d8da8a735e94c2a1ccb77b4583808ee8405313951e7146ac83ede3671dc292-index.json')
Expand Down Expand Up @@ -113,3 +118,31 @@ test('patch package throws an exception if not all patches are applied', async (
}, opts)
).rejects.toThrow('The following patches were not applied: is-negative@1.0.0')
})

test('the patched package is updated if the patch is modified', async () => {
prepareEmpty()
f.copy('patch-pkg', 'patches')
const patchPath = path.resolve('patches', 'is-positive@1.0.0.patch')

const patchedDependencies = {
'is-positive@1.0.0': path.relative(process.cwd(), patchPath),
}
const opts = await testDefaults({
fastUnpack: false,
sideEffectsCacheRead: true,
sideEffectsCacheWrite: true,
patchedDependencies,
}, {}, {}, { packageImportMethod: 'hardlink' })
const manifest = {
dependencies: {
'is-positive': '1.0.0',
},
}
await install(manifest, opts)

const patchContent = fs.readFileSync(patchPath, 'utf8')
fs.writeFileSync(patchPath, patchContent.replace('// patched', '// edited patch'), 'utf8')

await install(manifest, opts)
expect(fs.readFileSync('node_modules/is-positive/index.js', 'utf8')).toContain('// edited patch')
})
3 changes: 3 additions & 0 deletions packages/core/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
{
"path": "../core-loggers"
},
{
"path": "../crypto.base32-hash"
},
{
"path": "../dependency-path"
},
Expand Down
1 change: 0 additions & 1 deletion packages/headless/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@
"@pnpm/calc-dep-state": "workspace:3.0.0",
"@pnpm/constants": "workspace:6.1.0",
"@pnpm/core-loggers": "workspace:7.0.4",
"@pnpm/crypto.base32-hash": "workspace:1.0.0",
"@pnpm/error": "workspace:3.0.1",
"@pnpm/filter-lockfile": "workspace:6.0.7",
"@pnpm/hoist": "workspace:6.1.5",
Expand Down
2 changes: 2 additions & 0 deletions packages/headless/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
readCurrentLockfile,
readWantedLockfile,
writeCurrentLockfile,
PatchFile,
} from '@pnpm/lockfile-file'
import { writePnpFile } from '@pnpm/lockfile-to-pnp'
import {
Expand Down Expand Up @@ -111,6 +112,7 @@ export interface HeadlessOptions {
lockfileDir: string
modulesDir?: string
virtualStoreDir?: string
patchedDependencies?: Record<string, PatchFile>
scriptsPrependNodePath?: boolean | 'warn-only'
scriptShell?: string
shellEmulator?: boolean
Expand Down
23 changes: 4 additions & 19 deletions packages/headless/src/lockfileToDepGraph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { WANTED_LOCKFILE } from '@pnpm/constants'
import {
progressLogger,
} from '@pnpm/core-loggers'
import { createBase32HashFromFile } from '@pnpm/crypto.base32-hash'
import {
Lockfile,
PackageSnapshot,
Expand All @@ -16,7 +15,7 @@ import {
import logger from '@pnpm/logger'
import { IncludedDependencies } from '@pnpm/modules-yaml'
import packageIsInstallable from '@pnpm/package-is-installable'
import { Registries } from '@pnpm/types'
import { PatchFile, Registries } from '@pnpm/types'
import {
FetchPackageToStoreFunction,
PackageFilesResponse,
Expand Down Expand Up @@ -45,10 +44,7 @@ export interface DependenciesGraphNode {
prepare: boolean
hasBin: boolean
filesIndexFile: string
patchFile?: {
path: string
hash: string
}
patchFile?: PatchFile
}

export interface DependenciesGraph {
Expand All @@ -63,6 +59,7 @@ export interface LockfileToDepGraphOptions {
lockfileDir: string
nodeVersion: string
pnpmVersion: string
patchedDependencies?: Record<string, PatchFile>
registries: Registries
sideEffectsCacheRead: boolean
skipped: Set<string>
Expand Down Expand Up @@ -180,7 +177,7 @@ export default async function lockfileToDepGraph (
optionalDependencies: new Set(Object.keys(pkgSnapshot.optionalDependencies ?? {})),
prepare: pkgSnapshot.prepare === true,
requiresBuild: pkgSnapshot.requiresBuild === true,
patchFile: await tryReadPatchFile(opts.lockfileDir, lockfile, pkgName, pkgVersion),
patchFile: opts.patchedDependencies?.[`${pkgName}@${pkgVersion}`],
}
pkgSnapshotByLocation[dir] = pkgSnapshot
})
Expand Down Expand Up @@ -220,18 +217,6 @@ export default async function lockfileToDepGraph (
return { graph, directDependenciesByImporterId }
}

export async function tryReadPatchFile (lockfileDir: string, lockfile: Lockfile, pkgName: string, pkgVersion: string) {
const patchFileRelativePath = lockfile.patchedDependencies?.[`${pkgName}@${pkgVersion}`]
if (!patchFileRelativePath) {
return undefined
}
const patchFilePath = path.join(lockfileDir, patchFileRelativePath)
return {
path: patchFilePath,
hash: await createBase32HashFromFile(patchFilePath),
}
}

async function getChildrenPaths (
ctx: {
graph: DependenciesGraph
Expand Down
6 changes: 3 additions & 3 deletions packages/headless/src/lockfileToHoistedDepGraph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from '@pnpm/lockfile-utils'
import { IncludedDependencies } from '@pnpm/modules-yaml'
import packageIsInstallable from '@pnpm/package-is-installable'
import { Registries } from '@pnpm/types'
import { PatchFile, Registries } from '@pnpm/types'
import {
FetchPackageToStoreFunction,
StoreController,
Expand All @@ -23,7 +23,6 @@ import {
DepHierarchy,
DirectDependenciesByImporterId,
LockfileToDepGraphResult,
tryReadPatchFile,
} from './lockfileToDepGraph'

export interface LockfileToHoistedDepGraphOptions {
Expand All @@ -36,6 +35,7 @@ export interface LockfileToHoistedDepGraphOptions {
nodeVersion: string
pnpmVersion: string
registries: Registries
patchedDependencies?: Record<string, PatchFile>
sideEffectsCacheRead: boolean
skipped: Set<string>
storeController: StoreController
Expand Down Expand Up @@ -209,7 +209,7 @@ async function fetchDeps (
optionalDependencies: new Set(Object.keys(pkgSnapshot.optionalDependencies ?? {})),
prepare: pkgSnapshot.prepare === true,
requiresBuild: pkgSnapshot.requiresBuild === true,
patchFile: await tryReadPatchFile(opts.lockfileDir, opts.lockfile, pkgName, pkgVersion),
patchFile: opts.patchedDependencies?.[`${pkgName}@${pkgVersion}`],
}
opts.pkgLocationByDepPath[depPath] = dir
depHierarchy[dir] = await fetchDeps(opts, path.join(dir, 'node_modules'), dep.dependencies)
Expand Down
3 changes: 0 additions & 3 deletions packages/headless/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@
{
"path": "../core-loggers"
},
{
"path": "../crypto.base32-hash"
},
{
"path": "../dependency-path"
},
Expand Down
6 changes: 4 additions & 2 deletions packages/lockfile-types/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { DependenciesMeta } from '@pnpm/types'
import { DependenciesMeta, PatchFile } from '@pnpm/types'

export { PatchFile }

export interface Lockfile {
importers: Record<string, ProjectSnapshot>
Expand All @@ -8,7 +10,7 @@ export interface Lockfile {
onlyBuiltDependencies?: string[]
overrides?: Record<string, string>
packageExtensionsChecksum?: string
patchedDependencies?: Record<string, string>
patchedDependencies?: Record<string, PatchFile>
}

export interface ProjectSnapshot {
Expand Down
1 change: 0 additions & 1 deletion packages/resolve-dependencies/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
"dependencies": {
"@pnpm/constants": "workspace:6.1.0",
"@pnpm/core-loggers": "workspace:7.0.4",
"@pnpm/crypto.base32-hash": "workspace:1.0.0",
"@pnpm/error": "workspace:3.0.1",
"@pnpm/lockfile-types": "workspace:4.1.0",
"@pnpm/lockfile-utils": "workspace:4.0.6",
Expand Down
6 changes: 3 additions & 3 deletions packages/resolve-dependencies/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export default async function (

// We only check whether patches were applied in cases when the whole lockfile was reanalyzed.
if (opts.patchedDependencies && (opts.forceFullResolution || !opts.wantedLockfile.packages?.length)) {
verifyPatches(opts.patchedDependencies, appliedPatches)
verifyPatches(Object.keys(opts.patchedDependencies), appliedPatches)
}

const linkedDependenciesByProjectId: Record<string, LinkedDependency[]> = {}
Expand Down Expand Up @@ -239,8 +239,8 @@ export default async function (
}
}

function verifyPatches (patchedDependencies: Record<string, string>, appliedPatches: Set<string>) {
const nonAppliedPatches: string[] = Object.keys(patchedDependencies).filter((patchKey) => !appliedPatches.has(patchKey))
function verifyPatches (patchedDependencies: string[], appliedPatches: Set<string>) {
const nonAppliedPatches: string[] = patchedDependencies.filter((patchKey) => !appliedPatches.has(patchKey))
if (nonAppliedPatches.length) {
throw new PnpmError('PATCH_NOT_APPLIED', `The following patches were not applied: ${nonAppliedPatches.join(', ')}`, {
hint: 'Either remove them from "patchedDependencies" or update them to much packages in your dependencies.',
Expand Down
Loading