Skip to content

Commit 0f25090

Browse files
cpsauerxinzhengzhang
authored andcommitted
Unify --file parsing logic
Promote multiple --file assert to log_error Why? It's a usage error we're communicating to the user, not a code invariant we're checking in debug mode. We could recover, but we don't, because we expect only automated usage and want to give clear feedback. More pythonic if Improve my --file flag messaging consistency Call library to escape regex Helps avoid edge cases! Also handles the most common special character, '.' Add an error to catch unsupported --file <file> form Tweak -- logic now that --file arguments could be after --file docs Make aquery spacing consistent Flip the if statement in constructor target_statment Use let to combine attr query Merge compile_commands.json if --file Small simplification: endswith already can take a tuple of possibilities (as elsewhere) Small simplification of header path for file flag. (This code section likely to be replaced later, though.) Fix my typo on --file= discoverable docs Some tweaks to merge Adds note about behavior, some edge case handling around other flags that might start with --file, permissions issues, etc.
1 parent a4a6a90 commit 0f25090

File tree

2 files changed

+35
-17
lines changed

2 files changed

+35
-17
lines changed

refresh.template.py

+33-17
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,9 @@
33
44
Interface (after template expansion):
55
- `bazel run` to regenerate compile_commands.json, so autocomplete (and any other clang tooling!) reflect the latest Bazel build files.
6-
- No arguments are needed; info from the rule baked into the template expansion.
6+
- No arguments are needed; info from the rule is baked into the template expansion.
77
- Any arguments passed are interpreted as arguments needed for the builds being analyzed.
8+
- The one exception is --file=<file_target>, which can be used to update commands for just one file. This is intended for programmatic use from editor plugins.
89
- Requires being run under Bazel so we can access the workspace root environment variable.
910
- Output: a compile_commands.json in the workspace root that clang tooling (or you!) can look at to figure out how files are being compiled by Bazel
1011
- Crucially, this output is de-Bazeled; The result is a command that could be run from the workspace root directly, with no Bazel-specific requirements, environment variables, etc.
@@ -828,14 +829,20 @@ def _get_commands(target: str, flags: str):
828829
# Log clear completion messages
829830
log_info(f">>> Analyzing commands used in {target}")
830831

832+
# Pass along all arguments to aquery, except for --file=
831833
additional_flags = shlex.split(flags) + [arg for arg in sys.argv[1:] if not arg.startswith('--file=')]
832-
file_flags = [arg for arg in sys.argv[1:] if arg.startswith('--file=')]
833-
assert len(file_flags) < 2, f"Only one or zero --file is supported current args = {sys.argv[1:]}"
834+
file_flags = [arg[len('--file='):] for arg in sys.argv[1:] if arg.startswith('--file=')]
835+
if len(file_flags) > 1:
836+
log_error(">>> At most one --file flag is supported.")
837+
sys.exit(1)
838+
if any(arg.startswith('--file') for arg in additional_flags):
839+
log_error(">>> Only the --file=<file_target> form is supported.")
840+
sys.exit(1)
834841

835842
# Detect anything that looks like a build target in the flags, and issue a warning.
836-
# Note that positional arguments after -- are all interpreted as target patterns. (If it's at the end, then no worries.)
843+
# Note that positional arguments after -- are all interpreted as target patterns.
837844
# And that we have to look for targets. checking for a - prefix is not enough. Consider the case of `-c opt` leading to a false positive
838-
if ('--' in additional_flags[:-1]
845+
if ('--' in additional_flags
839846
or any(re.match(r'-?(@|:|//)', f) for f in additional_flags)):
840847
log_warning(""">>> The flags you passed seem to contain targets.
841848
Try adding them as targets in your refresh_compile_commands rather than flags.
@@ -853,25 +860,22 @@ def _get_commands(target: str, flags: str):
853860
if {exclude_external_sources}:
854861
# For efficiency, have bazel filter out external targets (and therefore actions) before they even get turned into actions or serialized and sent to us. Note: this is a different mechanism than is used for excluding just external headers.
855862
target_statment = f"filter('^(//|@//)',{target_statment})"
856-
if (len(file_flags) == 1):
857-
# Strip --file=
858-
file_path = file_flags[0][7:]
859-
# Query escape
860-
file_path = file_path.replace("+", "\+").replace("-", "\-")
861-
# For header file we try to find from hdrs and srcs to get the targets
862-
if file_path.endswith('.h'):
863-
# Since attr function can't query with full path, get the file name to query
864-
head, tail = os.path.split(file_path)
865-
target_statment = f"attr(hdrs, '{tail}', {target_statment}) + attr(srcs, '{tail}', {target_statment})"
863+
if file_flags:
864+
file_path = file_flags[0]
865+
if file_path.endswith(_get_files.source_extensions):
866+
target_statment = f"inputs('{re.escape(file_path)}', {target_statment})"
866867
else:
867-
target_statment = f"inputs('{file_path}', {target_statment})"
868+
# For header files we try to find from hdrs and srcs to get the targets
869+
# Since attr function can't query with full path, get the file name to query
870+
fname = os.path.basename(file_path)
871+
target_statment = f"let v = {target_statment} in attr(hdrs, '{fname}', $v) + attr(srcs, '{fname}', $v)"
868872
aquery_args = [
869873
'bazel',
870874
'aquery',
871875
# Aquery docs if you need em: https://docs.bazel.build/versions/master/aquery.html
872876
# Aquery output proto reference: https://github.com/bazelbuild/bazel/blob/master/src/main/protobuf/analysis_v2.proto
873877
# One bummer, not described in the docs, is that aquery filters over *all* actions for a given target, rather than just those that would be run by a build to produce a given output. This mostly isn't a problem, but can sometimes surface extra, unnecessary, misconfigured actions. Chris has emailed the authors to discuss and filed an issue so anyone reading this could track it: https://github.com/bazelbuild/bazel/issues/14156.
874-
f"mnemonic('(Objc|Cpp)Compile',{target_statment})",
878+
f"mnemonic('(Objc|Cpp)Compile', {target_statment})",
875879
# We switched to jsonproto instead of proto because of https://github.com/bazelbuild/bazel/issues/13404. We could change back when fixed--reverting most of the commit that added this line and tweaking the build file to depend on the target in that issue. That said, it's kinda nice to be free of the dependency, unless (OPTIMNOTE) jsonproto becomes a performance bottleneck compated to binary protos.
876880
'--output=jsonproto',
877881
# We'll disable artifact output for efficiency, since it's large and we don't use them. Small win timewise, but dramatically less json output from aquery.
@@ -1068,6 +1072,18 @@ def _ensure_cwd_is_workspace_root():
10681072
for (target, flags) in target_flag_pairs:
10691073
compile_command_entries.extend(_get_commands(target, flags))
10701074

1075+
# --file triggers incremental update of compile_commands.json
1076+
if any(arg.startswith('--file=') for arg in sys.argv[1:]) and os.path.isfile('compile_commands.json'):
1077+
previous_compile_command_entries = []
1078+
try:
1079+
with open('compile_commands.json') as compile_commands_file:
1080+
previous_compile_command_entries = json.load(compile_commands_file)
1081+
except:
1082+
log_warning(">>> Couldn't read previous compile_commands.json. Overwriting instead of merging...")
1083+
else:
1084+
updated_files = set(entry['file'] for entry in compile_command_entries)
1085+
compile_command_entries += [entry for entry in previous_compile_command_entries if entry['file'] not in updated_files]
1086+
10711087
# Chain output into compile_commands.json
10721088
with open('compile_commands.json', 'w') as output_file:
10731089
json.dump(

refresh_compile_commands.bzl

+2
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ refresh_compile_commands(
4949
# exclude_headers = "external",
5050
# Still not fast enough?
5151
# Make sure you're specifying just the targets you care about by setting `targets`, above.
52+
# That's still not enough; I'm working on a huge codebase!
53+
# This tool supports a fast, incremental mode that can be used to add/update commands as individual files are opened. If you'd be willing to collaborate on writing a simple editor plugin invokes this tool on file open, please write in! (And see --file flag in refresh.template.py)
5254
```
5355
"""
5456

0 commit comments

Comments
 (0)