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

Add better support for building and testing through Bazel. #6

Merged
merged 14 commits into from
Jun 27, 2024

Conversation

davidlion
Copy link
Member

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.

@davidlion davidlion marked this pull request as ready for review June 24, 2024 19:53
Copy link
Member

@junhaoliao junhaoliao left a 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")
Copy link
Member

@junhaoliao junhaoliao Jun 24, 2024

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?)

Copy link
Member

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?

Copy link
Member Author

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

cpp/deps.bzl Show resolved Hide resolved
ffi/BUILD.bazel Outdated Show resolved Hide resolved
ir/BUILD.bazel Outdated Show resolved Hide resolved
search/BUILD.bazel Outdated Show resolved Hide resolved
ir/BUILD.bazel Outdated Show resolved Hide resolved
cpp/deps.bzl Show resolved Hide resolved
junhaoliao
junhaoliao previously approved these changes Jun 27, 2024
Copy link
Member

@junhaoliao junhaoliao left a 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.

kirkrodrigues
kirkrodrigues previously approved these changes Jun 27, 2024
Copy link
Member

@kirkrodrigues kirkrodrigues left a 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.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@@ -2,3 +2,7 @@ build/
cpp/.cache/
cpp/clp/
**/compile_commands.json
bazel-bin
Copy link
Member

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

MODULE.bazel Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@davidlion davidlion dismissed stale reviews from kirkrodrigues and junhaoliao via b02a80e June 27, 2024 22:24
@davidlion davidlion changed the title Support building and testing entirely through bazel. Add better support for building and testing through Bazel. Jun 27, 2024
@kirkrodrigues kirkrodrigues merged commit 898c8c2 into y-scope:main Jun 27, 2024
10 checks passed
@davidlion davidlion deleted the bazel-fix branch June 27, 2024 23:35
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