Setting `--experimental_no_product_name_out_symlink` with a custom `--symlink_prefix` removes `bazel-out`, breaking paths that go through it

[lukasoyen]

When setting --symlink_prefix on the commandline or .bazelrc, the bazel-out symlink might not exist or be somewhere else. But extract.py currently assumes bazel-out exists and generates a compile_commands.json file containing many references to it.

There seems to be no way of accessing the symlink_prefix via an environment variable or similar, so the refresh_compile_commands() might need an additional argument.

[cpsauer]

For any others reading this later, I'm updating my top comment with a suggested workaround based on our new understanding:

I'd suggest disabling --symlink-prefix (and --experimental_no_product_name_out_symlink if relevant), to regain bazel-out, fixing things.

Having bazel-out--and the external symlink from the README--is really worthwhile. They make it easy for you to see your outputs--and external dependencies. And they also make your source tree have the same directory structure as the build sandbox, making all of our debugging easier and eliminating lots of edge cases.

If you really don't like having those symlinks, read on to see how you could write a fix, but heads that it'll be tricky. See also #65.

---

Hi, Lukas! Thanks for giving the tool a try and pointing out an interesting case I hadn't considered. Sorry things aren't working out of the box. Let's figure it out :)

I've been trying to reproduce this one over here--and the behavior I'm seeing is that even with --symlink_prefix=whatever or --symlink_prefix=, bazel-out still gets created on run, keeping the paths valid. (Which would check out against, e.g. bazelbuild/bazel#3796. I'm doing, for example, bazel run :refresh_all --symlink_prefix=foo on both bazel latest and bazel rolling, and also trying putting it in .bazelrc under aquery and build.). You're for sure seeing otherwise? If so, anything else I should know or try?

If it does persists, I think this is the easiest way to get things working:
Given what you say about not being able to access symlink_prefix, this one seems like it might be better worked around, rather than doing duplicate configuration in the rule.
What about just manually creating the bazel-out symlink to point through your custom named symlink, similar to what we did with external in the README? (I presume you can't remove the flag and that it serves some essential function. Would love to learn what you're using it for!)

Thanks!
Chris

P.S. I see Bremen as your location. I've been there, and have some family relatively nearby :)
P.P.S. I'll get to that other one soon. Requires a little more doing, and it's my bed time.

[lukasoyen]

Hey, sorry you are right, my setup above was incomplete. We actually have build --symlink_prefix=custom/prefix --experimental_no_product_name_out_symlink in our .bazelrc (running on bazel 4.2.1).

We are in a process of migrating a code-base using make and gprbuild/msbuild to bazel. There are some
idiosyncrasies we have to maintain while doing the migration, so we set --symlink_prefix and --experimental_no_product_name_out_symlink. While removing the latter flag would be an option, I'd prefer not to confuse the developers with yet another weird directory/symlink. Maybe I have to weaken that requirement if we get to the point where the external symlink is necessary. We'll see.

Windows is next on the TODO list for VSCode-bazel integration, so expect some updates soon.

[cpsauer]

Aha! That would explain it.

Up to you on whether to have the symlink! I think it's well worth a symlink or two to be able to see Bazel's outputs and external dependencies--and to have the source tree identically to (a full version of) Bazel's sandbox for easy debugging. But that's your call.

If you definitely don't want the symlink(s), I think the easiest route would be to do an automated substitution of bazel-out with the absolute path equivalent when the bazel_out symlink is not present. While I'd continue to advise just leaving the symlink, I'd be delighted to accept a PR!

Some thoughts that occurred on the implementation, in case they're handy and you want to go that route:

• I was originally thinking quick call to sed at the end of refresh.sh.template would be a simple and fast way to do it, "s///g"'ing bazel-out, for $(bazel info output_path)
  • There's a little trickiness on the match:
    • We'll want to not match places where bazel-out is already a component of an absolute path--for example when Bazel emits fdebug-prefix-map.
    • And the path will need some escaping to play nice with sed.
      • Handy ref link for that if you did want to do it https://unix.stackexchange.com/questions/32907/what-characters-do-i-need-to-escape-when-using-sed-in-a-sh-script
    • And you'd want to match bazel-out/ to avoid issues with files like "bazel-output-test.cc", as a hypothetical example.
• Another good option would be to do it directly in extract.py, running bazel info output_path from the bazel_workspace_dir and checking for symlink existence, before matching and replacing.
• If you did one of the above, would you mind also writing the parallel case for external?

---

I'll edit this issue to reflect our new understanding. And I'll close it down for now, thinking of disabling that flag as the recommended workaround. But happy to reopen--or just work with you on that PR--if you'd like.

[lukasoyen]

Just as an update, currently WIP https://github.com/axivion/bazel-compile-commands-extractor/tree/python-refresh.py

[cpsauer]

Oh, great! Thanks for working on making this better and more cross-platform. I also really appreciate your tossing up a WIP for early communication. That's awesome!

Giving it a read! I'll tap out thoughts that occur in case they're helpful.
(Seems like these changes are both for Windows support and for this issue, but I'll reply here unless you say otherwise. I'd also be super happy with this as a WIP/draft PR if that's easier for you.)

• Looks like the first step was to shift things out of a shell-script driver. I'm certainly down with eliminating bash, sed, and friends as dependencies. The shell script was basically just for fast iteration and templating, and has probably outlived its purpose. Sed BSD/Linux compatibility did indeed require care, you're right!
  • You'll find I'm eager to learn the "whys" as I go, so I can continue to make good decisions around them :)
    • Here, I'd kinda assumed that most Windows Bazel users would have Bash/sh_binary set up. Sounds like that's wrong?
      • (Windows isn't yet a dev platform for us--as you might have guessed from the code--so I'm inexperienced in the world of Bazel Windows. Would appreciate any insights you have.)
  • I wonder if a good way of making things more modular will emerge as you keep going here. It was kinda nice to keep driving/templating separate, but certainly not essential.
    • I can imagine a template just for settings? (Not required or anything, just spinning ideas.)
  • Overall a fan of pulling logic into the way better programming language that is Python :)
  • Yikes--looks like I forgot to remove the TODOs in refresh_compile_commands.bzl. I'll hold off to not create merge conflicts for you, but feel free to kill 'em. Sorry about that. I had indeed checked the edge cases they were intended to remind me of.
  • I'd be inclined to prefer docstrings to interface comments, but maybe I've missed a good reason to do otherwise!
  • I love the substitution of the comment character, so it's valid python either way. Nice. shlex is also a great learning for me.
• Looks like this issue was up next. Great!
  • Love the your care with the code and the docs. Thank you!
  • Thanks also for doing external at the same time.
  • I think there might be a couple more tricky replacement cases here that we should handle. Some examples from my memory of compile_command.json that I've seen: -Ibazel-out/<...>/bin/external/com_google_protobuf/objectivec -Iexternal/google_toolbox_for_mac
    • Perhaps there's also windows path trickiness here.
  • I'd love it if we could lean towards auto-configuring.
    • For example, how about detecting if bazel-out exists -- after calling a command that would create it -- and automatically applying the substitution if it does not?
    • And perhaps the same for external? Another good solution could be to create it unless a flag is passed.
    • (I'm not super up to speed with the symlink situation on Windows.)
• 👍🏻 Having a setting to not extract headers. Good stuff.
  • Maybe let's move the skipping logic out a function call or two? The dataclass doesn't seem obviously better to me, but I trust you.
• On the uniquing case: I'm guessing you're thinking about solving the case where two separate targets lead to two aquery calls with duplicate entries?

Anyway, I'll pause for now. But thank you! I really appreciate your rolling up your sleeves to make things better.

[lukasoyen]

Here, I'd kinda assumed that most Windows Bazel users would have Bash/sh_binary set up. Sounds like that's wrong?

Maybe, we currently don't. And in fact https://docs.bazel.build/versions/main/windows.html#using-bazel-build-without-bash talks about one can use bazel without bash. It seems they but some effort into setting up rules so that they don't require bash.

I can imagine a template just for settings? (Not required or anything, just spinning ideas.)

Sure, that is possible. But might be require some fiddling in how python imports other files. And would require to declare these files as py_library() or as additional sources for the py_binary(). I would go the other way and teach refresh.py how to parse the command line (while keeping the templating around to support both use-cases).

I'll hold off to not create merge conflicts for you, but feel free to kill 'em.

Go ahead, I'll rebase.

I'd be inclined to prefer docstrings to interface comments, but maybe I've missed a good reason to do otherwise!

I agree.

Perhaps there's also windows path trickiness here.

Yes, I've already stumbled upon /Isome-include-path on Windows.

On the uniquing case: I'm guessing you're thinking about solving the case where two separate targets lead to two aquery calls with duplicate entries?

Yes I thought about it. But I would like to keep things simple and post PRs incrementally.