8000 Upstream changes from Selenium's fork by p0deje · Pull Request #118 · bazelruby/rules_ruby · GitHub
[go: up one dir, main page]
Skip to content

Upstream changes from Selenium's fork #118

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Nov 8, 2021
Merged

Upstream changes from Selenium's fork #118

merged 8 commits into from
Nov 8, 2021

Conversation

p0deje
Copy link
Contributor
@p0deje p0deje commented Oct 25, 2021 •
edited
Loading

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:

  1. 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
  2. No longer require gemfile_lock for ruby_bundle(). This file is usually gitignored in Ruby gems.
  3. 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.
  4. 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.

@p0deje p0deje requested review from kigster and yugui as code owners October 25, 2021 19:25
@p0deje p0deje marked this pull request as draft October 25, 2021 19:26
@kigster
Copy link
Contributor
kigster commented Oct 26, 2021

image

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

@p0deje
Copy link
Contributor Author
p0deje commented Oct 26, 2021

Thanks, I'll fix the formatting issues.

@p0deje p0deje changed the title Upstream Selenium changes Upstream changes from Selenium's fork Oct 27, 2021
@p0deje p0deje marked this pull request as ready for review October 27, 2021 21:05
@kigster
Copy link
Contributor
kigster commented Oct 30, 2021

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
Copy link
Contributor
kigster commented Oct 30, 2021

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
Copy link
Contributor
kigster commented Oct 30, 2021 •
edited
Loading

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
kigster requested changes Oct 30, 2021
View reviewed changes
Copy link
Contributor
@kigster kigster left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

README.adoc
gemfile = "//apps/shopping:Gemfile",
gemfile_lock = "//apps/shopping:Gemfile.lock",
gemfile = "//:apps/shopping/Gemfile",
gemfile_lock = "//:apps/shopping/Gemfile.lock",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why the : is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
)

README.adoc
@@ -477,6 +485,7 @@ ruby_bundle(
bundler_version = "2.1.4",
includes = {},
excludes = {},
srcs = [],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did we not have src here before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this attribute.

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

|`gemfile_lock` a|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ruby/private/bundle/create_bundle_build_file.rb
@@ -247,6 +251,9 @@ def remove_bundler_version!
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain that line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor
kigster commented Oct 30, 2021

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
Copy link
Contributor Author
p0deje commented Oct 31, 2021

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
Copy link
Contributor Author
p0deje commented Oct 31, 2021

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
Copy link
Contributor Author
p0deje commented Oct 31, 2021

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
Copy link
Contributor
kigster commented Nov 5, 2021

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

@p0deje
Copy link
Contributor Author
p0deje commented Nov 8, 2021

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
Copy link
Contributor
kigster commented Nov 8, 2021

Alright, I am merging it then!

@kigster kigster merged commit a391ccf into bazelruby:master Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kigster kigster kigster requested changes

@yugui yugui Awaiting requested review from yugui yugui is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@p0deje @kigster
0