Add cpp test, fix locale issues and fix CI issues

[seanmakesgames]

Fixes rubyjs/mini_racer#259 Unblocks rubyjs/mini_racer#271 and rubyjs/mini_racer#261 Also adds first c++ test.

Fixes #40

[seanmakesgames]

@lloeki - @Fayti1703 was able to reproduce the issue on his machine and did a bit of digging:

I have it down to something missing in icu-small (ures_open) is setting U_MISSING_RESOURCE_ERROR = 2 (/*< The requested resource cannot be found /). Not sure what exactly, nor where node is getting that data from. I have a guess -- there's an extern variable that is being macro'd together in node.cc, starting with icusmdt... but good luck figuring out where that is actually defined. I'm kinda wondering if node changed how they package the data for ICU into the binary, which means libv8-node is not pulling it with the lib anymore ...ok so turns out there's a icudt71_dat variable, which looks like it's the ICU data... no idea how exactly the node binary gets to it, its not being referred to in the source code from what I can see, but it does appear in the symbol tables for both libnode* and libv8_monolith.a, so probably just a matter of hooking it in there Might want to expose a method in libv8-node to do that so any other API consumers don't have to worry about it too much and instead just call an init method before using V8

I just noticed that full-icu is referenced in the build-libv8 script. https://github.com/rubyjs/libv8-node/blob/master/libexec/build-libv8#L26

I assume you have a bit of context here @lloeki ?

[seanmakesgames]

I have also confirmed that the issue does not reproduce in the built node executable src/node-v16.19.0/out/Release/node

[Fayti1703]

Managed to figure out what's going on with the crashing... turns out the monolith build script was including stubdata.o off of small-icu -- a file that includes an ICU data directory with zero entries (not even for the root locale!)

The compiler apparently found that object first when building dependent programs, so we were getting literally zero ICU data, which V8 isn't set up to handle.

Dropping the stubdata.o object from the libv8_monolith.a means the compiler instead finds the real data from icudt71_dat.o, which that fixes the crash issue (at least, in a simple test program).

I will note that the extra ! -path conditional to find might need some cross-OS testing, though. If it doesn't work on a specific OS, you can replace that with ar d $LIBV8_MONOLITH stubdata.o after collecting the objects.

[Fayti1703]

Something I just noticed: d742196 and 5cb6e87 conflict with 0edfea99d30e9e9b0915a8d22b6b6fa3da8df99b on master (from #31).

For the former, that's not a huge problem, it's basically a subset of the conflicting commit (though not quite because I pass -S and run ranlib; they don't) so we could just drop it, but the latter commit will need conflict resolution. Nothing too complicated, I think, but worth noting.

[lloeki]

Thanks for the investigation!

That find sure looks ugly now 😅 I've got some old attempt at implementing these shell scripts in Ruby; someday I'll complete that which should make these much more readable and consistent.

As for the conflicts, let's fix node-16 first and we'll sort that out afterwards.

[seanmakesgames]

I'm taking @Fayti1703 's test code and putting it into the test framework now. It should pass as a first case added. Additional new cases can be added easily afterwards. Then we can get back to 'the task at hand' of upgrading node/v8. :)

[lloeki]

As for the real real fix for the root cause, I presume it would entail having a true libv8_monolith.a target landed in upstream node / libv8 build system, otherwise this class of issue (either missing or extra inclusion of object files) may show up again.

[Fayti1703]

That find sure looks ugly now :sweat_smile: I've got some old attempt at implementing these shell scripts in Ruby; someday I'll complete that which should make these much more readable and consistent.

I think it's not that bad. Adding a few line continuations and indents in the find arguments would help already... plus, we could move the main part outside the case and only set variables for the system find/ar/ranlib binaries rather than writing the same thing 3 times.

Can add those changes to this PR if you're interested, @lloeki.

[Fayti1703]

As for the real real fix for the root cause, I presume it would entail having a true libv8_monolith.a target landed in upstream node / libv8 build system

Yeah, that would be the ideal solution... though we might be able to parse out the object files that are being linked into the libnode.so from make -n libnode output and determine what objects we need from there.

Plus, it looks like v8 is already bundled into a couple .a files, so we could potentially just merge those...

[lloeki]

Nah, don't sweat it, it was in jest regarding the stuff I need to get back to someday. Let's keep it simple, this is good enough to get us out of this hole.

Plus, it looks like v8 is already bundled into a couple .a files, so we could potentially just merge those...

IIRC I tried that a long time ago but it was not sufficient.

[seanmakesgames]

Got some first attempts at stuff in there. Am working on linux/darwin conditional linking now. Then onto actions integration. This is my first time with cmake (and it's been more than a few years since I wrangled compilers) ;) Let me know if you see anything you want changed.

[seanmakesgames]

Hey @lloeki -

I found this in the readme

See BUILDING.md. Also make sure to read CONTRIBUTING.md if you plan to help.

These files don't seem to exist-- did they move or something? I feel like we should throw a few things in here for our future selves.

[seanmakesgames]

@lloeki Looks like the workflows file does not use the makefile at the top level at all- is that intentional? I'm trying to figure out where to put the ctest build and run commands, and it looks like maybe the libexec script files, but they are kind of mixed?

[seanmakesgames]

Not sure why my actions are not showing up in the checks tab, but they are showing up over here for those who are curious: https://github.com/seanmakesgames/libv8-node/actions

[Fayti1703]

Not sure why my actions are not showing up in the checks tab

That tab is for actions on a PR in the base repository (i.e. rubyjs/libv8-node in this case). If this PR were ready to merge, and there were any actions defined on PRs, they'd run and show up there.

[seanmakesgames]

there were any actions defined on PRs, they'd run and show up there.

Strange though- I thought push was a wider net than pr -- since pushing to -any- branch will run the actions. In any case, the actions should pass before we merge. :D

[Fayti1703]

Quick update here: The state at 0f1dc90

• successfully completes the build jobs on x86_64 darwin and x86_64 (amd64) linux with GNU libc, including the test runs.
• fails the build job on arm64 darwin due to what I believe to be a host/target platform mismatch. If the arm64 build is a cross-compile (and it looks like it is), we'll need to skip the tests on that.
• fails the build job on arm64 linux due to -msign-return-address=all during the final link of the node build. (I'm not exactly sure why that happens, but the output looks a bit suspect)
• fails the build job on x86_64 (amd64) linux with musl libc due to some issue with the libv8_monolith.a archive. It looks like either the archive is incomplete or the ld version in use has problems references to objects inside the archive from objects within the same.
• fails all ruby test jobs because the mini_racer build looks for the architecture specific libv8 gem, despite using the ruby platform libv8 gem (e.g. libv8-node-16.19.0.0-x86_64-linux vs libv8-node-16.19.0.0). I think this is unrelated to the changes in this PR.

In any case, I would like to do a cleanup pass over the commits prior to merging them in, though I think it's best to defer that to after we have an acceptable resulting tree. I'll let @lloeki be the judge of when we're at that point.

[lloeki]

See BUILDING.md. Also make sure to read CONTRIBUTING.md if you plan to help. These files don't seem to exist-- did they move or something? I feel like we should throw a few things in here for our future selves.

Hmm, not sure. Maybe I forgot to add these. Feel free to fill some stuff in if you're keen on it!

A separate PR from this one would be good.

[lloeki]

Looks like the workflows file does not use the makefile at the top level at all- is that intentional?

Yeah, kind of.

• the Makefile is mostly a helper for local development
• the github workflow is what drives CI (no surprise here), I did not hit the Makefile to have clear separate steps and full control of docker execution within the workflow, calling out directly to libexec
• libexec is both to be used by the above and to be packaged inside the ruby platform gem for from-source building. In the latter case it is driven by stuff in ext/libv8-node/builder.rb. This builder can also be called by rake compile

So there are three "high level" build "orchestrators" currently:

• Makefile
• GH workflow
• ext/libv8-node/builder.rb (either called by rake compile or when installing the ruby platform gem)

I plan to unify and simplify these, hence my unpublished attempt to have libexec stuff be written in Ruby

[lloeki]

I'm trying to figure out where to put the ctest build and run commands, and it looks like maybe the libexec script files, but they are kind of mixed?

Hmm, good question. I don't think this should be published in the gem, so I would refrain from having it in libexec. I think a couple of shell files inside of test would be OK for now, to be called by some Makefile targets and GH workflow steps (but not ext/libv8-node/builder.rb)

(excluding the latter because e.g I don't want people to need cmake when installing from source)

[seanmakesgames]

Hey @lloeki We are very close to ready here. Can you give what we've done a good look over to let us know anything you'd like us to change here?

Additionally, do you know what is breaking the mini_racer test portions of the ci?

[Fayti1703]

Update to the previous build status:

• Success on x86_64 darwin and x86_64 linux on glibc and musl (since baa4729).
• arm64 fails due to tests (since 5d9a106, presumably fixed by 4a323db -- waiting on the build jobs to confirm)

[seanmakesgames]

@lloeki or @SamSaffron We need your help here. We've fixed most of the stuff, but there are a handful of varied failures in the tests of mini_racer's use of libv8:

https://github.com/seanmakesgames/libv8-node/actions/runs/3951363807

Maybe some of these can be ignored, because they are fixed in later branches / versions of libv8 and ruby? but I don't remember.

[SamSaffron]

Amazing! ... Ruby 2.6 is EOL. I think we can remove it from the test matrix. That would just leave Ruby 2.7 and up issues (and 2.7 is EOL pretty soon)

So mostly the issues left are around localization missing on base test VM ?

[seanmakesgames]

Amazing! ... Ruby 2.6 is EOL. I think we can remove it from the test matrix. That would just leave Ruby 2.7 and up issues (and 2.7 is EOL pretty soon)

🥳 🎉

So mostly the issues left are around localization missing on base test VM ?

When looking just at the 3.0 and 3.1 failing tests, yep!

@Fayti1703 I wonder if the ruby compile scripts are just missing some flags that you added to the other build jobs to get the languages linking up correctly. or- maybe that needs to be added into mini_racer code?

[Fayti1703]

So mostly the issues left are around localization missing on base test VM ?

@Fayti1703 I wonder if the ruby compile scripts are just missing some flags that you added to the other build jobs to get the languages linking up correctly. or- maybe that needs to be added into mini_racer code?

I believe that's a problem with the OS / container setup -- the ICU data is dynamically built using data from... somewhere whenever node itself is built.

[Fayti1703]

Something I just noticed about the ruby platform tests: They don't seem to use the ruby platform gem at all. It's compiled and installed alright, but the actual "Test with mini_racer" step then turns around and downloads libv8-node with the "correct" arch + libc from https://rubygems.org/

[Fayti1703]

(force-pushed without any changes to the resulting tree to clean up the history a bit)

[seanmakesgames]

This last commit! NICE @Fayti1703!

We are VERY green. @lloeki @SamSaffron Please take a look.

[SamSaffron]

Amazing ... give it is all green lets merge this.

What are the next steps we need here to publish a new version? Super keen to publish a new mini_racer

[lloeki]

What are the next steps we need here to publish a new version?

@SamSaffron since mini_racer version constraint on libv8-node is ~> 16.10.0.0 and this is clearly a bug fix, there is nothing to be done for node-16 on the mini_racer side, only pushing the new node 16 gems.

The typical libv8-node release process (that I should document) is:

• merge whatever PRs are wanted for a release into node-X (here node-16)
• wait for last merge commit CI to be green
• do a Release X.Y.Z.W commit on the branch (like so). It should update changelogs and whatever too.
• wait for CI to be green (it should given the changeset, but sometimes silly mistakes happen!)
• tag that release commit vX.Y.Z.W
• create a release from the tag on the GH page, and attach the CI artifacts of the release commit
• push the CI artifact gems to rubygems

(Most of these can be automated via GH workflows, e.g tag => create GH release + attack artifacts + push to rubygems†. or it can be a manual GH "dispatch workflow")

† this one can also both require and wait for 2FA with some interesting techniques

The degraded libv8-node release process is the same, except (when CI is broken because reasons outside of release blockers):

• merge whatever PRs are wanted for a release into node-X (here node-16)
• wait for last merge commit CI to be as green as possible
• do a Release X.Y.Z.W commit on the branch (like so). It should update changelogs and whatever too.
• build x86_64-{darwin,linux,linux-musl} gems on an Intel machine (resp. make test, make test/linux, make test/linux-musl)
• build arm64-darwin and aarch64-{linux,linux-musl}gems on an Intel machine (resp.make test, make test/linux, make test/linux-musl`)
• build ruby platform gem with rake build
• tag that release commit vX.Y.Z.W
• create a release from the tag on the GH page, and attach the artifacts of the release commit
• push the artifact gems to rubygems

[lloeki]

That said, the plan to update node major now should be:

• forward-port 16 into 17: update node-17 branch by cherry-pick or rebase --onto the node-16 changes we want in (please, no merge of node-16 into node-17)
• update mini_racer dependency in a branch+PR that points to node-17 PR branch
• make it work
• merge node-17 PR branch in node-17
• release libv8-node 17
• update mini_racer PR to point at libv8-node release instead of branch
• release mini_racer

Then:

• forward-port 17 into 18: update node-18 branch cherry-pick or rebase --onto these node-17 changes in
• and then basically the same as above, but sed s/17/18/g

[Fayti1703]

I'd be willing to rebase the changes here onto the node-17 branch and open a PR for them. Just need a go-ahead and a couple of minutes (most likely).

[seanmakesgames]

@Fayti1703 has created the rebase, and we are working on test stabilization now:

41

[lloeki]

FYI I'm working on getting a 16.19.0.0 gem release out.

[lloeki]

@SamSaffron since mini_racer version constraint on libv8-node is ~> 16.10.0.0 and this is clearly a bug fix

Duh I was confused, it's going to be 16.19.0.1 not 16.10.0.1. It's a bug fix from the tentative 16.19.0.0 I pushed but is not used yet by mini_racer.

Once pushed I'll open a PR for that version bump in mini_racer.

[lloeki]

Got CI fully green on the node-16 branch: https://github.com/rubyjs/libv8-node/actions/runs/4002211129

For a proper release CI is still missing aarch64-linux-musl though. Two options:

• Build and use a gcc cross-compiler (alpine has none in upstream packages). That's something I did for another project so I can port that one over
• Move all linux builds to cross-compile with clang. This would hopefully solve the warnings I get on gcc+aarch64 about a flag that got removed

I'll assess which one would be the quickest and do that. Then I can do the release.

[Fayti1703]

Another option would be to build a cross-compiled aarch64 musl-libc and using the gcc-wrapper from that on a Debian container... though I don't know how complicated that would be to set up in CI.

[lloeki]

That's a valid option although it's trumped by going the clang route (whether with a Debian base + musl sysroot or with Alpine) which is much easier to handle.

[lloeki]

Going clang may also enable libc/libc++ independent builds, see here for some of this "portable" stuff: https://github.com/DataDog/libddwaf/tree/9caeac92a11ee7d43bf5d0d1453723409ac30f3f/docker/libddwaf#portable-libddwaf-on-any-linux-26