Add cpp test, fix locale issues and fix CI issues #37

seanmakesgames · 2023-01-09T04:12:44Z

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

Fixes #40

Use `xargs` to build the `ar` command line instead of running `ar` manually to reduce startup costs. Build the symbol table after adding each file instead of continually rebuilding it.

seanmakesgames · 2023-01-09T04:37:45Z

@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 · 2023-01-09T04:42:09Z

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

Fayti1703 · 2023-01-09T21:41:13Z

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 · 2023-01-11T09:39:39Z

Something I just noticed: d742196 and 5cb6e87 conflict with 0edfea9 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 · 2023-01-11T16:23:46Z

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.

lib/libv8/node/version.rb

seanmakesgames · 2023-01-11T16:29:19Z

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 · 2023-01-11T16:32:18Z

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 · 2023-01-11T16:56:37Z

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.

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 · 2023-01-11T17:16:10Z

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 · 2023-01-11T20:18:07Z

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 · 2023-01-13T07:01:54Z

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 · 2023-01-13T16:12:23Z

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 · 2023-01-15T16:32:54Z

@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 · 2023-01-15T17:07:36Z

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 · 2023-01-15T17:23:42Z

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 · 2023-01-15T17:27:02Z

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 · 2023-01-17T09:25:48Z

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.

seanmakesgames · 2023-01-17T23:54:49Z

test/gtest/CMakeLists.txt

+)
+
+string(TOLOWER ${CMAKE_SYSTEM_NAME} system_name)
+# FIXME?: Pure Windows will error on the following command.


Windows is not in our support matrix anywhere
https://github.com/rubyjs/libv8-node#do-i-get-a-binary

lloeki · 2023-01-18T14:25:42Z

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 · 2023-01-18T14:35:08Z

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 · 2023-01-18T14:40:02Z

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 · 2023-01-18T16:49:58Z

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 · 2023-01-18T18:59:28Z

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)

and one that supports the newer cmake.

Alpine Linux's `ar` program doesn't support having objects with the same name in the archive twice, so we have to use the GNU extension for including paths. This commit *should* honestly be split into multiple separate commits, but unfortunately pretty much all of these changes have to be applied as a unit or a build step fails.

Fayti1703 · 2023-01-20T18:20:07Z

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

seanmakesgames · 2023-01-22T16:59:44Z

This last commit! NICE @Fayti1703!

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

SamSaffron · 2023-01-23T01:03:59Z

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 · 2023-01-23T10:44:43Z

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 · 2023-01-23T10:51:29Z

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 · 2023-01-23T19:10:20Z

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 · 2023-01-23T20:34:15Z

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

lloeki · 2023-01-24T15:47:54Z

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

lloeki · 2023-01-24T15:49:51Z

@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 · 2023-01-25T11:53:43Z

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 · 2023-01-25T12:27:10Z

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 · 2023-01-25T12:55:51Z

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 · 2023-01-25T12:58:46Z

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

Optimize 'libexec/build-monolith' on Linux

d742196

Use `xargs` to build the `ar` command line instead of running `ar` manually to reduce startup costs. Build the symbol table after adding each file instead of continually rebuilding it.

lloeki reviewed Jan 11, 2023

View reviewed changes

lib/libv8/node/version.rb Outdated Show resolved Hide resolved

seanmakesgames marked this pull request as ready for review January 15, 2023 16:37

seanmakesgames force-pushed the add_test branch from 9f41537 to 7849fff Compare January 15, 2023 16:43

seanmakesgames marked this pull request as draft January 15, 2023 17:06

seanmakesgames commented Jan 17, 2023

View reviewed changes

seanmakesgames marked this pull request as ready for review January 18, 2023 16:50

seanmakesgames mentioned this pull request Jan 18, 2023

Workflows do not use any artifacts in their later steps #39

Open

Fayti1703 and others added 7 commits January 20, 2023 19:10

CI: Install CMake in docker container

0248883

attempt to move forward docker image to newer version

02f6af3

and one that supports the newer cmake.

Auto-detect vendor dir in GTest CMake Configuration

fd333f5

Fix aarch64 cross-compile by dropping -msign-return-address on host

b1af510

skip arm64 c test on mac and lin

29c7341

2.6 is EOL -- remove and bump

9a61bac

Fayti1703 force-pushed the add_test branch from 48bbc9b to 9a61bac Compare January 20, 2023 18:10

Force ruby platform when testing ruby gem

97560d6

seanmakesgames changed the title ~~Add test~~ Add Cpp test, fix locale issues and fix CI issues Jan 22, 2023

seanmakesgames changed the title ~~Add Cpp test, fix locale issues and fix CI issues~~ Add cpp test, fix locale issues and fix CI issues Jan 22, 2023

seanmakesgames mentioned this pull request Jan 22, 2023

Built v8 is not being used in ruby platform tests. #40

Closed

SamSaffron merged commit 543ee88 into rubyjs:node-16 Jan 23, 2023

seanmakesgames deleted the add_test branch January 23, 2023 20:34

This was referenced Jan 23, 2023

Update readme with publishing new release steps #42

Open

Refactor test compilation code into correct script location #43

Open

Create building and contributing md files. #44

Open

Add cpp test, fix locale issues and fix CI issues #37

Add cpp test, fix locale issues and fix CI issues #37

Conversation

seanmakesgames commented Jan 9, 2023 • edited Loading

seanmakesgames commented Jan 9, 2023

seanmakesgames commented Jan 9, 2023

Fayti1703 commented Jan 9, 2023 • edited Loading

Fayti1703 commented Jan 11, 2023 • edited Loading

lloeki commented Jan 11, 2023

seanmakesgames commented Jan 11, 2023

lloeki commented Jan 11, 2023

Fayti1703 commented Jan 11, 2023

Fayti1703 commented Jan 11, 2023

lloeki commented Jan 11, 2023

seanmakesgames commented Jan 13, 2023

seanmakesgames commented Jan 13, 2023 • edited Loading

seanmakesgames commented Jan 15, 2023

seanmakesgames commented Jan 15, 2023

Fayti1703 commented Jan 15, 2023

seanmakesgames commented Jan 15, 2023 • edited Loading

Fayti1703 commented Jan 17, 2023 • edited Loading

seanmakesgames Jan 17, 2023

Choose a reason for hiding this comment

lloeki commented Jan 18, 2023 • edited Loading

lloeki commented Jan 18, 2023

lloeki commented Jan 18, 2023 • edited Loading

seanmakesgames commented Jan 18, 2023

Fayti1703 commented Jan 18, 2023

Fayti1703 commented Jan 20, 2023

seanmakesgames commented Jan 22, 2023

SamSaffron commented Jan 23, 2023

lloeki commented Jan 23, 2023 • edited Loading

lloeki commented Jan 23, 2023 • edited Loading

Fayti1703 commented Jan 23, 2023

seanmakesgames commented Jan 23, 2023

lloeki commented Jan 24, 2023

lloeki commented Jan 24, 2023

lloeki commented Jan 25, 2023

Fayti1703 commented Jan 25, 2023

lloeki commented Jan 25, 2023

lloeki commented Jan 25, 2023

seanmakesgames commented Jan 9, 2023 •

edited

Loading

Fayti1703 commented Jan 9, 2023 •

edited

Loading

Fayti1703 commented Jan 11, 2023 •

edited

Loading

seanmakesgames commented Jan 13, 2023 •

edited

Loading

seanmakesgames commented Jan 15, 2023 •

edited

Loading

Fayti1703 commented Jan 17, 2023 •

edited

Loading

lloeki commented Jan 18, 2023 •

edited

Loading

lloeki commented Jan 18, 2023 •

edited

Loading

lloeki commented Jan 23, 2023 •

edited

Loading

lloeki commented Jan 23, 2023 •

edited

Loading