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

Make ruby_binary compatible with container #17

Merged
merged 8 commits into from
Dec 2, 2019

Conversation

yugui
Copy link
Contributor

@yugui yugui commented Dec 1, 2019

Reimplement the runfiles resolution based on the python version

@yugui yugui requested a review from kigster December 1, 2019 10:26
@kigster kigster requested a review from grahamjenson December 1, 2019 14:10
@kigster
Copy link
Contributor

kigster commented Dec 1, 2019

Wow, @yugui , this looks amazing!

Could you be so kind and add a tiny bit of context into the description so that it's a bit easier to follow what is being changed and why? I am going to dig into this PR later today, but any additional description would be very helpful.

Thank you so much!

Konstantin

@kigster kigster added the needs-review Please review this PR label Dec 1, 2019
@grahamjenson
Copy link
Contributor

This looks really good, I like the removal the the loadpath script and have it all in one.

I was wondering about the ruby executable in the container image. Should pkg_tar bundling the ruby binary with the tar? Or shouldn't we rely on the ruby version inside the container?

The reason for the I ask is we are pretty focused on cross-compatability, and since most of us are on OSX here, bundling the binary will not work inside the image.

Cheers for this awesome work!

Copy link
Contributor

@grahamjenson grahamjenson left a comment

Choose a reason for hiding this comment

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

I think this will work but not sure the tests will run on OSX, because of cross compatibility. I will try run these later and see if I can get it working.

@yugui
Copy link
Contributor Author

yugui commented Dec 2, 2019

@kigster @grahamjenson
Thank you for your comments.
You are right. I should have explained more about the design and future plans.

What this PR does

  • Correctly handles several different types of runtime directory structure discussed at an equivalent code in rules_nodejs
    • I have reimplemented this runfiles resolution based on an equivalent code which py_binary provides.
  • Makes ruby_binary work fine in container.
  • Stop depending on the installation path of the host interpreter.
    • ruby_binary built on MacOS is now able to run on Linux as far as the binary and its dependencies do not contain native extensions.
    • The container test does work on MacOS because currently the test scripts are pure-Ruby.
  • Removes extra complexity in the original implementation.
    • Some of them were introduced just because of my misunderstanding about Bazel's sandbox system. Some others are no longer necessary

Limitations

  • This PR stopped bundling host interpreter in ruby_binary. So ruby_binary now depends on the interpreter installed on the runtime environment as py_binary does.
    • As a side-effect, this PR tentatively stopped resolving the interpreter path from runfiles and always tries to use the host interpreter.
    • I am going to recover the functionality of interpreter path resolution as nodejs_binary does, just after this PR. It will be necessary to support Sorbet's interpreters.
  • For cross-compatibility, it is not a problem for the existing tests because they are pure-Ruby. However, we will need a cross-compiling with Sorbet's interpreters and C cross-compilers to support native extensions.

Plans

How about working on the issues with the following steps?

@grahamjenson grahamjenson merged commit 287cdef into develop Dec 2, 2019
@grahamjenson grahamjenson deleted the refactor/runtime-compat branch December 2, 2019 05:25
@kigster
Copy link
Contributor

kigster commented Dec 2, 2019

@yugui @grahamjenson All of that sounds great!

Let's Start Using Github Project!

(CC @volkangurel) Guys — I would like to note that we started using the Github Project feature (which looks a lot like Trello) and I would love it if you added any work you plan on doing there in the appropriate column.

Basically, the Project View is a project view resembling Trello swimming lanes and is meant more for brainstorming than for project tracking. Cards we create can remain cards or be used to very easily auto-generate a new linked issue, where we should add more technical details, if needed.

Subsequent PR will reference the issue, and once merged, it will auto-fix the issue, and the associated card will move into the Done ✅ column. Since the three of us are already moving pretty quickly, I would love to have one place where we can all see where we are all at.

I am going to take the liberty and add Yugui's bullet points as Cards there , but will leave it to her to create the issues and move each card into the In Progress when able.

Mini Sprint Planner?

If it were possible to book a 45 minute time slot later this week for the three of us to hop on Google Hangout and do a mini-planner, that would be really great. Yugui, we have 7-hour difference, so say 10am in Japan would be 5pm in San Francisco and we can certainly make that happen easily. The only problem with this week is that Graham is at AWS Reinvent Conference.

Copy link
Contributor

@kigster kigster left a comment

Choose a reason for hiding this comment

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

👍

@kigster kigster removed the needs-review Please review this PR label Dec 2, 2019
@kigster kigster added this to the M-0.1 milestone Dec 16, 2019
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.

3 participants