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 support for bazel #56

Merged
merged 3 commits into from Oct 8, 2020

Conversation

FelixDuvalletKodiak
Copy link
Contributor

@FelixDuvalletKodiak FelixDuvalletKodiak commented Oct 2, 2020

Bazel support for the magic_enum repository would help others using
bazel integrate this library into their projects.

This PR adds a BUILD file which enables magic_enum to be imported
seamlessly into other WORKSPACE files without having to write a custom
BUILD file.

It also adds bazel binaries & tests to enable running them via bazel
locally:

bazel build //... 
bazel test //... 
bazel run //:example

Finally, bazel compilation artifacts are added to the gitignore file.

Bazel support for the magic_enum repository would help others using
bazel integrate this library into their projects.

This PR adds a BUILD file which enables magic_enum to be imported
seamlessly into other WORKSPACE files without having to write a custom
BUILD file.

It also adds bazel binaries & tests to enable running them via bazel
locally:

   export CC=clang ; bazel build //... ; bazel test //... ; bazel run //:example

Finally, bazel compilation artifacts are added to the gitignore file.
@Neargye Neargye self-requested a review October 2, 2020 17:56
BUILD.bazel Outdated
cc_library(
name = "magic_enum",
hdrs = ["include/magic_enum.hpp"],
strip_include_prefix = "include",
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested actually consuming this as an external dependency? I ask because IIRC strip_include_prefix doesn’t work in that case because there will be an extra external/ to contend with. I have not tested that your change doesn’t work, but the below modification definitely does.

Suggested change
strip_include_prefix = "include",
includes = ["include"],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's much nicer. Thanks for the suggestion, done.

.gitignore Outdated
@@ -48,3 +48,6 @@ install_manifest.txt
compile_commands.json
CTestTestfile.cmake
_deps

### Bazel build artifacts ###
bazel-*
Copy link
Contributor

Choose a reason for hiding this comment

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

[Nit] This will ignore bazel-* in all directories, but you actually only care about the symlinks created at the root of the repo.

Suggested change
bazel-*
/bazel-*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks.

@@ -0,0 +1,56 @@
licenses(["notice"])

exports_files(["LICENSE"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK bazel only has plans to support license files as part of the build, but since the repo provides it I am following (what I believe to be) an accepted convention of exporting it.
GoogleTest does this, for instance.

README.md Outdated
@@ -219,6 +219,17 @@ CPMAddPackage(
)
```

Bazel is also supported, simply add to your WORKSPACE file:
```
git_repository(
Copy link
Contributor

Choose a reason for hiding this comment

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

http_archive is preferred so I would use that as the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, done.

README.md Outdated
)
```
To use bazel inside the repository it's possible to do:
`export CC=clang ; bazel build //... ; bazel test //... ; bazel run //:example`
Copy link
Contributor

Choose a reason for hiding this comment

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

Specifying clang is confusing, since right below it says that multiple compilers are supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the comment, please let me know what you think.
I initially ran into an issue because the system-default compiler is not supported, but you're right that any supported can be used.

@FelixDuvalletKodiak
Copy link
Contributor Author

@HackAttack thanks for the review, please take another look when you get a chance.

@Neargye
Copy link
Owner

Neargye commented Oct 5, 2020

@FelixDuvalletKodiak Thank for your pr.

Let's wait for a review from @HackAttack , because I myself do not use bazel.

@Neargye Neargye added the enhancement New feature or request label Oct 5, 2020
@FelixDuvalletKodiak
Copy link
Contributor Author

@HackAttack friendly ping, are there further improvements we can make to this PR before merging it?

@Neargye
Copy link
Owner

Neargye commented Oct 8, 2020

@FelixDuvalletKodiak @HackAttack thanks!

@Neargye Neargye merged commit f3b4a01 into Neargye:master Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants