Allow using workspace name in labels

[kchodorow]

Right now, to refer to a label "in this repository," you refer to it as //foo:bar. If the WORKSPACE defines workspace(name = 'repo'), perhaps we should also allow referring to it as @repo//foo:bar.

This came up in bazelbuild/rules_rust#7.

[PiotrSikora]

I would very much like to see this implemented...

I'm working on a modular project, with each 3rd-party module in its own workspace (i.e. @project, @moduleA, @moduleB, etc.). All modules need to depend on @project//:core, but when they are imported as external repositories, I cannot build @project//:main (which depends on @moduleA and @moduleB), because @project's workspace cannot resolve @project to itself.

Is self-referencing local repository an acceptable workaround for now? i.e.

workspace(name = "project")

local_repository(
    name = "project",
    path = __workspace_dir__,
)

I've built @project//:main using this solution as well as by replacing @project//:core with @//:core in modules, and while both binaries have the same filesize and work, their checksums are different, which worries me a bit.

Also, when using the self-referencing solution, project-2.params file lists libcore.a twice (once from main directory and once from external/project)... libcore.a is exactly the same in both cases, so hopefully linker is smart enough about it, but again, this worries me a bit.

[kchodorow]

Yeah, self-referencing local repo is the recommended workaround for now.

Just some notes about implementing this: there are two changes needed for this.

1. At the moment, execution root put the local workspace under $(basename workspace-dir), not the workspace name. I have a change in progress to fix this.
2. Then update labels in the "main" repository to be workspace-aware.

[PiotrSikora]

Hey Kristina, thanks for the notes... Hopefully workspace-aware labels in the "main" repository are only needed in the meantime, before this is properly fixed?

[kchodorow]

Right now labels either include a workspace name (e.g., when you say deps = ["@foo//bar"]), they use the default workspace, i.e., whatever workspace they happen to be in (e.g., //bar resolves to @foo//bar in @foo, but @baz//bar if it were in @baz), or the main workspace, which means they don't know their workspace name but are referring to the workspace where the build is happening. Right now the main workspace is required all over the place. I think there still might be some call for it after this change goes in, but hopefully it'll be needed in fewer places.

[damienmg]

This is done for a long time, no? @kchodorow?

[kchodorow]

@damienmg this has not been done, see my comment on #1866.

[steven-johnson]

The local_repository() workaround doesn't work (AFAICT) if you have targets that produce generated files, because there seems to be confusion over which genfiles paths are permitted (e.g., "bazel-out/local-fastbuild/genfiles/Halide.h" vs "bazel-genfiles/external/halide/Halide.h").

[kchodorow]

Reopening, since I just noticed that this was rolled back.

@steven-johnson, can you elaborate/give a more complete example? Using it on genrule output works for me.

[steven-johnson]

@kchodorow, I'm trying to track it down right now -- it may be a case of user error (mixing prefixed and non-prefixed paths), but I'm not sure yet. Stay tuned.

[steven-johnson]

What seems to be happening is:
-- there's a generated file ("Halide.h")
-- This can end up generated in two places:
-- blaze build :halide_h -> bazel-out/local-fastbuild/genfiles/Halide.h
-- blaze build @halide//:halide_h -> bazel-genfiles/external/halide/Halide.h

my internal target is depending on this via the second syntax (fake-external-repo hack); however, the include paths passed to the C++ compiler search bazel-out/local-fastbuild/genfiles first, so it finds the "other" Halide.h... which it shouldn't see.

why the extra include paths are present is not clear to me. again, I'm assuming I'm doing something wrong, but it could also be just that this is a weird hack that I've found the broken edges of...

UPDATE: this seems to be a bit weird in repeatability; I think I've gotten my local repo into a state that sorta-kinda-works, but getting there felt pretty flaky. Ordinarily that wouldn't be acceptable, but given that this is very temporary (since I'm assuming the rollback can be unrolled soon) I may just live with it rather than trying to debug it.

[PiotrSikora]

Any update after the rollback?

[steven-johnson]

I presume this is still unfixed in 0.4.0?

[kchodorow]

Still unfixed. The go rules have updated the change that caused the rollback, though, so I should be able to roll forward when I get a chance.

[steven-johnson]

Looks like this is still going to be unfixed in 0.4.1?

[kchodorow]

Ugh, yes, sorry.

[jart]

Once this is fixed, I would recommend deprecating @//... labels since they break when another repository depends on that repository.

[steven-johnson]

So is this closed-fixed or closed-unfixed? (I tried building Bazel from tip and trying this out in a local repo and either it's still broken or I'm doing something wrong...)

[steven-johnson]

Update: as of ce7c4de, whether this works or not depends on how I invoke bazel test.

Say I have a test with a dep like this:

# This rule is in the @halide//test/generator package
cc_test(
    name = "gpu_object_lifetime_aottest",
    deps = ["@halide//test/common:gpu_object_lifetime_tracker"],
    # irrelevant details omitted
)

If I invoke via bazel test @halide//test/generator:gpu_object_lifetime_aottest then all is well.

If, however, I am in the toplevel directory, and am lazy, and omit the @halide, i.e. bazel test //test/generator:gpu_object_lifetime_aottest, I fail with

Target '@halide//test/common:gpu_object_lifetime_tracker' is not visible from target '//test/generator:gpu_object_lifetime_aottest'

[damienmg]

Argh obviously because the labels are not equivalent. I would call for fixing it by actually making both label equivalent instead of a creating a local repo pointing to the main one.

Keeping the release blocker tag so we will include it in the release, not really blocking though.

[steven-johnson]

Can someone update the status of this? AFAICT it's still broken; whether it's intended to be fixed for 0.5 or not is not clear.

[damienmg]

@kchodorow: do you have the bandwidth to do it?

[kchodorow]

Sorry, missed the update from @steven-johnson. I'm taking a guess that @halide//test/common:gpu_object_lifetime_tracker isn't globally visible (correct me if I'm wrong). For the near future, the case you ran into is WAI: Bazel does not "dedup" //test and @halide//test, so it thinks that they're different repositories. //test/generator:etc isn't publicly visible, so it cannot be used from a different repository.

[steven-johnson]

I'm taking a guess that @halide//test/common:gpu_object_lifetime_tracker isn't globally visible

Correct: it's restricted to @halide//test:__subpackages__

For the near future, the case you ran into is WAI

:-( That's unfortunate. I hope it will be improved at some point.

[hlopko]

Hello Kristina, is this still expected to be fixed in 0.5? Is this a release blocker?

[kchodorow]

It is not a release blocker. The remaining issues will not be fixed in time for 0.5.

[hlopko]

Hi Kristina, thanks for the info and the move to 0.6!

[steven-johnson]

Cycling back to this after many months: I'm assuming this isn't gonna be fixed anytime soon?

[hlopko]

@dslomov @damienmg

[dslomov]

Duplicate of #7130