Skip to content

feat: expose MRI C headers and JRuby jars#197

Merged
p0deje merged 12 commits into
mainfrom
ext
Jan 25, 2025
Merged

feat: expose MRI C headers and JRuby jars#197
p0deje merged 12 commits into
mainfrom
ext

Conversation

@p0deje

@p0deje p0deje commented Jan 23, 2025

Copy link
Copy Markdown
Member

Allows to use C headers on MRI to compile cc_library and JRuby jars to compile java_library targets.
See examples/native_ext for examples.
Relates to #186.

@JasonLunn

Copy link
Copy Markdown
Contributor

LGTM. Are the examples automatically tested, or would an explicit test be needed to make sure this works in CI?

@p0deje

p0deje commented Jan 24, 2025

Copy link
Copy Markdown
Member Author

@p0deje p0deje requested review from alexeagle and sushain97 January 24, 2025 22:07
@@ -0,0 +1,6 @@
# Not ready for the MODULE.lock file yet, as of Bazel 7.0.0 there are still some stability issues.
common --lockfile_mode=off

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: it's better to gitignore it instead. You can get some perf benefits locally by Bazel reading the lockfile from your previous work, and not share it to others

main = ":ruby_file",
)

cc_library(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: there's some flag in bazel forcing these symbols to be loaded from a ruleset rather than using the now-deprecated native symbols. If a user turns on that flag I think this will be broken for them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've added an explicit dependency on rules_cc and rules_java in 0f52ccb, I hope that makes sense. Not sure what is the lowest version I could use so I looked at what other rulesets use.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is OK but it feels a bit undesirable that you're forced into carrying these CC/Java toolchain deps even if you have no interest in using them. Don't have any good ideas to avoid it though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's why I originally didn't add those and used native rules. @alexeagle Can you advise what would be better here?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You don't really have a choice. This repo wants to expose the providers for interop with users repository, and providers are symbols rather than strings, so they have to be loaded from the same place.

We'll hope that rules_java and rules_cc are well-maintained so there aren't breaking changes that mean your users are sensitive to the version you pick. In bzlmod you can pick something old, since you're really picking a lower-bound; the MVS algorithm means users can still resolve to something newer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For both rulesets, I used > 1-year-old releases and crossed-check what other dependents do (e.g. rules_python, rules_kotlin).

main = ":ruby_file",
)

cc_library(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is OK but it feels a bit undesirable that you're forced into carrying these CC/Java toolchain deps even if you have no interest in using them. Don't have any good ideas to avoid it though.

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.

4 participants