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.

Sign up for GitHub

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(current-env): freezing when calling current env #122

Closed
wants to merge 1 commit into from
Closed

fix(current-env): freezing when calling current env #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 4 additions & 21 deletions lib/current-env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
const process = require('node:process')
const nodeOs = require('node:os')

function isMusl (file) {
return file.includes('libc.musl-') || file.includes('ld-musl-')
}
const { familySync } = require('detect-libc')
Copy link
Member

Choose a reason for hiding this comment

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

This spawns out to the shell to do its detection. I don't know if we want to add that to our security footprint at this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only in very rare cases, if ever (because it's only on linux as a fallback to reading the ldd file and the getReport)
https://github.com/lovell/detect-libc/blame/2642d9612fadf14f3eeb5cd56eb3a63b254c7693/lib/detect-libc.js#L144-L154

But if it's a problem we can just take the read from ldd file logic which will solve 99% of problems

Copy link
Member

Choose a reason for hiding this comment

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

I think the ldd file logic is likely the way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll put it in my PR then, could you explain how do you bypass the cache with a separate file and a mock or give an example?

Copy link
Member

Choose a reason for hiding this comment

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

This PR already has some of it, albeit by using requiremock. We use tap so we'd just want to use tap.mock.

I'd say current-env is already sufficiently isolated to be ready for mocking.

Perhaps mocking isn't even the right option here. The require cache is the problem. cleaning it as a t.afterEach may be sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

delete require.cache[require.resolve(module)]


function os () {
return process.platform
Expand All @@ -13,22 +10,8 @@ function cpu () {
return process.arch
}

function libc (osName) {
// this is to make it faster on non linux machines
if (osName !== 'linux') {
return undefined
}
let family
const originalExclude = process.report.excludeNetwork
process.report.excludeNetwork = true
const report = process.report.getReport()
process.report.excludeNetwork = originalExclude
if (report.header?.glibcVersionRuntime) {
family = 'glibc'
} else if (Array.isArray(report.sharedObjects) && report.sharedObjects.some(isMusl)) {
family = 'musl'
}
return family
function libc () {
return familySync() || undefined
Copy link
Contributor

@Tofandel Tofandel Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you cache locally the result of familySync?

As it can still be a not so fast call (blocking file read operation or blocking get report) and it's done for each dependency in the tree, so it should be cached because it will never change

Copy link
Contributor

Choose a reason for hiding this comment

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

NVM, it seems each subFunction is cached in detect libc, like getReport or getFamily from fs

}

function devEngines (env = {}) {
Expand All @@ -38,7 +21,7 @@ function devEngines (env = {}) {
name: env.cpu || cpu(),
},
libc: {
name: env.libc || libc(osName),
name: env.libc || libc(),
},
os: {
name: osName,
Expand Down
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
"description": "Check the engines and platform fields in package.json",
"main": "lib/index.js",
"dependencies": {
"detect-libc": "^2.0.3",
"semver": "^7.1.1"
},
"devDependencies": {
"@npmcli/eslint-config": "^5.0.0",
"@npmcli/template-oss": "4.23.3",
"rewiremock": "^3.14.5",
"tap": "^16.0.1"
},
"scripts": {
Expand Down
95 changes: 39 additions & 56 deletions test/check-platform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
const t = require('tap')
const rewiremock = require('rewiremock/node')
Copy link
Member

Choose a reason for hiding this comment

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

Tap has this built in as t.mock

rewiremock.overrideEntryPoint(module)

const { checkPlatform } = require('..')

t.test('target cpu wrong', async t =>
Expand Down Expand Up @@ -94,75 +97,55 @@ t.test('wrong libc with overridden libc', async t =>
}), { code: 'EBADPLATFORM' }))

t.test('libc', (t) => {
let PLATFORM = ''

const _processPlatform = Object.getOwnPropertyDescriptor(process, 'platform')
Object.defineProperty(process, 'platform', {
enumerable: true,
configurable: true,
get: () => PLATFORM,
})

let REPORT = {}
const _processReport = process.report.getReport
process.report.getReport = () => REPORT

t.teardown(() => {
Object.defineProperty(process, 'platform', _processPlatform)
process.report.getReport = _processReport
})

t.test('fails when not in linux', (t) => {
PLATFORM = 'darwin'

t.throws(() => checkPlatform({ libc: 'glibc' }), { code: 'EBADPLATFORM' },
const checkPlatformProxy = rewiremock.proxy(() => require('../'), {
'detect-libc': {
familySync: () => undefined,
},
'node:process': {
platform: 'darwin',
},
})

t.throws(() => checkPlatformProxy.checkPlatform({ libc: 'glibc' }), { code: 'EBADPLATFORM' },
'fails for glibc when not in linux')
t.throws(() => checkPlatform({ libc: 'musl' }), { code: 'EBADPLATFORM' },
t.throws(() => checkPlatformProxy.checkPlatform({ libc: 'musl' }), { code: 'EBADPLATFORM' },
'fails for musl when not in linux')
t.end()
})

t.test('glibc', (t) => {
PLATFORM = 'linux'

REPORT = {}
t.throws(() => checkPlatform({ libc: 'glibc' }), { code: 'EBADPLATFORM' },
'fails when report is missing header property')

REPORT = { header: {} }
t.throws(() => checkPlatform({ libc: 'glibc' }), { code: 'EBADPLATFORM' },
'fails when header is missing glibcVersionRuntime property')

REPORT = { header: { glibcVersionRuntime: '1' } }
t.doesNotThrow(() => checkPlatform({ libc: 'glibc' }), 'allows glibc on glibc')
t.throws(() => checkPlatform({ libc: 'musl' }), { code: 'EBADPLATFORM' },
const checkPlatformProxy = rewiremock.proxy(() => require('../'), {
'detect-libc': {
familySync: () => 'glibc',
},
'node:process': {
platform: 'linux',
},
})

t.doesNotThrow(
() => checkPlatformProxy.checkPlatform({ libc: 'glibc' }),
'allows glibc on glibc'
)
t.throws(() => checkPlatformProxy.checkPlatform({ libc: 'musl' }), { code: 'EBADPLATFORM' },
'does not allow musl on glibc')

t.end()
})

t.test('musl', (t) => {
PLATFORM = 'linux'

REPORT = {}
t.throws(() => checkPlatform({ libc: 'musl' }), { code: 'EBADPLATFORM' },
'fails when report is missing sharedObjects property')

REPORT = { sharedObjects: {} }
t.throws(() => checkPlatform({ libc: 'musl' }), { code: 'EBADPLATFORM' },
'fails when sharedObjects property is not an array')

REPORT = { sharedObjects: [] }
t.throws(() => checkPlatform({ libc: 'musl' }), { code: 'EBADPLATFORM' },
'fails when sharedObjects does not contain musl')

REPORT = { sharedObjects: ['ld-musl-foo'] }
t.doesNotThrow(() => checkPlatform({ libc: 'musl' }), 'allows musl on musl as ld-musl-')

REPORT = { sharedObjects: ['libc.musl-'] }
t.doesNotThrow(() => checkPlatform({ libc: 'musl' }), 'allows musl on musl as libc.musl-')

t.throws(() => checkPlatform({ libc: 'glibc' }), { code: 'EBADPLATFORM' },
const checkPlatformProxy = rewiremock.proxy(() => require('../'), {
'detect-libc': {
familySync: () => 'musl',
},
'node:process': {
platform: 'linux',
},
})

t.doesNotThrow(() => checkPlatformProxy.checkPlatform({ libc: 'musl' }), 'allows musl on musl')
t.throws(() => checkPlatformProxy.checkPlatform({ libc: 'glibc' }), { code: 'EBADPLATFORM' },
'does not allow glibc on musl')

t.end()
Expand Down