-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/file based filter todo1 #1
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
base: feature/file-based-filter
Are you sure you want to change the base?
Feature/file based filter todo1 #1
Conversation
Handling cases where input files are intermediates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for moving this forward! Quick inline comments below.
On aquery: I'm afraid I'm either not understanding or not seeing the bug you're talking about. As an example, I was running bazel aquery "mnemonic('(Objc|Cpp)Compile', allpaths(//API/C:iOS, let v = deps(//API/C:iOS) in attr(hdrs, 'DoubleBuffer.hpp', \$v) + attr(srcs, 'DoubleBuffer.hpp', \$v)))" --noinclude_aspects
and was getting the actions I'd expect
file_path = str(pure_path.relative_to(output_base)) | ||
else: | ||
# Treat pure_path as system absolute path | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really appreciate your tackling. But on a quick read, I think this part could use some polish along the lines we'd discussed.
A couple things that jump out:
We should probably still have an error for the case that's in-output-base but not external or execroot.
...
(Remember the backwards compatible relative_to helper, as above)
(I'm sure there's more I missed on a quick pass; I'm counting on you to experiment and track them down, and give it some solid manual testing.)
Probably also worth using subprocess.run instead of subprocess.check_output, just because of deprecation and to match the others? (Also, there's the encoding issue.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to ask if there are any other problems with the processing of paths besides subprocess.check_output? At present, I have modified these 3 calls, but I have not changed other places in the file
There is no problem to aquery like this. I went through a separate aquery to get the target who produced the input file. Reproduce
|
@@ -979,20 +979,52 @@ def _get_commands(target: str, flags: str): | |||
|
|||
|
|||
# Then, actually query Bazel's compile actions for that configured target | |||
target_statement = f'deps({target})' # TODO we should always be quoting targets when we splice them in. Let's use single quotes like with mnemonic, above. See slightly down in https://bazel.build/query/language#tokens | |||
target_statement = f"deps('{target}')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but as in the TODO, we should really be quoting targets everywhere!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also quote in other queries. But I found another problem in bazel query.
Because of the bug mentioned above I tried to use another query statement to return the middle_target_candidate.
f"filter({re.escape(candidate}$)"
but when the label starts with @
it will return empty realist... so I just remove the first @ symbol for workaround 6b098e8#diff-a1d7061df7c566a1f7656624ec608ad53dd3aff7a7789b9b6e4866a3b1616042R1016
Re aquery: Huh! Really not obvious to me what's going on there. Would you be down to report as an issue on bazelbuild/bazel and see if they help or confirm that it's a bug? By the way: Are you also seeing super slow GitHub downloads these past couple weeks? |
In my country like github, these are all blocked by default so I use vpn. But I did find that github has been hung up a few times recently and in most cases, it can be restored in half an hour.(I can't even tell if it's a github problem or my ladder problem) In terms of speed, because it is very slow, we have built mirrors for enterprise, so we can't perceive it |
Interesting! Thanks. Usually it's blazingly fast here--just not these last couple weeks. |
This is a pr that solves part of todo.
Fix timeout not working due to scope issue
Normalize input file to relative path
The following conditions were manually tested
Source file
srcs/app/xx/x.h
bazel info execution_root
/srcs/app/xx/x.hbazel info workspace
/srcs/app/xx/x.hexternal/zlib
1.2.13/crc32.h
1.2.13/crc32.hbazel info output_base
/external/zlibbazel info workspace
/external/zlib~1.2.13/crc32.hGenerated file
bazel-out//bin/external/_main
repo_deps_extbapis/xx/Ad.blrpc.hbazel info execution_root
bazel-out//bin/external/_mainrepo_deps_extbapis/xx/Ad.blrpc.hbazel-out//bin/srcs/app/xx/x_generate.h
bazel info execution_root
/bazel-out//bin/srcs/app/xx/x_generate.hSystem file
/Applications/Xcode.app/Contents//Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk/System/Library/Frameworks/UIKit.framework/Headers/UIKit.h
Handling cases where input files are intermediates
Looks like there's a bug in bazel aquery. Please help to see if there is a problem with the way I use it. If it really exists, I may only be able to query the result once, and then merge it and pass it to aquery.