-
Notifications
You must be signed in to change notification settings - Fork 9
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 build support #43
base: main
Are you sure you want to change the base?
Conversation
Hey @marcosgriselli, thanks for taking the time to submit this pull request. I'm not too familiar with Bazel so I'm not too sure of the implications of this change. I will run it by the team and get back to you. My only concern is that it might require additional maintenance as the library changes over time. For example, some features that we have been experimenting with depend on files and environment variables created by SPM or Xcode, is it likely that this will work the same in Bazel builds? @blaknite Have you had much experience with Bazel? |
@iampatbrown understandable if this is not a priority. Unfortunately the collector won't work effectively with caching out of the box with Bazel projects since changing the environment variables would break hermetics. Happy to help in case Bazel support is a feature this library wants to have. |
@marcosgriselli Are you using test collector with Bazel? How did you fixed the broken hermetics? |
@lucasmarcal-faire we took the collection portion of the library, write the outputs to a standard output path and upload via the API at the end of our testing pipeline which includes more than just the bazel invocation. |
I asked one of our BK engineers who is more experienced than me on Bazel for feedback on this PR. Here's what they said:
Does that make sense to you? |
swift_library( | ||
name = "BuildkiteTestCollector", | ||
srcs = glob(["Sources/**/*.swift"]), | ||
module_name = "BuildkiteTestCollector", | ||
visibility = ["//visibility:public"], | ||
deps = [], | ||
) |
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.
Hi @marcosgriselli, sorry for the late reply, I've made some attempts and encountered some build issues. To address them, I ended up with the following configuration:
swift_library( | |
name = "BuildkiteTestCollector", | |
srcs = glob(["Sources/**/*.swift"]), | |
module_name = "BuildkiteTestCollector", | |
visibility = ["//visibility:public"], | |
deps = [], | |
) | |
swift_library( | |
name = "Core", | |
testonly = True, | |
srcs = glob(["Sources/Core/**/*.swift"]), | |
visibility = ["//visibility:public"] | |
) | |
swift_library( | |
name = "BuildkiteTestCollector", | |
testonly = True, | |
srcs = glob(["Sources/BuildkiteTestCollector/**/*.swift"]), | |
deps = [ | |
":Core", | |
], | |
) |
- Separating into two modules to resolve a problem where we have two files with the same name.
- Adding
testonly = True,
to workarounderror: no such module 'XCTest'
Would love your thoughts on this code, much appreciated! π
Furthermore, I'm curious about the contents of the WORKSPACE file. Given the code you've provided, it's currently empty. Should it include the workspace snippet we could copy from the release? Or, since this is a library, should it be left to the consumer to figure out?
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.
SGTM! testonly
is really necessary because it adds XCTest to search paths.
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.
This change looks good imo. Although this point is still valid in a bazel setup, expecting environment variables that change with every commit will break hermeticity, so extracting the uploading to a post bazel action might be the best course for folks with this setup.
The empty WORKSPACE file is actually expected by bazel to define the root of the project.
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.
Make sense π , just to understand it clearly, so #49 would still be essential for supporting Bazel, and this PR is just make it support the Bazel building process?
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.
Yes
π¬ Summary of Changes
Add bazel build support
π§Ύ Checklist
π Items of Note