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

feat!: remove requiresBuild from the lockfile #7710

Merged
merged 7 commits into from
Feb 27, 2024
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
5 changes: 5 additions & 0 deletions .changeset/happy-bags-reply.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@pnpm/exec.pkg-requires-build": major
---

Initial release.
5 changes: 1 addition & 4 deletions deps/graph-builder/src/lockfileToDepGraph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ export interface DependenciesGraphNode {
optional: boolean
depPath: string // this option is only needed for saving pendingBuild when running with --ignore-scripts flag
isBuilt?: boolean
requiresBuild: boolean
prepare: boolean
requiresBuild?: boolean
hasBin: boolean
filesIndexFile?: string
patchFile?: PatchFile
Expand Down Expand Up @@ -195,8 +194,6 @@ export async function lockfileToDepGraph (
name: pkgName,
optional: !!pkgSnapshot.optional,
optionalDependencies: new Set(Object.keys(pkgSnapshot.optionalDependencies ?? {})),
prepare: pkgSnapshot.prepare === true,
requiresBuild: pkgSnapshot.requiresBuild === true,
patchFile: opts.patchedDependencies?.[`${pkgName}@${pkgVersion}`],
}
pkgSnapshotByLocation[dir] = pkgSnapshot
Expand Down
1 change: 1 addition & 0 deletions env/node.fetcher/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export async function fetchNode (fetch: FetchFromRegistry, version: string, targ
filesResponse: {
filesIndex: filesIndex as Record<string, string>,
resolvedFrom: 'remote',
requiresBuild: false,
},
force: true,
})
Expand Down
7 changes: 0 additions & 7 deletions exec/files-include-install-scripts/CHANGELOG.md

This file was deleted.

15 changes: 0 additions & 15 deletions exec/files-include-install-scripts/README.md

This file was deleted.

4 changes: 0 additions & 4 deletions exec/files-include-install-scripts/src/index.ts

This file was deleted.

15 changes: 15 additions & 0 deletions exec/pkg-requires-build/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# @pnpm/exec.pkg-requires-build

> Checks if a package requires to be built

[![npm version](https://img.shields.io/npm/v/@pnpm/exec.pkg-requires-build.svg)](https://www.npmjs.com/package/@pnpm/exec.pkg-requires-build)

## Installation

```sh
pnpm add @pnpm/exec.pkg-requires-build
```

## License

MIT
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@pnpm/exec.files-include-install-scripts",
"version": "1.0.0",
"description": "Checks if a package has files indicating that it needs to be built during installation",
"name": "@pnpm/exec.pkg-requires-build",
"version": "0.0.0",
"description": "Checks if a package requires to be built",
"main": "lib/index.js",
"types": "lib/index.d.ts",
"files": [
Expand All @@ -18,7 +18,7 @@
"fix": "tslint -c tslint.json src/**/*.ts test/**/*.ts --fix",
"compile": "tsc --build && pnpm run lint --fix"
},
"repository": "https://github.com/pnpm/pnpm/blob/main/exec/files-include-install-scripts",
"repository": "https://github.com/pnpm/pnpm/blob/main/exec/pkg-requires-build",
"keywords": [
"pnpm9",
"pnpm"
Expand All @@ -27,9 +27,12 @@
"bugs": {
"url": "https://github.com/pnpm/pnpm/issues"
},
"homepage": "https://github.com/pnpm/pnpm/blob/main/exec/files-include-install-scripts#readme",
"homepage": "https://github.com/pnpm/pnpm/blob/main/exec/pkg-requires-build#readme",
"dependencies": {
"@pnpm/types": "workspace:*"
},
"devDependencies": {
"@pnpm/exec.files-include-install-scripts": "workspace:*"
"@pnpm/exec.pkg-requires-build": "workspace:*"
},
"funding": "https://opencollective.com/pnpm",
"exports": {
Expand Down
17 changes: 17 additions & 0 deletions exec/pkg-requires-build/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { type DependencyManifest } from '@pnpm/types'

export function pkgRequiresBuild (manifest: Partial<DependencyManifest> | undefined, filesIndex: Record<string, unknown>) {
return Boolean(
manifest?.scripts != null && (
Boolean(manifest.scripts.preinstall) ||
Boolean(manifest.scripts.install) ||
Boolean(manifest.scripts.postinstall)
) ||
filesIncludeInstallScripts(filesIndex)
)
}

function filesIncludeInstallScripts (filesIndex: Record<string, unknown>): boolean {
return filesIndex['binding.gyp'] != null ||
Object.keys(filesIndex).some((filename) => !(filename.match(/^[.]hooks[\\/]/) == null)) // TODO: optimize this
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
"src/**/*.ts",
"../../__typings__/**/*.d.ts"
],
"references": [],
"references": [
{
"path": "../../packages/types"
}
],
"composite": true
}
1 change: 1 addition & 0 deletions fetching/directory-fetcher/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"@pnpm/logger": "^5.0.0"
},
"dependencies": {
"@pnpm/exec.pkg-requires-build": "workspace:*",
"@pnpm/fetcher-base": "workspace:*",
"@pnpm/fs.packlist": "workspace:*",
"@pnpm/read-project-manifest": "workspace:*",
Expand Down
41 changes: 18 additions & 23 deletions fetching/directory-fetcher/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { promises as fs, type Stats } from 'fs'
import path from 'path'
import { pkgRequiresBuild } from '@pnpm/exec.pkg-requires-build'
import type { DirectoryFetcher, DirectoryFetcherOptions } from '@pnpm/fetcher-base'
import { logger } from '@pnpm/logger'
import { packlist } from '@pnpm/fs.packlist'
Expand All @@ -21,7 +22,7 @@ export function createDirectoryFetcher (

const directoryFetcher: DirectoryFetcher = (cafs, resolution, opts) => {
const dir = path.join(opts.lockfileDir, resolution.directory)
return fetchFromDir(dir, opts)
return fetchFromDir(dir)
}

return {
Expand All @@ -36,30 +37,28 @@ export async function fetchFromDir (
opts: FetchFromDirOpts & CreateDirectoryFetcherOptions
) {
if (opts.includeOnlyPackageFiles) {
return fetchPackageFilesFromDir(dir, opts)
return fetchPackageFilesFromDir(dir)
}
const readFileStat: ReadFileStat = opts?.resolveSymlinks === true ? realFileStat : fileStat
return fetchAllFilesFromDir(readFileStat, dir, opts)
return fetchAllFilesFromDir(readFileStat, dir)
}

async function fetchAllFilesFromDir (
readFileStat: ReadFileStat,
dir: string,
opts: FetchFromDirOpts
dir: string
) {
const filesIndex = await _fetchAllFilesFromDir(readFileStat, dir)
let manifest: DependencyManifest | undefined
if (opts.readManifest) {
// In a regular pnpm workspace it will probably never happen that a dependency has no package.json file.
// Safe read was added to support the Bit workspace in which the components have no package.json files.
// Related PR in Bit: https://github.com/teambit/bit/pull/5251
manifest = await safeReadProjectManifestOnly(dir) as DependencyManifest ?? undefined
}
// In a regular pnpm workspace it will probably never happen that a dependency has no package.json file.
// Safe read was added to support the Bit workspace in which the components have no package.json files.
// Related PR in Bit: https://github.com/teambit/bit/pull/5251
const manifest = await safeReadProjectManifestOnly(dir) as DependencyManifest ?? undefined
const requiresBuild = pkgRequiresBuild(manifest, filesIndex)
return {
local: true as const,
filesIndex,
packageImportMethod: 'hardlink' as const,
manifest,
requiresBuild,
}
}

Expand Down Expand Up @@ -124,23 +123,19 @@ async function fileStat (filePath: string): Promise<{ filePath: string, stat: St
}
}

async function fetchPackageFilesFromDir (
dir: string,
opts: FetchFromDirOpts
) {
async function fetchPackageFilesFromDir (dir: string) {
const files = await packlist(dir)
const filesIndex: Record<string, string> = Object.fromEntries(files.map((file) => [file, path.join(dir, file)]))
let manifest: DependencyManifest | undefined
if (opts.readManifest) {
// In a regular pnpm workspace it will probably never happen that a dependency has no package.json file.
// Safe read was added to support the Bit workspace in which the components have no package.json files.
// Related PR in Bit: https://github.com/teambit/bit/pull/5251
manifest = await safeReadProjectManifestOnly(dir) as DependencyManifest ?? undefined
}
// In a regular pnpm workspace it will probably never happen that a dependency has no package.json file.
// Safe read was added to support the Bit workspace in which the components have no package.json files.
// Related PR in Bit: https://github.com/teambit/bit/pull/5251
const manifest = await safeReadProjectManifestOnly(dir) as DependencyManifest ?? undefined
const requiresBuild = pkgRequiresBuild(manifest, filesIndex)
return {
local: true as const,
filesIndex,
packageImportMethod: 'hardlink' as const,
manifest,
requiresBuild,
}
}
3 changes: 3 additions & 0 deletions fetching/directory-fetcher/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
{
"path": "../../__utils__/test-fixtures"
},
{
"path": "../../exec/pkg-requires-build"
},
{
"path": "../../fs/packlist"
},
Expand Down
10 changes: 9 additions & 1 deletion fetching/fetcher-base/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export interface FetchResult {
local?: boolean
manifest?: DependencyManifest
filesIndex: Record<string, string>
requiresBuild: boolean
}

export interface GitFetcherOptions {
Expand All @@ -34,7 +35,13 @@ export interface GitFetcherOptions {
pkg?: PkgNameVersion
}

export type GitFetcher = FetchFunction<GitResolution, GitFetcherOptions, { filesIndex: Record<string, string>, manifest?: DependencyManifest }>
export interface GitFetcherResult {
filesIndex: Record<string, string>
manifest?: DependencyManifest
requiresBuild: boolean
}

export type GitFetcher = FetchFunction<GitResolution, GitFetcherOptions, GitFetcherResult>

export interface DirectoryFetcherOptions {
lockfileDir: string
Expand All @@ -46,6 +53,7 @@ export interface DirectoryFetcherResult {
filesIndex: Record<string, string>
packageImportMethod: 'hardlink'
manifest?: DependencyManifest
requiresBuild: boolean
}

export type DirectoryFetcher = FetchFunction<DirectoryResolution, DirectoryFetcherOptions, DirectoryFetcherResult>
Expand Down
4 changes: 3 additions & 1 deletion fetching/tarball-fetcher/src/gitHostedTarballFetcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export interface CreateGitHostedTarballFetcher {
export function createGitHostedTarballFetcher (fetchRemoteTarball: FetchFunction, fetcherOpts: CreateGitHostedTarballFetcher): FetchFunction {
const fetch = async (cafs: Cafs, resolution: Resolution, opts: FetchOptions) => {
const tempIndexFile = pathTemp(opts.filesIndexFile)
const { filesIndex, manifest } = await fetchRemoteTarball(cafs, resolution, {
const { filesIndex, manifest, requiresBuild } = await fetchRemoteTarball(cafs, resolution, {
...opts,
filesIndexFile: tempIndexFile,
})
Expand All @@ -37,6 +37,7 @@ export function createGitHostedTarballFetcher (fetchRemoteTarball: FetchFunction
return {
filesIndex: prepareResult.filesIndex,
manifest: prepareResult.manifest ?? manifest,
requiresBuild,
}
} catch (err: any) { // eslint-disable-line
err.message = `Failed to prepare git-hosted package fetched from "${resolution.tarball}": ${err.message}`
Expand Down Expand Up @@ -67,6 +68,7 @@ async function prepareGitHostedPkg (
filesResponse: {
filesIndex,
resolvedFrom: 'remote',
requiresBuild: false,
},
force: true,
})
Expand Down
10 changes: 0 additions & 10 deletions lockfile/lockfile-file/src/lockfileFormatConverters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,6 @@ function normalizeLockfile (lockfile: InlineSpecifiersLockfile, opts: NormalizeL
if ((lockfileToSave.patchedDependencies != null) && isEmpty(lockfileToSave.patchedDependencies)) {
delete lockfileToSave.patchedDependencies
}
if (lockfileToSave.neverBuiltDependencies != null) {
if (isEmpty(lockfileToSave.neverBuiltDependencies)) {
delete lockfileToSave.neverBuiltDependencies
} else {
lockfileToSave.neverBuiltDependencies = lockfileToSave.neverBuiltDependencies.sort()
}
}
if (lockfileToSave.onlyBuiltDependencies != null) {
lockfileToSave.onlyBuiltDependencies = lockfileToSave.onlyBuiltDependencies.sort()
}
if (!lockfileToSave.packageExtensionsChecksum) {
delete lockfileToSave.packageExtensionsChecksum
}
Expand Down
2 changes: 0 additions & 2 deletions lockfile/lockfile-file/src/sortLockfileKeys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ type RootKey = keyof LockfileFile
const ROOT_KEYS: readonly RootKey[] = [
'lockfileVersion',
'settings',
'neverBuiltDependencies',
'onlyBuiltDependencies',
'overrides',
'packageExtensionsChecksum',
'pnpmfileChecksum',
Expand Down
4 changes: 0 additions & 4 deletions lockfile/lockfile-file/test/normalizeLockfile.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ import { convertToLockfileFile } from '../lib/lockfileFormatConverters'
test('empty overrides and neverBuiltDependencies are removed during lockfile normalization', () => {
expect(convertToLockfileFile({
lockfileVersion: LOCKFILE_VERSION,
// but this should be preserved.
onlyBuiltDependencies: [],
overrides: {},
neverBuiltDependencies: [],
patchedDependencies: {},
packages: {},
importers: {
Expand All @@ -24,7 +21,6 @@ test('empty overrides and neverBuiltDependencies are removed during lockfile nor
forceSharedFormat: false,
})).toStrictEqual({
lockfileVersion: LOCKFILE_VERSION,
onlyBuiltDependencies: [],
importers: {
foo: {
dependencies: {
Expand Down
4 changes: 0 additions & 4 deletions lockfile/lockfile-types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ export interface Lockfile {
lockfileVersion: number | string
time?: Record<string, string>
packages?: PackageSnapshots
neverBuiltDependencies?: string[]
onlyBuiltDependencies?: string[]
overrides?: Record<string, string>
packageExtensionsChecksum?: string
patchedDependencies?: Record<string, PatchFile>
Expand Down Expand Up @@ -77,9 +75,7 @@ export interface PackageSnapshot {
id?: string
dev?: true | false
optional?: true
requiresBuild?: true
patched?: true
prepare?: true
hasBin?: true
// name and version are only needed
// for packages that are hosted not in the npm registry
Expand Down
Loading
Loading