Add ruby release targets #11468
Add ruby release targets #11468
Conversation
maven_install.json
Outdated
| @@ -651,6 +651,18 @@ | |||
| "sha256": "66fdef91e9739348df7a096aa384a5685f4e875584cce89386a7a47251c4d8e9", | |||
| "url": "https://repo1.maven.org/maven2/org/hamcrest/hamcrest-core/1.3/hamcrest-core-1.3.jar" | |||
| }, | |||
| { | |||
| "coord": "org.jruby:jruby-complete:9.2.20.1", | |||
There was a problem hiding this comment.
does "JRuby support version" become something we have to have an established policy on now as well? cc @devjgm
There was a problem hiding this comment.
does "JRuby support version" become something we have to have an established policy on now as well? cc @devjgm
Most likely, yes.
There was a problem hiding this comment.
As luck would have it, we should probably move off of 9.2 real soon, bump 9.3.x to the latest patch release, and add 9.4.
These defines aren't necessary anymore, and don't actually do anything. The new pattern is to use Line 54 in ade256e |
I implemented that but now when I test I get: How can I use this and test locally? |
Co-authored-by: Igor Pstyga <igor.pstyga@gmail.com>
Used this and deleted comments about using the other flags. |
| # --- end runfiles.bash initialization --- | ||
|
|
||
| # Make a temporary directory and move to it to do all packaging work | ||
| mkdir -p tmp |
There was a problem hiding this comment.
Since we're in the sandbox now do we need a tmp directory?
There was a problem hiding this comment.
I needed to make a new directory to start with a clean directory since this ends up being run in bazel-bin's runfile directory where a copy of all of the dependency files are.
There was a problem hiding this comment.
This gets run in bazel-bin/ruby/jruby_release.runfiles/com_google_protobuf/, which should only contain your dependencies. Why do we need to copy all of them into a fresh folder? This seems redundant, we could just run gem build from this directory
|
|
||
| # Move all generated files to lib/google/protobuf | ||
| mkdir -p lib/google/protobuf | ||
| cp "$(rlocation com_google_protobuf/src/google/protobuf/any_pb.rb)" lib/google/protobuf |
There was a problem hiding this comment.
You can use dirname to copy these with a wildcard
There was a problem hiding this comment.
I went back and forth on this. rlocation doesn't support wildcards, but I think you are suggesting that I get the dirname and then use wildcards which could be possible.
I ended up manually listing them as more of a precaution. Everywhere else the files are wildcarded in which I think could make it very easy to lose a file. This will fail in that case, which I think ends up being a good check. On the other hand, a new file being added could cause this to fail silently. What do you think about that tradeoff?
There was a problem hiding this comment.
If we were missing a file I would expect the gem build/test to fail in our CI. This seems a bit redundant to me, and not worth the maintenance burden.
One alternative that might work nicely is to add an sh_test rule that actually runs the tests using the built gem. That way we can actually run our tests locally and not have to wait for the CI to be run over a PR. It would also simplify the ruby_install.yml file I think
|
|
||
| # Move all generated files to lib/google/protobuf | ||
| mkdir -p lib/google/protobuf | ||
| cp "$(rlocation com_google_protobuf/src/google/protobuf/any_pb.rb)" lib/google/protobuf |
There was a problem hiding this comment.
ln or mv would be slightly more efficient here
| # rvm use ruby-3.0 | ||
|
|
||
| # Make a temporary directory and move to it to do all packaging work | ||
| mkdir -p tmp |
| $HOME/bin/bazel run ruby:jruby_release | ||
| gem install bazel-bin/ruby/jruby_release.runfiles/com_google_protobuf/tmp/google-protobuf-* | ||
| if: ${{ contains(matrix.ruby, 'jruby') }} | ||
| - name: Test installation |
There was a problem hiding this comment.
Maybe we should add a sanity check to the beginning to make sure these tests fail before installation?
There was a problem hiding this comment.
Do you know how to run in GHA expecting a failure? I can't find a good way of doing that (but can confirm that I have seen this fail when the gems were not correct).
There was a problem hiding this comment.
! bazel test <target> will invert the result
Add bazel targets to create ruby release artifacts.
Should be run with: