-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Upgrade to version 2 of detect-libc #7694
Conversation
|
79171c3
to
f133192
Compare
let {family, version} = require('detect-libc'); | ||
if (family === 'glibc' && parseFloat(version) <= 2.17) { | ||
let {GLIBC, family, version} = require('detect-libc'); | ||
if ((await family()) === GLIBC && parseFloat(await version()) <= 2.17) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider
let [_family, _version] = await Promise.all([family(), version()]);
// ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. For those on Node.js 12+, which I think will be all parcel users, the call to family()
is very fast but the call to version()
will still involve spawning a child process, a small I/O overhead. Keeping these separate allows the call to the former to possibly shortcut the need to call the latter. That's bit of a micro-optimisation though, so I'm happy to switch to your suggestion for more readable code if you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 makes sense
We don't run on old Centos to test the segfault issue with the ImageOptimizer, so I don't think that codepath is taken. Probably worth a manual test to confirm it functions as before. @devongovett once we get CI stable, it may be worth considering running the integration tests on a system with old glibc too. |
Uses the Report API where available (Node.js 12+)
f133192
to
75ed27c
Compare
I've rebased to remove the conflict and I think the one open conversation can now be resolved. Does the "Waiting" status mean you're waiting for me to do something? Please do let me know if there's anything else that's required, thank you. |
Closing as no longer required since #8838 |
↪️ Pull Request
Hi, the latest
detect-libc
has been modernised to use the non-blocking Node.js Report API where available (Node.js >= 12), falling back to a more efficient single child process only when needed.It's a major version bump for
detect-libc
as it drops support for versions of Node.js < 8 butparcel
already requires >= 12 so this change is OK to include in a patch release.https://www.npmjs.com/package/detect-libc
💻 Examples
Should anyone be interested in the background to this, please see lovell/detect-libc#14
🚨 Test instructions
Hopefully existing unit tests in this repo cover its use.
detect-libc
has an extensive integration test suite, e.g. https://github.com/lovell/detect-libc/actions/runs/1717414863✔️ PR Todo