Reduce bazel API surface#733
Conversation
There was a problem hiding this comment.
This would prohibit usage of mw::service outside of our module.
Target service does not include the targets that you hide.
There was a problem hiding this comment.
Yes - but I don't think we are ready that it can be used? can it?
There was a problem hiding this comment.
Yes - but I don't think we are ready that it can be used? can it?
There was a problem hiding this comment.
Config management is migrating to S-CORE. They need this already.
There was a problem hiding this comment.
This would prohibit usage of mw::service outside of our module.
Target //score/mw/service does not include the targets that you hide.
There is no other public target in this package that would expose these symbols.
| visibility = [ | ||
| "//visibility:public", # platform_only | ||
| ], | ||
| visibility = ["//score/mw/com:__subpackages__"], |
There was a problem hiding this comment.
@bharatGoswami8 please verify whether this target has to be publicly visible.
There was a problem hiding this comment.
yes, it should be and as discussed in yesterday's CFT, we are going to move the public interfaces / APIs outside ofimplfolder.
| "//score/mw/com/gateway:__subpackages__", | ||
| "//score/mw/com/impl:__subpackages__", | ||
| "//score/mw/com/impl/bindings/lola:__subpackages__", | ||
| "//score/mw/com:__subpackages__", |
There was a problem hiding this comment.
This smells. There is no usage in //score/mw/com:__subpackages__.
Also with this not being publicly visible, I'm struggling how a user should actually create the mocks he requires for his unit tests...
@bemerybmw maybe you can check this?
There was a problem hiding this comment.
Not sure what you mean? e.g. score/mw/com/gateway/gateway_application is using it?
There was a problem hiding this comment.
Hmm missed that one then. But still, how are outside users to use our mocks? I might be missing something here, but to me this currently looks like we are accidentally hiding a feature from our users.
| @@ -19,155 +10,23 @@ | |||
| //quality/compiler_warnings:default_link_flags | |||
| //quality/compiler_warnings:treat_warnings_as_errors | |||
| //quality/coverage:enable_llvm_coverage_for_death_tests | |||
There was a problem hiding this comment.
Should not be publicly visible IMHO.
b4918c3 to
201e13a
Compare
castler
left a comment
castler
left a comment
There was a problem hiding this comment.
Answering review comments
There was a problem hiding this comment.
Yes - but I don't think we are ready that it can be used? can it?
There was a problem hiding this comment.
Yes - but I don't think we are ready that it can be used? can it?
| "//score/mw/com/gateway:__subpackages__", | ||
| "//score/mw/com/impl:__subpackages__", | ||
| "//score/mw/com/impl/bindings/lola:__subpackages__", | ||
| "//score/mw/com:__subpackages__", |
There was a problem hiding this comment.
Not sure what you mean? e.g. score/mw/com/gateway/gateway_application is using it?
| //score/mw/service/backend/mw_com:provided_service_builder | ||
| //score/mw/service/backend/mw_com:provided_service_decorator | ||
| //score/mw/service/backend/mw_com:single_instantiation_strategy | ||
| //score/mw/service:factory | ||
| //score/mw/service:provided_service | ||
| //score/mw/service:provided_service_container | ||
| //score/mw/service:proxy_future |
There was a problem hiding this comment.
I have a problem with these, since we already have active users.
https://github.com/eclipse-score/config_management/blob/438d7e72dcb6f5cd90d7135b037150e6281eca48/score/config_management/config_daemon/code/app/details/BUILD#L82-L83
https://github.com/eclipse-score/config_management/blob/438d7e72dcb6f5cd90d7135b037150e6281eca48/score/config_management/config_daemon/code/factory/details/BUILD#L50-L51
https://github.com/eclipse-score/config_management/blob/438d7e72dcb6f5cd90d7135b037150e6281eca48/score/config_management/config_provider/code/config_provider/factory/BUILD#L20
4ce813e to
bfd7143
Compare
github-project-automation
Bot
moved this from In Progress
to Done
in COM - Communication FT
Reduce overly broad visibility declarations across BUILD files so targets are internal by default unless intentionally public. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Without this bazel will treat the module_integration_test as part of this module, which can cause problems in formatting (or public API checks)
4 participants
This will reduce the exposed targets on a big scale, to only those targets that really should make up our public API.
If this commit breaks you - you are depending on things that are not meant for public usage.