-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add better support for building and testing through Bazel. #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. There were some minor changes I had to make to get this integrated into a monorepo which has not yet migrated to Bzlmod. Let's discuss offline to see if any process can be optimized.
MODULE.bazel
Outdated
@@ -0,0 +1,17 @@ | |||
module(name = "com_github_y_scope_clp_ffi_go", version = "0.0.5-beta") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a WORKSPACE? (Would it help projects which haven't migrated to Bzlmod?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REPO.bazel does seem good to have for specifying the OSS license for all sources: https://bazel.build/external/overview#repo.bazel Do we want to add such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a WORKSPACE? (Would it help projects which haven't migrated to Bzlmod?)
Afaik, this would only enable building ffi-go itself through old versions of bazel, which isn't necessary to support (on its own ffi-go doesn't really do anything).
REPO.bazel does seem good to have for specifying the OSS license for all sources: https://bazel.build/external/overview#repo.bazel Do we want to add such?
As discussed, exploring https://bazelbuild.github.io/rules_license/latest.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to integrate the latest commit into a WORKSPACE (non-Bzlmod) project without any modification.
Here is the WORKSPACE configuration:
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
def com_github_y_scope_clp_ffi_go():
_com_github_y_scope_clp_ffi_go_commit = "0d8c24777af639a65a317954eec75554e6b121b0"
http_archive(
name = "com_github_y_scope_clp_ffi_go",
sha256 = "19d2e46f13233a538b2dae103c07f46623ee19e48b5d4bc05348776b8e769d01",
urls = ["https://github.com/davidlion/clp-ffi-go/archive/{}.zip".format(_com_github_y_scope_clp_ffi_go_commit)],
strip_prefix = "clp-ffi-go-{}".format(_com_github_y_scope_clp_ffi_go_commit),
)
...
load("@com_github_y_scope_clp_ffi_go//cpp:deps.bzl", "com_github_y_scope_clp")
com_github_y_scope_clp()
The changes look good to me, and the PR is fine for the squashed commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments. I trust Junhao's review.
@@ -2,3 +2,7 @@ build/ | |||
cpp/.cache/ | |||
cpp/clp/ | |||
**/compile_commands.json | |||
bazel-bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we group and alphabetize? E.g., something like
# Bazel-generated files
bazel-bin
...
# C++ build generated files
cpp/.cache/
**/compile_commands.json
# Submodules
cpp/clp
Co-authored-by: kirkrodrigues <[email protected]>
Description
Previously building and testing through bazel was only partially supported and required non-trivial additions into the module using the library.
This PR adds support to completely build and test the library directly though bazel.
The main addition is adding a bazel module extension to support building the FFI core as a library from the CLP submodule. This allows bazel projects depending on ffi-go to pull it in with minimal work.
Validation performed
Build and testing though bazel added to workflows.
Imported the library into another bazel project locally.