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

Change all loads of bzl_library to have the absolute repository #184

Closed
wants to merge 7 commits into from

Conversation

aiuto
Copy link
Contributor

@aiuto aiuto commented Aug 21, 2019

Change all loads of bzl_library to have the absolute repository @bazel_skylib. This works around not having flipped the --incompatible_remap_main_repo flag. When that gets flipped we can unwind this change.

The exact problem we are working around is namespace resolution for providers which are global. As far as my limited mental model of the behavior goes, without the main repo remapping, we see the equivalent of //StarlarkLibraryInfo from within this library, but Stardoc sees @bazel_skylib//StarlarkLibraryInfo.

@fweikert With this in place, we can change the federation test to not require --incompatible_remap_main_repo. I was able to prove it, but not easily, because the mutual recursion of federation and skylib make it very difficult to test. More on that later.

bazelbuild/bazel#3115
bazelbuild/bazel#7130

aiuto added 2 commits August 21, 2019 17:20
@bazel_skylib. This works around not having flipped the
--incompatible_remap_main_repo flag. If that gets done
we can unwind this change.

The exact problem we are working around is namespace resolution
for providers which are global. Without the main repo remapping,
we see the equivalent of '//StarlarkLibraryInfo' from within
this library, but Stardoc sees '@bazel_skylib//StarlarkLibraryInfo'.

bazelbuild/bazel#3115
bazelbuild/bazel#7130
@aiuto aiuto requested review from fweikert and c-parsons August 21, 2019 21:38
@aiuto aiuto requested review from jin and laurentlb as code owners August 21, 2019 21:38
@c-parsons
Copy link
Collaborator

Just so I understand the effects of this change:

  1. Fixes uses of StarlarkLibraryInfo in cases where we have a "cyclic" repository dependency
  2. Requires that all users of bazel-skylib use it with the name @bazel_skylib.

Is (2) true, or is this just a valid workaround because bazel-skylib calls itself bazel_skylib in its own WORKSPACE?

@aiuto
Copy link
Contributor Author

aiuto commented Aug 22, 2019

Just so I understand the effects of this change:

  1. Fixes uses of StarlarkLibraryInfo in cases where we have a "cyclic" repository dependency

Yes. I don't think it is just cyclic, the problem of the no remap manifests in a lot of ways. For example, I had to do this in rules_pkg so that people using the rule from their own clients would find the helper.
https://github.com/bazelbuild/rules_pkg/blob/cb54c427343aa48c32e3c09ddcc8f6316cdbd5a6/pkg/pkg.bzl#L360
And, like here, if you don't bring in rules_pkg as @rules_pkg, then things don't work.

  1. Requires that all users of bazel-skylib use it with the name @bazel_skylib.
    Is (2) true?
    AFAICT, yes.

or is this just a valid workaround because bazel-skylib calls itself bazel_skylib in its own WORKSPACE?

I'm not sure what you mean. We can not call the repo @bazel-skylib because '-' is not allowed in repository names, so that renaming is mandatory.

@c-parsons
Copy link
Collaborator

I'm not sure what you mean. We can not call the repo @bazel-skylib because '-' is not allowed in repository names, so that renaming is mandatory.

If a user depends on this repository via a name other than @bazel_skylib, such as @my_bazel_skylib, will everything continue to work?

@aiuto
Copy link
Contributor Author

aiuto commented Aug 23, 2019

I'm not sure what you mean. We can not call the repo @bazel-skylib because '-' is not allowed in repository names, so that renaming is mandatory.

If a user depends on this repository via a name other than @bazel_skylib, such as @my_bazel_skylib, will everything continue to work?

bzl_library rules won't work. They will fail on StarlarkLIbraryInfo. But that pretty much

Without this change

  • if I load skylib as bazel_skylib and use @bazel_skylib//:bzl_library.bzl it works
  • if I load it as my_bazel_skylib it breaks.
  • if I try to generate stardoc for my bzl_library, it fails, unless I use --incompatible_remap_main_repo

With this change

  • if I load skylib as bazel_skylib and use @bazel_skylib//:bzl_library.bzl it works
  • if I load it as my_bazel_skylib it breaks.
  • if I try to generate stardoc for my bzl_library, it succeds regardless of --incompatible_remap_main_repo

I have a playground for this in bazelbuild/rules_pkg#81

Basically, because of the mutual recursion of skylib and stardoc, you can not load either under different names than expected. I'm willing to cast that in stone (and we can bump the version to 1.0.0 to indicate a breaking change) because with the federation we are going to force rules to come in as specific names. If you want to use my_bazel_skylib, you will be using that in spite of what the rest of the federation rules use.

@aiuto
Copy link
Contributor Author

aiuto commented Jul 7, 2020

Stale. We flipped the flag.

@aiuto aiuto closed this Jul 7, 2020
@aiuto aiuto deleted the noremap branch July 7, 2020 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants