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 bazel config file #48084

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add bazel config file #48084

wants to merge 1 commit into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Sep 10, 2024

I'm currently working on adding WPT to Cloudflare workers, and it has been a little bit of pain to define filegroups and run a WPT test runner. I propose adding a BUILD.bazel file as stated in this pull-request to expose the filegroups for other projects that run Bazel. This would simplify the adoption.

Once, this pull-request has some feedback, I'll add the rest of the directories/filegroups to the build file before merging.

cc @web-platform-tests/reviewers

@WeizhongX
Copy link
Contributor

Can you elaborate more on the purpose of this change?

@anonrig
Copy link
Member Author

anonrig commented Sep 10, 2024

Can you elaborate more on the purpose of this change?

Having a BAZEL.build and tests.bzl a inside here, but not on implementors, would allow glob files to be exported and used in bazel projects.

Here is a work-in-progress pull-request for our WPT test runner PoC: cloudflare/workerd#2585

We're dynamically generating a capnproto file inside bazel build step, that adds every file in url folder in a specific format to the workerd configuration format. Ref: https://github.com/cloudflare/workerd/pull/2585/files#diff-bcd6e77ca5642ae1d1b579304dc679a4bca44e3261a11d61a68e7c7c51acc107R53

Unfortunately, the only option for workerd repository is to have a floating patch on top of wpt repository, to have access to each glob file on the macro level of the build step. When tests.bzl file is imported on a bazel file and used inside a function in Bazel, it would resolve into an array of strings/paths.

To summarize, in order to iterate over the files inside a subfolder of WPT repository, we need to have glob(["url/**/*"]) defined inside a .bzl file at the same folder the project.

@jcscottiii
Copy link
Contributor

I think this type of change would require a RFC. Right @jgraham ?

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 11, 2024

At least there should be documentation somewhere in the repo what this is for

@anonrig
Copy link
Member Author

anonrig commented Sep 11, 2024

At least there should be documentation somewhere in the repo what this is for

Where do you suggest adding that?

@anonrig anonrig force-pushed the anonrig/add-bazel-support branch 2 times, most recently from 8d91c91 to f70111e Compare September 11, 2024 17:53
@anonrig
Copy link
Member Author

anonrig commented Sep 11, 2024

I think this is ready to review. I've added all submodules in the project as well as a Github workflow to check the formatting and linting of the Bazel files. I appreciate if you could take a look. @jgraham @Ms2ger @jcscottiii @WeizhongX

@anonrig anonrig force-pushed the anonrig/add-bazel-support branch from f70111e to cb48c30 Compare September 11, 2024 17:55
.github/workflows/bazel.yml Show resolved Hide resolved
tests.bzl Outdated Show resolved Hide resolved
tests.bzl Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the anonrig/add-bazel-support branch from cb48c30 to cd6cdb2 Compare September 11, 2024 18:13
@wpt-pr-bot wpt-pr-bot requested a review from foolip September 11, 2024 18:14
@anonrig anonrig force-pushed the anonrig/add-bazel-support branch 3 times, most recently from 4dea022 to 88395bc Compare September 11, 2024 18:33
tests.bzl Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the anonrig/add-bazel-support branch from 88395bc to 8a88493 Compare September 11, 2024 18:43
@jgraham
Copy link
Contributor

jgraham commented Sep 16, 2024

From an admin perspective this seems like a very hard sell to me. In particular:

  • Nothing else is using bazel. We don't want consumers to see bazel files and assume that's something they need to install or similar.
  • It's unclear what the long term maintenance story is. Who is going to own this code over time?
  • It's adding extra third party dependencies to the CI (at least in the actions configuration).

I tend to agree with @Ms2ger and @jcscottiii that some more discussion about requirements, preferably as part of an RFC, would be needed to accept this. Otherwise I'd suggest keeping this configuration in a different repo if possible.

@anonrig
Copy link
Member Author

anonrig commented Sep 16, 2024

Thank you for the review @jgraham.

Nothing else is using bazel. We don't want consumers to see bazel files and assume that's something they need to install or similar.

I can't think of a solution to this except: We can add a comment/warning to the files.

It's unclear what the long term maintenance story is. Who is going to own this code over time?

There is no particular maintenance cost, since all of the .bzl files in this pull-request are dynamically generated. Unless, the main project structure changes, it's highly unlikely to be changed. PS: I'm also the author/maintainer of Ada URL, and I'm more than happy to maintain these 2 files.

It's adding extra third party dependencies to the CI (at least in the actions configuration).

It doesn't have to be. I just added it to make sure the formatting/linting is there. I can remove it, and we can assume good faith while reviewing pull-requests which is highly-unlikely.


I am more than happy to open a RFC particularly for this change.

@shs96c
Copy link
Contributor

shs96c commented Sep 24, 2024

One pattern that gets used quite often for this kind of thing is an "overlay" There's a good summary of this in this blogpost, with a concrete implementation being how Google used to apply bazel build files to LLVM: https://github.com/google/llvm-bazel.

Because the build file is so simple, you could also use the build_file_content attribute of http_archive to do the same thing, though this would necessarily mean that the tests.bzl file would be missing.

@shs96c
Copy link
Contributor

shs96c commented Sep 25, 2024

Thinking some more, if the BUILD.bazel file used exports_files then you may be able to do the iteration over them elsewhere (though I'm not 100% sure that will work as expected)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants