Upstream changes from Selenium's fork by p0deje · Pull Request #118 · bazelruby/rules_ruby

p0deje · 2021-10-25T19:25:07Z

This PR introduces multiple changes that are necessary to make Ruby rules work in Selenium https://github.com/SeleniumHQ/selenium. Historically, we've been maintaining our fork of https://github.com/coinbase/rules_ruby but would prefer to upstream the changes to Bazelruby.

You should be able to review this PR by commit, but let me go through the changes here too:

Allow to symlink additional files for ruby_bundle() via src attribute. This allows to specify *.gemspec files and use Bazel for regular Ruby gem development. Example
No longer require gemfile_lock for ruby_bundle(). This file is usually gitignored in Ruby gems.
Multiple fixes to make Ruby host SDK load on Windows, Linux and JRuby. Windows support is pretty much non-existent, but at least Bazel now loads with Ruby SDK in WORKSPACE.
Local gems installed using gem 'foo', path: 'foo' are now skipped when generating BUILD.bazel for ruby_bundle(). There seems to be no need to include them.

kigster · 2021-10-26T19:26:10Z

I think the tests are passing, just some buildifier formatting errors?

p0deje · 2021-10-26T22:20:59Z

Thanks, I'll fix the formatting issues.

kigster · 2021-10-30T06:24:02Z

I am trying to build this branch, and for some reason it's no longer finding a local Ruby available and so the toolchain switches to building Ruby each time:

(23:22:13) INFO: Current date is 2021-10-29
(23:22:13) DEBUG: Rule 'bazel_gazelle' indicated that a canonical reproducible form can be obtained by modifying arguments shallow_since = "1590680880 -0400"
(23:22:13) DEBUG: Repository bazel_gazelle instantiated at:
  /Users/kig/workspace/bazel/rules_ruby-1/WORKSPACE:39:15: in <toplevel>
Repository rule git_repository defined at:
  /private/var/tmp/_bazel_kig/49f867657fa362f9f488ae2165d48e8f/external/bazel_tools/tools/build_defs/repo/git.bzl:199:33: in <toplevel>
(23:22:13) DEBUG: Rule 'com_google_protobuf' indicated that a canonical reproducible form can be obtained by modifying arguments shallow_since = "1595293740 -0700"
(23:22:13) DEBUG: Repository com_google_protobuf instantiated at:
  /Users/kig/workspace/bazel/rules_ruby-1/WORKSPACE:45:15: in <toplevel>
Repository rule git_repository defined at:
  /private/var/tmp/_bazel_kig/49f867657fa362f9f488ae2165d48e8f/external/bazel_tools/tools/build_defs/repo/git.bzl:199:33: in <toplevel>
(23:22:28) Analyzing: 27 targets (25 packages loaded, 36 targets configured)
(23:22:28) Analyzing: 27 targets (27 packages loaded, 36 targets configured)
/var/folders/jq/853fg3814rs6xx_zxk9sgsv40000gn/T/ruby-build.20211029232228.62636.j1JKT4 /private/var/tmp/_bazel_kig/49f867657fa362f9f488ae2165d48e8f/external/org_ruby_lang_ruby_toolchain
Downloading openssl-1.1.1k.tar.gz...
HTTP/1.1 200 OK
Content-Type: binary/octet-stream
Content-Length: 9823400
Connection: keep-alive
Date: Sat, 30 Oct 2021 06:22:30 GMT
Last-Modified: Mon, 12 Apr 2021 08:10:49 GMT
ETag: "c4e7d95f782b08116afa27b30393dd27"
Accept-Ranges: bytes
Server: AmazonS3
X-Cache: Miss from cloudfront
Via: 1.1 da42857c896088f1d50640bf030a2034.cloudfront.net (CloudFront)
X-Amz-Cf-Pop: LAX50-C1
X-Amz-Cf-Id: 3lspeHONIMqaazi9JUz4Apkr_zhuZcVgkD_qeuZvJJZiQ9XhX-L6sg==

Any idea why that might be happening?

kigster · 2021-10-30T06:27:06Z

Just to be clear — we decided a long time ago that if you have rbenv and your rbenv's ruby version matches whatever your workspace is needing, it will use the local ruby to save time. We understood that this not the cleanest way to support ruby, because of the coupling between pre-insatalled Ruby and the bazel-built project. But at the time waiting for Ruby to build each time was not just impractical, it was killing all desire to use bazel.

So any suggestions on this are welcome and much appreciated.

kigster · 2021-10-30T22:10:06Z

A actually, j think picking the local ruby is working still, but building c extensions in tests fails:

Please see the discussion #119 for more details.

kigster

I really appreciate all the work, especially all the windows related changes, but I feel that the PR can really benefit from additional comments and documentation.

I am also going to look into why circle CI is not working

kigster · 2021-10-30T22:14:36Z

README.adoc

-    gemfile = "//apps/shopping:Gemfile",
-    gemfile_lock = "//apps/shopping:Gemfile.lock",
+    gemfile = "//:apps/shopping/Gemfile",
+    gemfile_lock = "//:apps/shopping/Gemfile.lock",


Can you explain why the : is needed?

I think the original documentation is not working because the colon should preceed directory name. At least that's what I had to do in Selenium where all Ruby code lives in rb directory (https://github.com/SeleniumHQ/selenium/blob/trunk/WORKSPACE#L286):

ruby_bundle( name = "bundle", srcs = [ "//:rb/lib/selenium/devtools/version.rb", "//:rb/lib/selenium/webdriver/version.rb", "//:rb/selenium-devtools.gemspec", "//:rb/selenium-webdriver.gemspec", ], gemfile = "//:rb/Gemfile", )

kigster · 2021-10-30T22:21:21Z

README.adoc

    bundler_version = "2.1.4",
    includes = {},
    excludes = {},
+    srcs = [],


How did we not have src here before?

I've added this attribute.

kigster · 2021-10-30T22:22:38Z

README.adoc

@@ -497,12 +506,17 @@ A unique name for this rule.
 The `Gemfile` which Bundler runs with.

 |`gemfile_lock` a|


Should this be now called gemfile? Since we compute the lock file automatically?

You are right. I'll fix it so that it still respects gemfile_lock when it's passed.

Just pushed fixup commit 7f192df which addresses this and actually makes less changes to existing. If it's good, I'll rebase and squash the commits together.

kigster · 2021-10-30T22:23:38Z

ruby/private/bundle/create_bundle_build_file.rb


  def register_gem(spec, template_out, bundle_lib_paths, bundle_binaries)
+    # Do not register local gems
+    return if spec.source.path?


Can you explain that line?

It's not possible to register local gems as they are not linking to external/bundle. This guard just prevents the code from even trying to do so.

kigster · 2021-10-30T22:29:29Z

By the way:

Local gems installed using gem 'foo', path: 'foo' are now skipped when generating BUILD.bazel for ruby_bundle(). There seems to be no need to include them.

Perhaps this could be a flag we pass to ruby_bundle?

In my mind locally built gems are useless if they can't be later referenced in a bundle.

p0deje · 2021-10-31T15:23:01Z

By the way:

Local gems installed using gem 'foo', path: 'foo' are now skipped when generating BUILD.bazel for ruby_bundle(). There seems to be no need to include them.

Perhaps this could be a flag we pass to ruby_bundle?

In my mind locally built gems are useless if they can't be later referenced in a bundle.

Maybe that's not the best way to handle local gems. What we do in Selenium is registering local gem we develop:

# Gemfile
source 'https://rubygems.org'
gemspec

This way our gem is now available to the Bundler. However, we don't want to make it available in bundle BUILD.bazel because we are still going to have our own build targets for this gem.

However, there might be other cases when gems are loaded via path which should be available via BUILD.bazel. I'm not sure how to between these 2 types of local gems.

p0deje · 2021-10-31T15:24:47Z

A actually, j think picking the local ruby is working still, but building c extensions in tests fails:

Hm, I can look into this if you share more details on how to reproduce the failure.

p0deje · 2021-10-31T15:48:58Z

I really appreciate all the work, especially all the windows related changes, but I feel that the PR can really benefit from additional comments and documentation.

Do you have anything particular you would like documenting? I've added comments for non-obvious parts of code and docs for new/changed attributes, but not sure what else would you like to see.

kigster · 2021-11-05T05:56:56Z

Apologies for the delay, but life's gotten in the way.

p0deje · 2021-11-08T21:08:34Z

Apologies for the delay, but life's gotten in the way.

No worries, take your time! I already switched Selenium to use this branch and it works just fine.

kigster · 2021-11-08T22:00:37Z

Alright, I am merging it then!

p0deje requested review from kigster and yugui as code owners October 25, 2021 19:25

p0deje marked this pull request as draft October 25, 2021 19:26

p0deje added 2 commits October 27, 2021 13:41

Allow to symlink additional files for ruby_bundle()

b7574c4

Allow to use ruby_bundle() without Gemfile.lock

9ed01cb

p0deje force-pushed the selenium branch from 8843c18 to cdb2265 Compare October 27, 2021 20:43

p0deje added 5 commits October 27, 2021 13:53

Fix host Ruby runtime crash on Fedora 34

69faf99

Fix host Ruby runtime loading on Windows

976ee03

Fix host Ruby runtime loading for JRuby on Linux

299c4d8

Do not register local gems within ruby_bundle()

4b1c480

Support JRuby gem loading

36893fe

p0deje force-pushed the selenium branch from cdb2265 to 36893fe Compare October 27, 2021 20:53

p0deje changed the title ~~Upstream Selenium changes~~ Upstream changes from Selenium's fork Oct 27, 2021

p0deje marked this pull request as ready for review October 27, 2021 21:05

kigster requested changes Oct 30, 2021

View reviewed changes

fixup! Allow to use ruby_bundle() without Gemfile.lock

7f192df

kigster merged commit a391ccf into bazelruby:master Nov 8, 2021

		@@ -497,12 +506,17 @@ A unique name for this rule.
		The `Gemfile` which Bundler runs with.

		\|`gemfile_lock` a\|

Comments

Conversation

p0deje commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kigster commented Oct 26, 2021

Uh oh!

p0deje commented Oct 26, 2021

Uh oh!

kigster commented Oct 30, 2021

Uh oh!

kigster commented Oct 30, 2021

Uh oh!

kigster commented Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kigster left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kigster commented Oct 30, 2021

Uh oh!

p0deje commented Oct 31, 2021

Uh oh!

p0deje commented Oct 31, 2021

Uh oh!

p0deje commented Oct 31, 2021

Uh oh!

kigster commented Nov 5, 2021

Uh oh!

p0deje commented Nov 8, 2021

Uh oh!

kigster commented Nov 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

p0deje commented Oct 25, 2021 •

edited

Loading

kigster commented Oct 30, 2021 •

edited

Loading

kigster left a comment •

edited

Loading