search

bazeltools / bazel-deps

Generate bazel dependencies for maven artifacts
MIT License
250 stars 122 forks source link

Label -> fromRoot: add // before empty root #251

Closed Reflexe closed 5 years ago

Reflexe commented 5 years ago

In a case of an empty third party directory, we'll get visilibity of ":subpackages" which will deny access from higher packages in the same third party directory. This is an attempt to fix that.

Reflexe commented 5 years ago

It is actually a part of a repostiroy_rule that runs the tool automatically from the workspace file (instead of running it after every in the dependencies).

Reflexe commented 5 years ago

By the way, I suggest you to take a look at https://github.com/Reflexe/bazel-auto-maven-libs

thundergolfer commented 5 years ago

@johnynek This might be buggy. After updating to last master, which includes this PR, we now get: 


--- a/third_party/jvm/org/scala_lang/BUILD
--
  | +++ b/third_party/jvm/org/scala_lang/BUILD
  | @@ -19,7 +19,7 @@ java_library(
  | java_library(
  | name = "scala_library",
  | exports = [
  | -        "@io_bazel_rules_scala_scala_library"
  | +        "@io_bazel_rules_scala_scala_library//"
  | ],
  | licenses = [
  | "notice"
  | @@ -34,7 +34,7 @@ java_library(
  | java_library(
  | name = "scala_reflect",
  | exports = [
  | -        "@io_bazel_rules_scala_scala_reflect"
  | +        "@io_bazel_rules_scala_scala_reflect//"
  | ],
  | licenses = [
  | "notice"
  | error: Dirty git status

Which blows up bazel.

Reflexe commented 5 years ago

Oops, didn't think about that use case. We could add an additional case instead that will return "//" in case of an empty segments and workspace name.

I'll fix that after work.

On Wed, 17 Apr 2019, 10:50 Jonathon Belotti, notifications@github.com wrote:

@johnynek https://github.com/johnynek This might be buggy. After updating to last master, which includes this PR, we now get:

--- a/third_party/jvm/org/scala_lang/BUILD

--

| +++ b/third_party/jvm/org/scala_lang/BUILD

| @@ -19,7 +19,7 @@ java_library(

| java_library(

| name = "scala_library",

| exports = [

| - "@io_bazel_rules_scala_scala_library"

| + "@io_bazel_rules_scala_scala_library//"

| ],

| licenses = [

| "notice"

| @@ -34,7 +34,7 @@ java_library(

| java_library(

| name = "scala_reflect",

| exports = [

| - "@io_bazel_rules_scala_scala_reflect"

| + "@io_bazel_rules_scala_scala_reflect//"

| ],

| licenses = [

| "notice"

| error: Dirty git status

Which blows up bazel.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/johnynek/bazel-deps/pull/251#issuecomment-483976982, or mute the thread https://github.com/notifications/unsubscribe-auth/AVzDKPcFmX5FLalUJfx8PwL_qBqInTEUks5vhtJJgaJpZM4cra2h .

Reflexe commented 5 years ago

@thundergolfer Work's done. Can you check it again the my latest commit at https://github.com/Reflexe/bazel-deps ? thanks.

thundergolfer commented 5 years ago

@Reflexe Thanks for getting onto a fix quickly. I'm not likely to test-drive your latest soon (easter holidays 😄).

I'm not completely familiar with this repo, but do we not have tests that could exercise this change?