Skip to content

Add bazel configuration #1251

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

Merged
merged 1 commit into from
Apr 4, 2023
Merged

Conversation

keith
Copy link
Member

@keith keith commented Jan 19, 2023

This allows bazel users to depend on these libraries directly through the SwiftSyntax repo instead of everyone having to provide their own duplicate BUILD files. Since swift-syntax is pretty uniform this is setup using a simple bazel macro that uses the conventions of swiftpm to setup each library. This also includes a bazel target for always building a library with optimizations which can be useful if you want the improved performance and aren't trying to debug swift-syntax itself.

Build instructions:

brew install bazelisk
bazelisk build ...

@keith
Copy link
Member Author

keith commented Jan 19, 2023

For context through an offline conversation with @ahoppen we decided I should submit a PR for wider discussion. Currently users are encouraged to use this wrapper repo https://github.com/keith/swift-syntax-bazel/ which includes the same BUILD file (and previously automatically statically linked the C++ library for you). I can continue maintaining that repo if this feels like too much overhead, but I think there is some value in centralizing this as I know quite a few companies who have written tools based on swift-syntax that are built with bazel. This would also be nicer for adding swift-syntax to bazel's central dependency registry as opposed to pointing to a repo like mine.

We could also enable this on CI if we wanted to validate it, but in this PR I've chosen to act like the LLVM bazel setup which means it will be updated with fixes async but also not block folks who don't care about bazel.

@shepting
Copy link

This would be useful for us at Airbnb.

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to me to have this Bazel configuration here in the repository. Should the README be updated with Bazel build instructions, or is the presence of the Bazel build files sufficient for most users?

@keith keith force-pushed the ks/add-bazel-configuration branch from 7d364e4 to f498529 Compare February 16, 2023 00:57
@keith
Copy link
Member Author

keith commented Feb 16, 2023

I added a small blurb to the README with basic instructions. It's pretty vanilla configuration so overall it should be pretty natural for folks to include

@keith
Copy link
Member Author

keith commented Feb 16, 2023

@swift-ci please test

@keith keith marked this pull request as ready for review February 16, 2023 00:58
@keith keith requested a review from ahoppen as a code owner February 16, 2023 00:58
@keith
Copy link
Member Author

keith commented Mar 21, 2023

@ahoppen @DougGregor can you review 🙏🏻

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies for the long delay in responding here. This is a tricky area for us, because we are already maintaining and testing two distinct build systems (SwiftPM and CMake) that are necessary for the project. The primary maintainers do not feel like they have the capacity to maintain support for a third build system, both due to lack of experience with Bazel as well as ongoing maintenance of additional Continuous Integration to ensure that the Bazel build remains healthy.

That said, we recognize the benefits to the Swift community of having Bazel support in this repository, so we are willing to try this out. We can merge in this Bazel configuration, but we want to clearly call out that you are responsible for its ongoing maintenance (as well as any co-maintainers you'd like to nominate), including responding to any issues raised against Bazel support in this repository. I've made some suggestions in the code review. If in the future the Bazel configuration is not being maintained well---for example, reported issues with it are going unaddressed for long periods of time---we will deprecate and remove it from the repository. It's possible that someone could bring up community-hosted CI to catch Bazel issues more quickly when they arise.

@DougGregor
Copy link
Member

(Pinging @keith in case you missed the above, sorry again for the delay)

This allows bazel users to depend on these libraries directly through
the SwiftSyntax repo instead of everyone having to provide their own
duplicate BUILD files. Since swift-syntax is pretty uniform this is
setup using a simple bazel macro that uses the conventions of swiftpm to
setup each library. This also includes a bazel target for always
building a library with optimizations which can be useful if you want
the improved performance and aren't trying to debug swift-syntax itself.
@keith
Copy link
Member Author

keith commented Apr 3, 2023

Thanks for the review! Those conditions are totally reasonable. We can add more folks to the list if the issue count gets out of hand, but I don't expect it will.

@keith keith force-pushed the ks/add-bazel-configuration branch from a5283eb to 0a73615 Compare April 3, 2023 22:37
@keith
Copy link
Member Author

keith commented Apr 3, 2023

@swift-ci please test and merge

@keith
Copy link
Member Author

keith commented Apr 3, 2023

@swift-ci please test

1 similar comment
@DougGregor
Copy link
Member

@swift-ci please test

@DougGregor
Copy link
Member

@swift-ci please test Windows

@DougGregor
Copy link
Member

@swift-ci please clean test Windows

@DougGregor
Copy link
Member

All set! Merging. @keith would you mind cherry-picking this to the release/5.9 branch?

@DougGregor DougGregor merged commit 0535623 into swiftlang:main Apr 4, 2023
@keith keith deleted the ks/add-bazel-configuration branch April 4, 2023 04:25
@keith
Copy link
Member Author

keith commented Apr 4, 2023

Thanks! #1488

(I believe I have access here I think it was a blip earlier when starting CI failed for me)

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