my repo already has external directory...

[alkis]

but that does not point to external bazel-out/.../../../external. It would be nice if the name of this link could be configurable in refresh_compile_commands. Alternatively the script could be smarter to look in several possible paths for external: external, bazel-external, vendor, bazel-vendor.

[alkis]

I got this to work... Here's the diff:

diff --git a/refresh.template.py b/refresh.template.py
index 6486fab..8ee52db 100644
--- a/refresh.template.py
+++ b/refresh.template.py
@@ -221,7 +221,8 @@ def _all_platform_patch(compile_args: typing.List[str]):
     # For more context see: https://github.com/hedronvision/bazel-compile-commands-extractor/issues/21
     compile_args = (arg for arg in compile_args if not arg == '-fno-canonical-system-headers')
 
-    # Any other general fixes would go here...
+    ext = _all_platform_patch.external
+    compile_args = (ext + arg[8:] if arg.startswith('external/') else arg for arg in compile_args)
 
     return list(compile_args)
 
@@ -348,6 +349,8 @@ if __name__ == "__main__":
         # End: Command template filled by Bazel
     ]
     compile_command_entries = []
+    _all_platform_patch.external = "@EXTERNAL@"
+
     for (target, flags) in target_flag_pairs:
         compile_command_entries.extend(_get_commands(target, flags))
 
diff --git a/refresh_compile_commands.bzl b/refresh_compile_commands.bzl
index 5514c9d..a4d553b 100644
--- a/refresh_compile_commands.bzl
+++ b/refresh_compile_commands.bzl
@@ -36,7 +36,7 @@ refresh_compile_commands(
 ########################################
 # Implementation
 
-def refresh_compile_commands(name, targets = None):
+def refresh_compile_commands(name, targets = None, external = None):
     # Convert the various, acceptable target shorthands into the dictionary format
     if not targets: # Default to all targets in main workspace
         targets = {"@//...": ""}
@@ -47,7 +47,7 @@ def refresh_compile_commands(name, targets = None):
 
     # Generate runnable python script from template
     script_name = name + ".py"
-    _expand_template(name = script_name, labels_to_flags = targets)
+    _expand_template(name = script_name, labels_to_flags = targets, external = external)
     native.py_binary(name = name, srcs = [script_name])
 
 
@@ -58,13 +58,17 @@ def _expand_template_impl(ctx):
         output = script,
         is_executable = True,
         template = ctx.file._script_template,
-        substitutions = {"        {target_flag_pairs}": "\n".join(["        (%r, %r)," % p for p in ctx.attr.labels_to_flags.items()])} # Note, don't delete whitespace. Correctly doing multiline indenting.
+        substitutions = {
+            "        {target_flag_pairs}": "\n".join(["        (%r, %r)," % p for p in ctx.attr.labels_to_flags.items()]), # Note, don't delete whitespace. Correctly doing multiline indenting.
+            "@EXTERNAL@": ctx.attr.external,
+        }
     )
     return DefaultInfo(files = depset([script]))
 
 _expand_template = rule(
     attrs = {
         "labels_to_flags": attr.string_dict(mandatory = True), # string keys instead of label_keyed because Bazel doesn't support parsing wildcard target patterns (..., *, :all) in BUILD attributes.
+        "external": attr.string(default = "external"),
         "_script_template": attr.label(allow_single_file = True, default = "refresh.template.py")
     },
     implementation = _expand_template_impl

[cpsauer]

hey, @alkis! Thanks for trying the tool--and for your persistence. I'm sorry this name collision on external got in the way of easy setup. And thanks for raising a new and interesting case!

About this external directory in your workspace root: From what you're writing, I'm inferring it is a normal directory (not a symlink to some other location inside Bazel). And that it contains other things you want in your repo, maybe including code. Is that right?

If not, please correct me. But if so, my guess is that this collision is going to be a problem with Bazel much more generally than this tool, even if this tool is the one making the collision explicit. As a key example, Bazel uses the external subdirectory in its temporary, execution sandbox for external workspaces you've imported in WORKSPACE (ls $(bazel info execution_root) to see), and I'd think any bazel-used resources or code there might have weird behavior and collision risk. And I also don't think fixes, like the diff above, would be able to distinguish between files coming from the two external directories and know which to remap where. Basically, I'm concerned your external directory is going to collide with Bazel's build process more generally than this tool in a way we can't fix well.

If the above are right--and there isn't some other key thing I'm missing--I'm worried that supporting this problematic case well isn't a better overall solution than just renaming the problematic external directory itself to something else. That said, please tell me if I'm wrong or misunderstood!

Happy, certainly, to accept improvements to the external symlink. Like, for example, we could have an option to not require the symlink, and instead just rewrite the paths to where the symlink would have gone. (I think @lummax had started up on that.) But heads also that there're some subtle cases you'll run into that would also trip up your code above. (Consider, for example, the cases of -Iexternal/<path> and --sysroot=external/ flags). All that said, the external symlink is really nice for eliminating edge cases by making the source tree behave like a complete version of the execution sandbox. The current requirement of the external symlink is certainly not by accident, removing it takes some extra work and care.

All the best,
Chris

[alkis]

You are correct external is a real directory and it is ignored in .bazelignore. It is pointed to by a symlink for bazel to pick it up. I agree with you that renaming this directory and installing external symlink is the right thing to do.

The problem is that the project is in transition between 2 build systems and bazel, plus it is spread across OSS and internal codebases. Renaming the directory is... hard :-)

[cpsauer]

Ah! bazelignore prevents the collision. Makes sense. Thanks :)

As an aside: Just got off the phone with a friend at Databricks who said he'd interacted with you over creating an interview question. Small world :)

I hear you on the rename pain, and I'd like this to work well for you. Obviously you've got your patch above, and maybe that's enough (?). Probably worth at least handling additional flags (like -I, above) that are going to break things for you.

If you're looking to land a more general, maintained fix in upstream repo without too much work, I still think the best way forward would be to have a mode where it doesn't depend on an external at all, doing the same rewrite operation but to a fully resolved path. This would solve both this issue and some other use cases for little additional effort, while not encouraging people to overload things. That is, either the source tree looks like the compilation sandbox (w/ external) or we leave it untouched. We could also just automatically do the right thing :) If external already exists or the user specifies, massage the commands. Otherwise, create the external symlink.

Another benefit is that @lummax has already played around with this a fair bit. So some of the work is already done, even if it's not otherwise top of mind. (We've been focused on adding Windows support.) Would you want to join forces with him on it? Or maybe he'd want you to run with it while he works on Windows? (What do you think, @lummax?) IIRC, his initial commit was a little further along on some of the flag issues, though a couple remained. For reference, here was his first cut axivion@3d4ae54 (including some unrelated improvements), and the issue that motivated it in #14, along with some discussion.

[alkis]

Small world indeed 😄 .

The patch is sadly not enough. I tested this and for it to work I would need to inspect other args and modify include paths and perhaps link paths with some heuristics. Right now we are exploring renaming the upstream directory in which case we can rename external and get to a good state. If that doesn't work I like your idea of doing this in the script automatically instead of an option.

Either way, as you already pointed out in the docs, the best way to do this is with the magic symlink so that the repo looks like the sandbox as much as possible and commands can be used verbatim. The external directory is baked enough into bazel that doing renames suggested above would be an exercise in pain and lots of (undesirable) complexity.

[cpsauer]

:)

Sounds like we're on the same page, then. Let me know how it goes!

[I'm going to close this down for now, but if you want me to reopen or to discuss further, just drop me a note.]

All the best,
Chris

[cpsauer]

@alkis, I just wanted to say that I'm excited that you got //external moved in spark. Go Databricks and Spark! Hope this is serving you all well