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 bzlmod support + tests #1057

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Add bzlmod support + tests #1057

merged 1 commit into from
Sep 14, 2023

Conversation

zaucy
Copy link
Contributor

@zaucy zaucy commented Aug 28, 2023

Updated and refactored bazel support in preparation to submit to the bazel central registry https://registry.bazel.build/

Additionally I've added:

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (7178970) 100.00% compared to head (29102ee) 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1057   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          142       142           
  Lines        24424     24424           
=========================================
  Hits         24424     24424           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

test/BUILD.bazel Outdated Show resolved Hide resolved
BUILD.bazel Show resolved Hide resolved
README.md Show resolved Hide resolved
@skypjack skypjack self-assigned this Aug 30, 2023
@skypjack skypjack added enhancement accepted requests, sooner or later I'll do it build system requests or issues related to the build system labels Aug 30, 2023
.github/workflows/bazel-release-archive.yml Show resolved Hide resolved
.github/workflows/bazel.yml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@zaucy zaucy requested a review from skypjack September 9, 2023 22:59
@zaucy zaucy marked this pull request as ready for review September 9, 2023 23:00
@skypjack skypjack changed the base branch from master to wip September 12, 2023 06:39
Copy link
Owner

@skypjack skypjack left a comment

Choose a reason for hiding this comment

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

I'm ok with these changes. However, let me check a thing with you before merging.
I see a file within the src dir as well as many others all around the codebase, such as those within `test'.
Is there any way in bazel to centralize things and avoid this kind of pollution?
Not a problem on itself but I dislike the way cmake works already because it has the same problem and I prefer to keep dirs clean as much as possible. So, if there exists a way, I'd love to explore it, otherwise nvm and we can merge it as-is.
Thanks and sorry for bothering with my freaking obsessions. 😅

@zaucy
Copy link
Contributor Author

zaucy commented Sep 12, 2023

I understand, polluting the repo with files that you're not familiar with can be annoying :P

I added src/BUILD.bazel to avoid using the strip_include_prefix feature since it has a slight negative impact for developer experience. I can remove the file and use strip_include_prefix if it's annoying.

I can also consolidate the BUILD.bazel tests into test/BUILD.bazel instead if you'd like. It would make all the targets at the root of the test module which is fine, but not preferred for bazel users. If you want I can make that change.

Unfortunately with bazel I couldn't, for instance, put everything in 1 folder separate from everything. Bazel's build files really want to be where the sources are.

@zaucy
Copy link
Contributor Author

zaucy commented Sep 12, 2023

the current bazel test targets are this:

//entt/common:common
//entt/entity:component
//entt/entity:entity
//entt/entity:group
//entt/entity:handle
//entt/entity:helper
//entt/entity:observer
//entt/entity:organizer
//entt/entity:registry
//entt/entity:runtime_view
//entt/entity:sigh_mixin
//entt/entity:snapshot
//entt/entity:sparse_set
//entt/entity:storage
//entt/entity:storage_entity
//entt/entity:view
//entt/meta:meta_any
//entt/meta:meta_base
//entt/meta:meta_container
//entt/meta:meta_context
//entt/meta:meta_conv
//entt/meta:meta_ctor
//entt/meta:meta_data
//entt/meta:meta_dtor
//entt/meta:meta_func
//entt/meta:meta_handle
//entt/meta:meta_pointer
//entt/meta:meta_prop
//entt/meta:meta_range
//entt/meta:meta_template
//entt/meta:meta_type
//entt/meta:meta_utility

I could make them flat like I mentioned and add some prefix instead of using the folder so it's something like this:


//:entt_common_common
//:entt_entity_component
//:entt_entity_entity
//:entt_entity_group
//:entt_entity_handle
//:entt_entity_helper
//:entt_entity_observer
//:entt_entity_organizer
//:entt_entity_registry
//:entt_entity_runtime_view
//:entt_entity_sigh_mixin
//:entt_entity_snapshot
//:entt_entity_sparse_set
//:entt_entity_storage
//:entt_entity_storage_entity
//:entt_entity_view
//:entt_meta_meta_any
//:entt_meta_meta_base
//:entt_meta_meta_container
//:entt_meta_meta_context
//:entt_meta_meta_conv
//:entt_meta_meta_ctor
//:entt_meta_meta_data
//:entt_meta_meta_dtor
//:entt_meta_meta_func
//:entt_meta_meta_handle
//:entt_meta_meta_pointer
//:entt_meta_meta_prop
//:entt_meta_meta_range
//:entt_meta_meta_template
//:entt_meta_meta_type
//:entt_meta_meta_utility

Not my preference, but I can do that!

@skypjack
Copy link
Owner

Nah, not a problem then.
I guess I'll keep it intact as long as you're around, given that I know almost nothing about bazel. 😅
Then 🤷‍♂️ let's see, without any help from the community I'll probably drop the support, dunno.

@skypjack skypjack merged commit 01453b6 into skypjack:wip Sep 14, 2023
40 checks passed
@zaucy zaucy deleted the bzlmod branch September 14, 2023 07:00
@zaucy
Copy link
Contributor Author

zaucy commented Sep 14, 2023

I'll be around! We're using EnTT with bazel so it's a must for us :) I'm happy to maintain. I usually watch the repo for changes, but feel free to @zaucy if something comes up.

thanks for the review and merge!

zaucy added a commit to ecsact-dev/entt that referenced this pull request Sep 21, 2023
@skypjack
Copy link
Owner

@zaucy before I spend my day looking into the failure, do you happen to know already what the cause is for this error? 🙂

@zaucy
Copy link
Contributor Author

zaucy commented Dec 13, 2023

@skypjack should be fixed here #1098

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system requests or issues related to the build system enhancement accepted requests, sooner or later I'll do it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants