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

External Include Paths Point into Unstable Execroot Instead of Stable External Directory #2766

Closed
cpsauer opened this issue Jul 8, 2021 · 12 comments
Assignees
Labels
product: IntelliJ IntelliJ plugin stale Issues or PRs that are stale (no activity for 30 days) topic: bazel Bazel integration (external repositories, aspects, flags, etc) topic: external dependencies type: bug

Comments

@cpsauer
Copy link
Contributor

cpsauer commented Jul 8, 2021

Hello wonderful Bazel folks,

TL;DR / summary: C++ header finding--and probably more--breaks for external libraries when you run a build command that uses a different set of external libraries. This issue can be fixed by having paths go through <outputBase>/external/, rather than the current <outputBase>/execroot/__main__/external/.

Discovery context: I'd noticed this after initially making the same mistake when writing a VSCode<>Clangd<>Bazel integration. I think this has the same root cause as #1256 --a deeper fix here might allow removal of the code that patched it in 81afc2c. A similar problem has also caused issues in Tulsi.

What's going wrong in more detail:
When the plugin gets paths to external libraries from Bazel, they usually point into the execution root's external folder, the temporary sandbox of symlinks reconfigured for each build. This leads leads the plugin to point the IDE into the temporary sandbox for external header search paths (and other things, I'd assume).

That's a trap. When you run a build that doesn't use the same set of external workspaces (like for example, a build for one of several targets in the project), external paths start breaking--and the red squiggles come out to indicate their displeasure. The temporary build sandbox, the execution root ("execroot") has gotten torn down and reconfigured, so some paths no longer exist. You can work around this by re-syncing, but it's annoying to have building break editing--feels like clearly undesirable behavior.

You can see that this problem is happening by opening, say, an NDK file in ASwB and looking at the header include paths. You can see the header include paths for a given file in Android Studio by "Show Compiler Info" (via Help→Find Action, aka CMD+SHIFT+A). You'll see paths containing /private/var/tmp/_bazel_<username>/<hash>/execroot/__main__/external, and verify that the symlinks those paths go through broken if you build some other part will get broken if a build that doesn't use the external workspace in question.

The fix: Those files live more durably inside /private/var/tmp/_bazel_<username>/<hash>/external, so the execroot-based paths should be rewritten to point there, just as we'd do for paths in the execroot that point into the normal workspace. That way you aren't relying on the symlinks in execroot that often get deleted out from under you!

Thanks for the work you do!
Chris
(ex-Googler)

P.S. This is parallel to a lingering Bug in Tulsi: bazelbuild/tulsi#164 [Now resolved.]
P.P.S. If someone's taking a serious look at this, it'd be good to check that there isn't a parallel problem with generated files.

@cpsauer cpsauer changed the title external execroot External Include Paths Point into Unstable Execroot Instead of Stable External Directory Jul 8, 2021
cpsauer added a commit to hedronvision/bazelbuild-intellij that referenced this issue Jul 9, 2021
This prevents header include path breakage when the execroot is reconfigured on rebuild.
See bazelbuild#2766
@cpsauer
Copy link
Contributor Author

cpsauer commented Jul 9, 2021

I've now fixed this in my working fork. The commit (plus fixup) are backlinked above and below if the Bazel folk to use the code. (My understanding from the README is that you'd always like issues first rather than PRs.)

And if anyone using the plugin wants the fix (and others) the meantime, you can grab the plugin from:
https://github.com/hedronvision/bazelbuild-intellij

cpsauer added a commit to hedronvision/bazelbuild-intellij that referenced this issue Jul 18, 2021
…r include path breakage when the execroot is reconfigured on rebuild. See bazelbuild#2766
cpsauer added a commit to hedronvision/bazelbuild-intellij that referenced this issue Apr 26, 2022
This prevents header include path breakage when the execroot is reconfigured on rebuild.
See bazelbuild#2766
cpsauer added a commit to hedronvision/bazelbuild-intellij that referenced this issue Apr 26, 2022
…r include path breakage when the execroot is reconfigured on rebuild. See bazelbuild#2766
@sgowroji sgowroji added type: feature request product: IntelliJ IntelliJ plugin topic: bazel Bazel integration (external repositories, aspects, flags, etc) topic: external dependencies awaiting-maintainer Awaiting review from Bazel team on issues labels Nov 1, 2022
@sgowroji sgowroji added the more-data-needed Awaiting response from issue author label Nov 1, 2022
@sgowroji
Copy link
Member

sgowroji commented Nov 1, 2022

Thank you @cpsauer for sharing the above request. Could you Please share the PR's, I can arrange it for review.

cpsauer added a commit to cpsauer/intellij that referenced this issue Nov 1, 2022
cpsauer added a commit to cpsauer/intellij that referenced this issue Nov 1, 2022
@sgowroji sgowroji removed the more-data-needed Awaiting response from issue author label Nov 1, 2022
@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 1, 2022

Wow! Hi @sgowroji. Sure. I thought there was no chance I'd ever hear back from you on this.

Here's the PR #4063
I just cherrypicked the above commits and then took a shot at fixing the tests. Hopefully that works. Took me a bit to load this back into my head, since it's been a good while.

As evidence that I'm not full of it: Since filing, I fixed this over at tulsi, too, and got that merged (see linked issue above). Plus, I built hedronvision/bazel-compile-commands-extractor, which provides this functionality for vscode.

@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 1, 2022

Couple more notes before we decide that that PR fully closes this:

  1. The fix to Alternative to jar cache for solving "red code" #1256, linked above, looks dicy. Seriously, that blaze-out patch just covered bugs, imo, since you could easily have a deeply nested directory named external and it'll munch all those subdirectories. We should merge this and then you guys should have JarCache.java call into this code instead if it doesn't already.
  2. The same problematic solution was copy-pasted into BlazeGoPackage.java, sadly. I'd recommend fixing the same way as above.
  3. I've got at least one other C++ project-breaking bug over in the fork I maintain. Please please take a peek and I'd be delighted to finally get to move on from that fork. https://github.com/hedronvision/bazelbuild-intellij

@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 15, 2022

@sgowroji, is a review forthcoming on these? Checking in, since it's been a couple weeks.

@sgowroji
Copy link
Member

Thank you @cpsauer for patience. We have shared the details to the engineer who will assist you soon on the above issue.

@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 29, 2022

Friendly bump! Checking in since you'd asked for a PR for review and it's been a month.
Thanks for all you do.

[Btw, I also happened to be browsing the gazelle source, and noted that they also handle external correctly, under output_base, as per the PR.

@mai93 mai93 assigned tpasternak and unassigned mai93 Jan 9, 2023
cpsauer added a commit to cpsauer/intellij that referenced this issue Jan 31, 2023
…putBase)

Based on tpasternak's good feedback.
Please see bazelbuild#2766 and bazelbuild#4063 for context.
cpsauer added a commit to cpsauer/intellij that referenced this issue Jan 31, 2023
…putBase)

Based on tpasternak's good feedback.
Please see bazelbuild#2766 and bazelbuild#4063 for context.
cpsauer added a commit to cpsauer/intellij that referenced this issue Jan 31, 2023
…putBase)

Based on tpasternak's good feedback.
Please see bazelbuild#2766 and bazelbuild#4063 for context.
@Starkiller4011
Copy link

C++ user here looking for this to be implemented soonest so I can get rid of those annoying red squiggles. Thanks.

@cpsauer
Copy link
Contributor Author

cpsauer commented Feb 11, 2023

Maintainers:
On labels, this is definitely a bug and not a feature request, IMO, since it's unintended behavior of existing functionality rather than new functionality.
And please don't close after the resolution of #4063. Unresolved sub-issues in #2766 (comment)

@sgowroji sgowroji added type: bug and removed awaiting-maintainer Awaiting review from Bazel team on issues labels Feb 11, 2023
tpasternak pushed a commit that referenced this issue Feb 13, 2023
* Resolve external paths to their workspaces
Please see #2766 for context

* Switch external path resolution implementation to getFileRootedAt(outputBase)
Based on tpasternak's good feedback.
Please see #2766 and #4063 for context.
@cpsauer
Copy link
Contributor Author

cpsauer commented Feb 14, 2023

@Starkiller4011, heads that the general/C++ fix was just merged in.

[Others, still please don't close bc buggy code for Jars, Go, (and maybe others) still exists.]

@github-actions
Copy link

Thank you for contributing to the IntelliJ repository! This issue has been marked as stale since it has not had any activity in the last 6 months. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-maintainer". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Aug 16, 2023
@github-actions
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: IntelliJ IntelliJ plugin stale Issues or PRs that are stale (no activity for 30 days) topic: bazel Bazel integration (external repositories, aspects, flags, etc) topic: external dependencies type: bug
Projects
Development

No branches or pull requests

5 participants