Fix undefined symbol CpuProfilingLoggingMode on Node 12.0 -> 12.15

[Widdershin]

Applies the suggested change from #27 to only include the CpuProfilingLoggingMode on Node 12.16+.

There is a problem though, which is that node-pre-gyp does not expose the Node version for substitution in the binary field in the package.json. The major/minor/patch mentioned in #27 correspond to the module version unfortunately.

I have a proposed fix, which is to wrap node-pre-gyp, and change the binary host mirror path depending on the Node version. This means that Node v12.0 -> v12.15 will attempt to download from https://github.com/hyj1991/v8-profiler-node8/releases/download/node-12-missing-symbol.

This stops the impacted versions from getting the wrong binary, and will currently cause them to fallback to build.

I wasn't sure what your process for publishing binaries is, but do you think it could be adapted to do a special build for Node 12.15 that is published to the node-12-missing-symbol subdirectory?

If you let me know what the process is I am happy to do all the grunt work. If we can do that all versions of Node will be able to install a prebuilt binary with the correct code.

[hyj1991]

Sorry, I have been busy these days, maybe I'll review it this weekend

[Widdershin]

Thanks, didn’t mean to rush you, had just forgotten to request the review :)

…

On Wed, 18 Nov 2020 at 2:38 PM, hyj1991 ***@***.***> wrote: Sorry, I have been busy these days, maybe I'll review it this weekend — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#28 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBIHLUFZJ2SSG4NOJKOH3SQMQQTANCNFSM4TYBNBHQ> .

[Widdershin]

Rebased and resolved conflicts.

This PR is fairly hacky, but I'm still not sure I can think of a better solution. Feel free to close it if it's not to your taste.

I'm working around this in my project by installing an older version of v8-profiler-node8 if the user is on 12.0-12.15, but since we now know that has a memory issue, I'll probably just recommend people avoid those versions of Node.

[hyj1991]

Maybe we can avoid this problem by not providing the pre-compiled binary of Node-v12.x.

[hyj1991]

Maintaining pre-compiled binary in this way seems will cause more maintenance problems.

[hyj1991]

In fact, I did not provide node-v12.x pre-compiled binary at v7.2.1@next.

[Widdershin]

That's probably the easiest fix, although I've found the pre-compiled binaries are extremely helpful, especially for environments like AWS Lambda or Azure App Services where the environment doesn't come with the toolchain for building native plugins.

I think as it stands this PR would cause it compile on Node 12.0-12.5, but would use a published binary for later versions, so we could keep shipping 12.x binaries.

The extra step of creating another binary for the impacted versions and placing it in /node-12-missing-symbol is optional, and I can see how that's a maintenance hassle.

I can always recommend that anyone who has issues with binary compilation use 14.x onwards, so I'm happy with any of the options really :)

[Widdershin]

Also just a heads up but v7.x-staging doesn't currently build on Node 12.0-12.15, needs the changes to cpu_profiler.cc from this PR.

[hyj1991]

And I also want to know will the modification of the install script affects the rest of the versions? If not, it seems we can use this for 12.x.

[Widdershin]

@hyj1991 it should just act as pass-through to node-pre-gyp, it will just set the npm_config_profiler_binary_host_mirror env var if we're on 12.0-12.15. So shouldn't have any impact on other versions.

[hyj1991]

+1

[hyj1991]

Another thing is that we may be not able to put the pre-compiled binaries to the host:

https://github.com/hyj1991/v8-profiler-node8/releases/download/node-12-missing-symbol/xxxx

Actually, the automatically generated address of the GitHub release page is:

https://github.com/hyj1991/v8-profiler-node8/releases/download/xxxxx

[Widdershin]

Ah, good to know. Let’s just not build those special case binaries. It should still trigger a build from source when it can’t find the binary for 12.0-12.15, and we can just recommend using a latest LTS for binaries

…

On Wed, 30 Jun 2021 at 13:27, hyj1991 ***@***.***> wrote: Another thing is that we may be not able to put the pre-compiled binaries to the host https://github.com/hyj1991/v8-profiler-node8/releases/download/node-12-missing-symbol/xxxx, actually the automatically generated address of the GitHub release page is https://github.com/hyj1991/v8-profiler-node8/releases/download/xxxxx — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#28 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBIHLWIHHFVZVS7VMAOELTVJXHRANCNFSM4TYBNBHQ> .