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

Support MODULE.bazel #2781

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Support MODULE.bazel #2781

merged 3 commits into from
Dec 13, 2023

Conversation

igormcoelho
Copy link
Contributor

Description

First of all, I love Catch2 library! And I also love Bazel Build... recently (today dec, 12 2023) the Bazel team launched version 7.0.0, that now began the process to force all users to abandon legacy WORKSPACE pattern into a newer MODULE.bazel pattern.
To be short, the changes I'm proposing are only two lines of code and a new file named MODULE.bazel, and this makes Catch2 compatible with Bzlmod system. The file is the following:

module(name = "catch2")
bazel_dep(name = "bazel_skylib", version = "1.4.2")

The good thing is that it manages so much better the bazel_skylib dependency currently on WORKSPACE:

workspace(name = "catch2")

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "bazel_skylib",
    sha256 = "66ffd9315665bfaafc96b52278f57c7e2dd09f5ede279ea6d39b2be471e7e3aa",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.4.2/bazel-skylib-1.4.2.tar.gz",
        "https://github.com/bazelbuild/bazel-skylib/releases/download/1.4.2/bazel-skylib-1.4.2.tar.gz",
    ],
)

load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace")

bazel_skylib_workspace()

So, this won't be necessary in the future (for now, it's best to keep both ways).
The MODULE system is better, because it allows transitive dependencies, meaning that users won't suffer by not having the skylib dependency (it will be downloaded automatically, what does not happen today!).

Detailed Example for Legacy (today)

Noways, users do the following. On their WORKSPACE, they put:

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
    name = "catch2",
    urls = ["https://github.com/catchorg/Catch2/archive/v3.4.0.tar.gz"],
    strip_prefix = "Catch2-3.4.0",
    sha256 = "122928b814b75717316c71af69bd2b43387643ba076a6ec16e7882bfb2dfacbb"
)
# catch2 dependency on skylib falls back on users...
http_archive(
    name = "bazel_skylib",
    sha256 = "66ffd9315665bfaafc96b52278f57c7e2dd09f5ede279ea6d39b2be471e7e3aa",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.4.2/bazel-skylib-1.4.2.tar.gz",
        "https://github.com/bazelbuild/bazel-skylib/releases/download/1.4.2/bazel-skylib-1.4.2.tar.gz",
    ],
)
load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace")
bazel_skylib_workspace()

And then, the users may consume the target @catch2//:catch2_main normally.

Detailed Example for Proposal (modules system)

With these two lines, users do the following. On their MODULES.bazel, they put:

bazel_dep(name = "catch2", dev_dependency = True)
git_override(
    module_name = "catch2",
    # using my own testing link here, must use catchorg/Catch2 after this PR is accepted
    remote = "https://github.com/igormcoelho/Catch2-devel.git", 
    commit = "b4e6de3e68c035b5ed14c190317209269f90e34a",
)

And then, the users may consume the target @catch2//:catch2_main normally (no need to load dependency skylib, unless they really need it).

What if this PR is not accepted

Nowadays, users can only use Legacy WORKSPACE system, because the remote repository needs to have the MODULE.bazel file... I tried to workaround this, but it' not possible... So, it's two lines and a file that will help and simplify the life of a lot of people.

GitHub Issues

This PR is very simple, so it can be discussed here, if you find it valuable for Catch2 (please accept it!!!).

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Merging #2781 (b4e6de3) into devel (53d0d91) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##            devel    #2781      +/-   ##
==========================================
+ Coverage   91.51%   91.52%   +0.01%     
==========================================
  Files         198      198              
  Lines        8243     8243              
==========================================
+ Hits         7543     7544       +1     
+ Misses        700      699       -1     

@igormcoelho
Copy link
Contributor Author

Marking @Vertexwahn @horenmar, as they last changed WORKSPACE.bazel file ;)

@Vertexwahn
Copy link
Contributor

Vertexwahn commented Dec 13, 2023

@igormcoelho For me the change is fine. Maybe you can run buildifier on ’MODULE.bazel’. Also using the newest Skylib (1.5.0) would make sense.

@horenmar horenmar added the Building and Packaging Issues affecting build/packaging scripts and utilities label Dec 13, 2023
@horenmar
Copy link
Member

Sure, why not.

@horenmar horenmar merged commit d40a328 into catchorg:devel Dec 13, 2023
3 checks passed
@igormcoelho
Copy link
Contributor Author

Thanks a lot @Vertexwahn and @horenmar !

@igormcoelho For me the change is fine. Maybe you can run buildifier on ’MODULE.bazel’. Also using the newest Skylib (1.5.0) would make sense.

I chose 1.4.2 because it's the current version on WORKSPACE... I think that future upgrades should keep them both in same version, so if one migrates to 1.5.0, both should migrate.
There's two interesting things to consider in the future, but I could not properly understand until now, so I did not propose yet:

  1. There's some way to keep "better compatibility" between Legacy WORKSPACE and current Module... something like creating a file named WORKSPACE and another called WORKSPACE.bzlmod ... so that in some situations, the systems choose one or another. I didn't think it's the case, because Catch2 it's really very simple in this regard... maybe it's for more complex projects
  2. There's a way to pass the version to the MODULE.bazel file, instead of just fixing a standard one, like 1.4.2 or 1.5.0, so that if some external user has some more advanced version (say we adopt 1.4.2 here but user is on 1.5.0), then, MODULE.bazel will react to that and automatically adopt the maximum version of all. It looks interesting too.
  3. I think documentation should be improved too for Bazel, since now it's so much easier for users to use Catch2 (specially considering Bzlmod).. so maybe I come back in the future to try to improve this things too ;)

Best regards!

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Dec 13, 2023

For more information (for future devs coming here), I just tried here the two following setups:

#bazel_dep(name = "bazel_skylib", version = "1.5.0")
bazel_dep(name = "catch2", dev_dependency = True)
git_override(
    module_name = "catch2",
    remote = "https://github.com/catchorg/Catch2.git",
    commit = "3acb8b30f1ce0c801de0e3880ea1f6467dd0105c",
)

And then:

$ bazel mod deps @catch2
<root> (@_)
└───catch2@_ 
    └───bazel_skylib@1.4.2 

Then I force in my project 1.5.0:

bazel_dep(name = "bazel_skylib", version = "1.5.0")
bazel_dep(name = "catch2", dev_dependency = True)
git_override(
    module_name = "catch2",
    remote = "https://github.com/catchorg/Catch2.git",
    commit = "3acb8b30f1ce0c801de0e3880ea1f6467dd0105c",
)

Then it automatically chooses 1.5.0 for catch2:

 $ bazel mod deps @catch2
<root> (@_)
└───catch2@_ 
    └───bazel_skylib@1.5.0 

So, I guess, at least for non-major improvements in skylib (1.X.X), it's irrelevant what is put here as dependency :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Building and Packaging Issues affecting build/packaging scripts and utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants