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

Fix Libv8::Node::Paths with RubyGems >=3.3.21 on *-linux-gnu platforms #36

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

knu
Copy link

@knu knu commented Oct 29, 2022

RubyGems 3.3.21 changed the value format of Gem::Platform.local for Ruby configured with *-linux-gnu as the platform name.

$ ruby -ve 'p Gem::VERSION; p Gem::Platform.local.to_s'
ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-linux-gnu]
"3.3.20"
"x86_64-linux"

$ ruby -ve 'p Gem::VERSION; p Gem::Platform.local.to_s'
ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-linux-gnu]
"3.3.21"
"x86_64-linux-gnu"

$ ruby -ve 'p Gem::VERSION; p Gem::Platform.local.to_s'
ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-linux-gnu]
"3.3.24"
"x86_64-linux-gnu"

As you can see, it now has the -gnu prefix and Libv8::Node::Paths.object_paths thus has it when the built binary actually lives in the directory x86_64-linux, breaking the build of the gems like mini_racer on those platforms.

RUBY_TARGET_PLATFORM is set to *-linux, so Libv8::Node::Paths.platform needs to align with that.

lloeki and others added 6 commits January 8, 2023 16:04
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.
* Running libv8 on GraalVM LLVM is unlikely to ever work, instead using
  Graal.js makes more sense. This is already the case in mini_racer.
  libv8-node is a necessary dependency of mini_racer on CRuby, so for
  TruffleRuby the extconf.rb simply creates a dummy Makefile.
Skip compilation on TruffleRuby
This fixes an issue where functions that require ICU data segfault
the process due to an unexpected lack of ICU data.

The actual ICU data is provided by the icudt71_dat object.
@brevilo
Copy link

brevilo commented Mar 18, 2024

Just ran into the same issue. What's holding back a merge?

@lloeki
Copy link
Collaborator

lloeki commented Apr 9, 2024

What's holding back a merge?

Small bits of time I have available are spent on the catch up plan: rubyjs/mini_racer#277

I'll eventually get back to this. It might also be linked to rubyjs/mini_racer#284 (comment) as the *-linux packages should really be *-linux-gnu instead.

@MaZderMind
Copy link

This breaks install of mini_racer on Ubuntu 24.04 shipped ruby:

» docker run -it ubuntu:24.04 bash
root@98484a7e79bb:/# apt update && apt install -y ruby ruby-dev build-essential
root@98484a7e79bb:/# gem install mini_racer
…
compiling mini_racer_extension.cc
linking shared-object mini_racer_extension.so
/usr/bin/ld: cannot find /var/lib/gems/3.2.0/gems/libv8-node-21.7.2.0-x86_64-linux/vendor/v8/x86_64-linux-gnu/libv8/obj/libv8_monolith.a: No such file or directory

@SamSaffron
Copy link
Collaborator

@knu can you rebase, I really would love to release a gem with this fix, being broken on brand new ruby is a problem @lloeki

RubyGems 3.3.21 changed the value format of `Gem::Platform.local` for Ruby configured with `*-linux-gnu` as the platform name.

```console
$ ruby -ve 'p Gem::VERSION; p Gem::Platform.local.to_s'
ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-linux-gnu]
"3.3.20"
"x86_64-linux"

$ ruby -ve 'p Gem::VERSION; p Gem::Platform.local.to_s'
ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-linux-gnu]
"3.3.21"
"x86_64-linux-gnu"

$ ruby -ve 'p Gem::VERSION; p Gem::Platform.local.to_s'
ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-linux-gnu]
"3.3.24"
"x86_64-linux-gnu"
```

As you can see, it now has the `-gnu` prefix and `Libv8::Node::Paths.object_paths` thus has it when the built binary actually lives in the directory `x86_64-linux`, breaking the build of
the gems like mini_racer on those platforms.

[`RUBY_TARGET_PLATFORM` is set to `*-linux`](https://github.com/knu/libv8-node/blob/ad9105562185571f7b486f7770986f6a160318b2/libexec/platform#L28-L29), so `Libv8::Node::Paths.platform` needs to align with that.
@knu
Copy link
Author

knu commented Jun 24, 2024

Done. I'm not following the changes since I submitted this PR, though.

@lloeki
Copy link
Collaborator

lloeki commented Jun 24, 2024

can you rebase

FYI master does not exactly represent what it usually means in other more "classic" repos as work happens in node-${version} branches.

So instead, PRs should typically target the latest node-${version} branch.

I've been tempted to simply clear out master to reflect that and have it only contain a README explaining the development model.

I really would love to release a gem with this fix

@SamSaffron as I mentioned here (way down in the comment I'm working on it (well I'm going to work on it during the coming week as soon as time permits)

being broken on brand new ruby is a problem

It's not "brand new Ruby", it's RubyGems on rubies that have RUBY_PLATFORM with a -gnu suffix, which appears to be only the apt-packaged Ruby and RubyGems of ubuntu:24.04 on Docker Hub and no other: see here.

Doesn't mean a fix is not warranted but let's be reasonable about the actual urgency.

@MaZderMind
Copy link

I would +1 your suggestions regarding the master branch. That would clarify things quite a bit.

@lloeki
Copy link
Collaborator

lloeki commented Jul 2, 2024

Just a note, I'm not forgetting about this, I just keep on getting short-handed on time.

@cschroed
Copy link

cschroed commented Jul 2, 2024

Just a note, I'm not forgetting about this, I just keep on getting short-handed on time.

Thanks for keeping me up to date, I do appreciate it greatly.

I'm not proficient in this space, node/js/etc, but if there is anything I can do to help, please do let me know.

@lloeki
Copy link
Collaborator

lloeki commented Jul 23, 2024

Released 22.5.1.0 which does NOT include a fix yet. I have it scheduled next, should be 22.5.1.1.

@cschroed
Copy link

Released 22.5.1.0 which does NOT include a fix yet. I have it scheduled next, should be 22.5.1.1.

Thanks for the update and progress. We have a few more tasks on a current project that should take another 3 weeks. So hopefully that coincides with the next release for you. Not trying to apply pressure, just being hopeful/optimistic.

I understand you are maintaining this on your own time and appreciate all the effort and updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants