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(autoprofile): rework prebuilds #990

Closed
wants to merge 20 commits into from
Closed

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Jan 5, 2024

refs 149946

Move away from manual prebuilds for the autoprofile package to using prebuildify.

The PR replaces the prebuilds for darwin x64 and linux x64 (musl + glibc) and adds more prebuilds for darwin arm64 (m1), linux arm64 and armv6 and armv7 (raspberry pi).

This is the first iteration. The next iteration is to get rid of the manual prebuilds in the shared metrics package.

Testing / QA

Installing the autoprofiler on darwin x64 from local source without prebuild

npm i ~/dev/instana/nodejs/packages/autoprofile/instana-autoprofile-3.1.0.tgz --save --verbose
added 39 packages, and audited 40 packages in 14s

Installing the autoprofiler on darwin x64 from local source with prebuild

npm i ~/dev/instana/nodejs/packages/autoprofile/instana-autoprofile-3.1.0.tgz --save --verbose
added 39 packages, and audited 40 packages in 3s

Installing the autoprofiler on darwin x64 from npm including manual prebuilds

npm i @instana/autoprofile@latest --save
added 38 packages, and audited 39 packages in 13s

-> caused by a bug in the node-gyp-fallback script.

Installing the autoprofiler on linux x64 musl from npm including manual prebuilds

=> [3/3] RUN npm install @instana/autoprofile --save --verbose 10.8s

Installing the autoprofiler on linux x64 musl from local source with prebuild

=> [4/4] RUN npm install instana-autoprofile-3.1.0.tgz --save --verbose 6.3s

Installing the autoprofiler on linux x64 musl from local source without prebuild

=> [5/5] RUN npm install instana-autoprofile-3.1.0.tgz --save --verbose 18.3s

Installing the autoprofiler on linux x64 glibc from npm including manual prebuilds

=> [4/4] RUN npm install @instana/autoprofile --save --verbose 7.8s

Installing the autoprofiler on linux x64 glibc from local source with prebuild

=> [4/4] RUN npm install instana-autoprofile-3.1.0.tgz --save --verbose 6.5s

Installing the autoprofiler on linux x64 glibc from local source without prebuild

=> [4/4] RUN npm install instana-autoprofile-3.1.0.tgz --save --verbose 16.6s

Installing the autoprofiler on linux arm64 glibc from npm including manual prebuilds

=> [4/4] RUN npm install @instana/autoprofile --save --verbose 118.7s

Installing the autoprofiler on linux arm64 glibc from local source with prebuild

=> [4/4] RUN npm install instana-autoprofile-local-with-prebuild.tgz --save --verbose 31.1s.

Tasks

@kirrg001 kirrg001 force-pushed the feat/autoprofiler-prebuild branch 2 times, most recently from ac9c34a to 51da462 Compare January 8, 2024 12:06
@kirrg001 kirrg001 marked this pull request as ready for review January 9, 2024 15:09
@kirrg001 kirrg001 requested a review from a team as a code owner January 9, 2024 15:09
@kirrg001
Copy link
Contributor Author

kirrg001 commented Jan 9, 2024

RFR but do not merge. We need to wait for the merge & release of prebuild/prebuildify-cross#18.

@aryamohanan
Copy link
Contributor

Awesome work 🥳

packages/autoprofile/CONTRIBUTING.md Show resolved Hide resolved
packages/autoprofile/CONTRIBUTING.md Outdated Show resolved Hide resolved
const childProcess = require('child_process');

const minNodeVersion = require('../package.json').engines.node;
const excludeTargets = ['15.0.0', '17.0.0', '19.0.0'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:We are keeping a static array for excludeTargets. This array is used only if no ABI is specified. Consider adding a comment here or mentioning this information in the contributing.md file for clarity and future contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah hmmm I am wondering if we should remove the whole logic if no abi version is present.
It was helpful for the initial build, but in the future we will only build a specific abi version.

package.json Show resolved Hide resolved
@kirrg001
Copy link
Contributor Author

kirrg001 commented Feb 2, 2024

prebuildify-cross released 5.1.0. Will update 👍

@kirrg001 kirrg001 force-pushed the feat/autoprofiler-prebuild branch from 3bb4a96 to 338cf41 Compare April 29, 2024 10:20
@kirrg001 kirrg001 changed the title feat(autoprofile): added prebuilds feat(autoprofile): imrproved prebuilds Apr 29, 2024
@kirrg001 kirrg001 changed the title feat(autoprofile): imrproved prebuilds feat(autoprofile): rework prebuilds Apr 29, 2024
@kirrg001 kirrg001 mentioned this pull request Apr 29, 2024
4 tasks
@kirrg001
Copy link
Contributor Author

Closing because of wrong branch name.
#1135

@kirrg001 kirrg001 closed this Apr 29, 2024
@kirrg001 kirrg001 deleted the feat/autoprofiler-prebuild branch January 15, 2025 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants