bazelruby / rules_ruby

Formerly canonical rules for ruby, that are about 2-3 years behind current Bazel. If they work for you great, but if not — please try the new rules ruby by Alex Radionov: https://github.com/bazel-contrib/rules_ruby
Apache License 2.0
99 stars 37 forks source link

Upstream changes from Selenium's fork #118

Closed p0deje closed 2 years ago

p0deje commented 2 years ago

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.
kigster commented 2 years ago

image

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

p0deje commented 2 years ago

Thanks, I'll fix the formatting issues.

kigster commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

Please see the discussion https://github.com/bazelruby/rules_ruby/discussions/119 for more details.

kigster commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

p0deje commented 2 years ago

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 commented 2 years ago

Alright, I am merging it then!