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.

Sign up for GitHub

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

Jump to bottom

Simplify Bazel #312

Merged
merged 2 commits into from
Nov 15, 2023
Merged

Simplify Bazel #312

merged 2 commits into from
Nov 15, 2023

Conversation

cpsauer
Copy link
Contributor

@cpsauer cpsauer commented Nov 14, 2023 •
edited
Loading

Please see #309 for context, but this sidesteps the bazel dependencies and cleans up the copts where they're not actually used.

Fixes #309

@cpsauer cpsauer mentioned this pull request Nov 14, 2023
Closed
zaucy
zaucy suggested changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@zaucy zaucy left a comment

Choose a reason for hiding this comment

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

If there's a way to not declare platforms in the test (and everyone upstream who uses bzlmod) that would be great.

test/BUILD.bazel Outdated
Comment on lines 9 to 10
"@bazel_tools//tools/cpp:clang-cl": _MSVC_FLAGS,
"@bazel_tools//tools/cpp:msvc": _MSVC_FLAGS,
Copy link
Contributor

Choose a reason for hiding this comment

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

These options don't work unless specifying platforms as far as I know. Example from my previous bzlmod PR https://github.com/Neargye/magic_enum/pull/254/files#diff-e13b51fb7feeb38d3fb7dee88724107c57fb3d4805068b5f8d57a2d0900fb0dc. It'll just fallback to the default.

e.g.

cl : Command line warning D9002 : ignoring unknown option '-std=c++17'

Followed by many errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah shoot. Sorry I missed that. The other one worked without platforms, I take it? Presumably the toolchains set the @bazel_tools//tools/cpp:compiler flag internally?

I think the right move, then, is to re-add the bazelmod rules_cc dependency for tests (only). Seems cleaner than duplicating the config settings and still accomplishes all the previous goals around consuming the library itself. I'll push up a commit that does just that.

@cpsauer cpsauer closed this Nov 14, 2023
@cpsauer
Simplify Bazel
6d8ef98 
Please see Neargye#309 for context, but this sidesteps the bazel dependencies and cleans up the copts where they're not actually used.
@cpsauer cpsauer reopened this Nov 14, 2023
@cpsauer cpsauer mentioned this pull request Nov 14, 2023
Merged
zaucy
zaucy approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@zaucy zaucy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested on one of my projects and it seems fine to me! 👍 (although I only tested on Windows)

test/MODULE.bazel
local_path_override(module_name = "magic_enum", path = "..")

bazel_dep(name = "rules_cc", version = "0.0.8")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe put the bazel dep back up top? :P

Copy link
Contributor Author

@cpsauer cpsauer Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'd reordered intentionally, thinking we might want the magic-enum dependency configuration all together for clarity that it's being overridden and quick-skimmability.

But I don't feel super strongly or anything--feel free to change if you feel strongly the other way? Or generally lmk if you think there's a different principle I should be absorbing here.

(I'm assuming from the approval that you don't want to block merging on this, yeah? Still v interested in what you have to say, ofc.)

@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 14, 2023

Thanks so much for testing @zaucy, and working so thoughtfully with me to make things even better. I really appreciate your being great to collaborate with!

All good to merge, I think, yeah?

@Neargye, thanks to you too for being so responsive and great!

Cheers,
Chris

@Neargye Neargye merged commit 016883d into Neargye:master Nov 15, 2023
18 checks passed
@Neargye
Copy link
Owner

Neargye commented Nov 15, 2023

@cpsauer @zaucy Thanks for help!

@Neargye Neargye added this to the v0.9.5 milestone Nov 15, 2023
@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 15, 2023

Thank you, Daniil!

@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 15, 2023

@Neargye, how imminent is v0.9.5, do you think? Trying to decide whether to switch to commit-by-commit to get things back working w/ Bazel vs backpin/waiting for the next release.

@cpsauer cpsauer deleted the no-dep-on-rules-cc branch November 15, 2023 23:18
@Neargye
Copy link
Owner

Neargye commented Nov 16, 2023

@cpsauer v0.9.5 is released

@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 16, 2023

Yay! Thanks :)

@Porkepix Porkepix mentioned this pull request Nov 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zaucy zaucy zaucy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.9.5
Development

Successfully merging this pull request may close these issues.

Bazel: Might be worth removing dependency on rules_cc
3 participants
@cpsauer @Neargye @zaucy