Skip to content
This repository has been archived by the owner on Sep 15, 2021. It is now read-only.

Create bzl_library targets #124

Merged
merged 1 commit into from
Nov 11, 2020
Merged

Conversation

achew22
Copy link
Member

@achew22 achew22 commented Oct 29, 2020

This is to allow people who transitively depend on this repo to use
stardoc to generate documentation for their Bazel targets.

For the origin of this commit, please see
bazel-contrib/bazel-gazelle#760 (comment)
and the linked PRs in that thread.

This is to allow people who transitively depend on this repo to use
stardoc to generate documentation for their Bazel targets.
name = "java_repositories",
srcs = ["java_repositories.bzl"],
visibility = ["//visibility:public"],
# Temporarily omit @bazel_tools//... bzl files from the bzl_library until
Copy link
Member Author

Choose a reason for hiding this comment

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

The upstream change lives here: bazelbuild/bazel#12258

@@ -120,3 +107,9 @@ pkg_tar(
strip_prefix = "/",
package_dir = "federation/sample",
)

bzl_library(
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why buildifier thinks this should be at the bottom but it is very adamant about it. I can manually bump it back to the top if you'd like.

@@ -1,10 +1,9 @@
package(default_visibility=["//visibility:public"])
Copy link
Member Author

Choose a reason for hiding this comment

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

Entries for //tests/... can easily be excluded if you'd prefer they not exist, but they are technically importable by third parties since bzl files come with an implicit public visibility in the context of Bazel's load statements unless they live in a directory called private.

Copy link
Contributor

@aiuto aiuto left a comment

Choose a reason for hiding this comment

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

FYI: We are about to rewrite this entire project so it is useful, so this PR might soon be moot. In any case, I think it is a real bug that someone depending on the repo would see the internal code. We need distributions that have only what you need at runtime, the rule version list for a given Bazel version.

I'm not going to get a chance to properly evaluate this until sometime next week.

@Wyverald / @fweikert Can one of you look.

@achew22
Copy link
Member Author

achew22 commented Oct 29, 2020

FYI: We are about to rewrite this entire project so it is useful, so this PR might soon be moot.

This is a blocker for documentation generation right now for rules_go which depends on rules_cc which depends on bazel-federation.

In any case, I think it is a real bug that someone depending on the repo would see the internal code.

Given the example:

.
├── BUILD
├── Example.java
├── WORKSPACE
└── internal
    ├── BUILD
    └── MyInternalClass.java

//:example (a java_library) depends on //internal:my_internal_class (that was visibility = ["//:__subpackages__"]).

I would expect there to be a java_library for the //internal package, but no one outside the repo would be consuming it. Maybe you can elaborate on your concern here.

We need distributions that have only what you need at runtime, the rule version list for a given Bazel version.

I'm not sure what the action is here.

@aiuto aiuto requested a review from Wyverald October 29, 2020 20:58
@aiuto
Copy link
Contributor

aiuto commented Oct 29, 2020

Sorry. I was just talking about what should/will be rather than what is. I'll shut up because it does not help you get past your immediate problem.

Other people are taking over this work, so I don't think it is my choice to merge or not, but from what I see, I think it should be merged.

@achew22
Copy link
Member Author

achew22 commented Nov 3, 2020

Friendly ping on this

Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay. I don't think I know Federation's setup well enough to review this. I'm indeed working on a new system, but it would likely replace Federation, rather than modify it.

Nevertheless, I'll go ahead and approve this to unblock you (also since @aiuto essentially gave an informal approval above).

@achew22
Copy link
Member Author

achew22 commented Nov 11, 2020

@fweikert, given @aiuto's implicit and @Wyverald's explicit approval. Is there any chance we could merge this?

@fweikert fweikert merged commit 8ce885a into bazelbuild:master Nov 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants