bazelbuild / tulsi

An Xcode Project Generator For Bazel
http://tulsi.bazel.build
Apache License 2.0
548 stars 123 forks source link

Header include paths for external rules don't work #164

Closed michaeleisel closed 3 years ago

michaeleisel commented 4 years ago

I have an external rule, objc_library(includes = [...], ...), and objc_library rules that depend on it that need those include paths. however, i see a list of $(TULSI_BWRS)/bazel-tulsi-includes/x/x/external/MyRule/... and $(TULSI_WR)/bazel-tulsi-includes/x/x/external/MyRule/... when in fact the correct path seems to be $(TULSI_BWRS)/bazel-tulsi-includes/x/x/external/MyRule/...

tinder-maxwellelliott commented 4 years ago

Is this ticket correct? $(TULSI_BWRS)/bazel-tulsi-includes/x/x/external/MyRule/... is listed twice. Is there another path you meant to type there?

michaeleisel commented 4 years ago

one starts with TULSI_BWRS, the other with TULSI_WR

tinder-maxwellelliott commented 4 years ago

I understand now, thanks for clarifying.

cpsauer commented 4 years ago

Same issue: Things seem to be badly broken with the external header integration. [Though like @tinder-maxwellelliott the first time around, I'm not understanding exactly what @michaeleisel is saying about the paths.]

What I'm seeing: All external headers are not being found by Xcode. That is, #includeing headers loaded from external workspaces lead to 'whatever.h' file not found errors in the editor. (Things build fine with bazel and the build button.) Corollary: No autocomplete for those external types. Header Search Paths indeed don't point to the headers. Looking at the targets generated in the project file, in the Header Search Paths, I see paths similar to, but not identical to what @michaeleisel describes. In particular, for each external dependency, I see, e.g. $(TULSI_WR)/external/boost and $(TULSI_BWRS)/bazel-tulsi-includes/x/x/external/boost. But there's no external folder in the workspace root (TULSI_WR), nor in $(TULSI_BWRS)/bazel-tulsi-includes/x/x/, which includes only the path to the Info.plist for the main target (!?). Update: The latter no longer seems to show up in my latest builds.

Is there something I'm missing? Is anyone else getting external files to work? <- See workaround below

cpsauer commented 4 years ago

Update: For anyone else encountering this, we worked around the issue by linking the external folder into the workspace. Concretely, just run ln -s bazel-<WORKSPACE_NAME>/../../external ., and you should be good to go.

Things are working impressively well after that!

tinder-maxwellelliott commented 4 years ago

Update: For anyone else encountering this, we worked around the issue by linking the external folder into the workspace. Concretely, just run ln -s bazel-<WORKSPACE_NAME>/external ., and you should be good to go.

Things are working impressively well after that!

Is this working for generated files as well? i.e. files that are created via genrules

cpsauer commented 4 years ago

@tinder-maxwellelliott, good point. Haven't gotten a chance to yet--and have a couple things I need to punch off the list first. Would you be down to check genrules and report back?

tinder-maxwellelliott commented 4 years ago

I will verify this in an example project. Thank you

On Thu, Nov 19, 2020 at 1:40 AM cpsauer notifications@github.com wrote:

@tinder-maxwellelliott https://github.com/tinder-maxwellelliott, good point. Haven't gotten a chance to yet--and have a couple things I need to punch off the list first. Would you be down to check genrules and report back?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/tulsi/issues/164#issuecomment-730164249, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANQS7NV6RZZFNNWPFWPWPCLSQS4W3ANCNFSM4P5DHZUQ .

tinder-maxwellelliott commented 4 years ago

@cpsauer It appears that even with this symlink genfiles are not appearing in a Tulsi project. Example project can be seen here https://github.com/bazelbuild/examples/compare/master...tinder-maxwellelliott:tulsi_bazel_gen_issues

cpsauer commented 4 years ago

Wait, @tinder-maxwellelliott, is that a test to see if in-workspace genfiles are being picked up? Just to make sure we aren't talking past each other: I was intending the above as a workaround for files from external workspaces (originally not thinking about the generated case). Like for loading external libraries like boost from the WORKSPACE file, for example.

tinder-maxwellelliott commented 4 years ago

@cpsauer Sorry for the confusion, I simply thought that this could possibly also fix the issue of generated source files not appearing in the generated Xcode project https://github.com/bazelbuild/tulsi/issues/149.

cpsauer commented 3 years ago

Hey all, update the workaround above (edited to minimize confusion).

It should really be ln -s bazel-<WORKSPACE_NAME>/../../external ., not ln -s bazel-<WORKSPACE_NAME>/external . Why? The latter folder is re-symlinked on every build, while the former seems to hold all external dependencies. Without the change you'll start missing headers if you do other builds that don't use them. With the new one, that shouldn't happen.

cpsauer commented 3 years ago

@steeve, @thii, I was looking through the change (GitHub emailed me). First and foremost, thank you!

I also think there's a potential issue with fixing external headers this way--and I wanted to ask you about it, if that's all right. I'm looking for it, only because I originally made the same mistake in my workaround symlink.

The PR above changed the external paths to point into <outputBase>/execroot/__main__/external, right, via BWRS?

If so, I think Xcode will fail to find external includes after you run a build that uses a different set of external workspaces. (Imagine as an example, generating a project with Tulsi, editing some files, running some tests or a building one of several binary targets, and then having external header include paths become invalid.) At least that's what I was seeing when I was working around this with symlinks (as above).

Why? My understanding is that the execroot gets torn down before every build and relinked to include just the external workspaces used during that next build. So pointing into the execroot for header searching is likely to break unexpectedly; the symlinks in execroot often get deleted out from under you!

TL;DR/summary: I think we want to search in <outputBase>/external, not <outputBase>/execroot/__main__/external. I'd made the same mistake when working around the issue--see the commend previous to this one.

Apologies if I'm wrong, and thanks for thinking about it, Chris

steeve commented 3 years ago

@cpsauer you are absolutely right, I actually found out the patch isn't complete (sources are missing, I had a wrong symlink dangling).

However, I guess that would require tulsi to create a new symlink, directly to the output_base rather than the execroot.

You do get away with it if you build the target first, of course (which you need to do anyway since some files are generated, like modulemaps at least).

cpsauer commented 3 years ago

Yeah agreed. On the symlink, I'm skeptical that we ever want to point Xcode into execroot?

Where should we go from here, @steeve?

steeve commented 3 years ago

By the way, here is a following patch https://github.com/bazelbuild/tulsi/pull/229

I don't have much time to look at it, however, we could be pointing the more permanent execroot from the output_base, but I'm not sure it's trivial to get.

thii commented 3 years ago

Pointing Xcode to <output base>/external or <execution root>/external (which is <output base>/execroot/<main workspace name>/external) is debatable. I would say the latter is the more "correct" location since it only contains repositories that are part of the build (that just happened), and because the former can contains outdated repositories. But since we would want Xcode indexing to work with all the files all the time, <output base>/external seems more reasonable.

cpsauer commented 3 years ago

Hey all--finally got around to fixing this in #255. Would love your help getting it in!

I do think that it's important we go through output base, rather than execroot. The current behavior is that building any one target breaks the external file references and include paths for all the others that don't perfectly overlap with it. That's no good.

[And more generally, pointing into temporary sandboxes (like execroot) rather than accumulated caches seems like a big trap that'll cause more pain the more we do it! There are a bunch of bugs (like, e.g. generated file breakage) that look to me like they descend from this. But one fix at a time.]

Thanks so much for your kind, thoughtful, open-mindedness throughout this issue :)

cpsauer commented 3 years ago

@thii, any chance I could get you to help the PR through? Or @steeve, any guidance?

cpsauer commented 3 years ago

Wahoo! Great to put this one to rest.