Resolve symlinks in all plist/yaml files under the classic codechecker rule #14
Conversation
test/BUILD
Outdated
| # Test for resolving virtual include prefix | ||
| cc_binary( | ||
| name = "virtual_include", | ||
| srcs = ["src/virtual_include.cc"], | ||
| deps = [":virtual_include_lib"], | ||
| includes = ["inc"], | ||
| ) | ||
|
|
||
| # Header lib for virtual include | ||
| cc_library( | ||
| name = "virtual_include_lib", | ||
| hdrs = glob(["inc/virtual_include_lib/*.h"]), | ||
| strip_include_prefix = "inc", | ||
| visibility = ["//visibility:public"], | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
I think the test for virtual include (strip_include_prefix) already exists at line 43
There was a problem hiding this comment.
A library target for a virtual include does exist. However, I needed an error to produce the output I test on, and didn't want to edit a file used by other tests; this is the reason for the new library target, instead of reusing the one on line 43.
There was a problem hiding this comment.
I think the new test is appropriate, we should leave the old one as-is.
furtib
changed the title
|
Let's split this into two! I will open a separate PR for the test. |
|
This has a unit test in: #25 |
furtib
force-pushed
the
fix_plist_paths
branch
2 times, most recently
from
2302891 to
c21fbaf
Compare
Test for #14 The problem was that paths in plist files didn't get resolved when there was an error in a header file. To reproduce the bug, I had to: - Add a header file that contains an error - Add a source file that includes said header - Add a rule for the new source fileí Depends on: #60 --------- Co-authored-by: Kristóf Umann <[email protected]>
furtib
force-pushed
the
fix_plist_paths
branch
from
c21fbaf to
0faac6f
Compare
furtib
changed the title
furtib
changed the title
furtib
force-pushed
the
fix_plist_paths
branch
3 times, most recently
from
fa97a16 to
681412c
Compare
furtib
force-pushed
the
fix_plist_paths
branch
3 times, most recently
from
9c35784 to
7eae847
Compare
Test for Ericsson#14 The problem was that paths in plist files didn't get resolved when there was an error in a header file. To reproduce the bug, I had to: - Add a header file that contains an error - Add a source file that includes said header - Add a rule for the new source fileí Depends on: Ericsson#60 --------- Co-authored-by: Kristóf Umann <[email protected]>
furtib
force-pushed
the
fix_plist_paths
branch
from
7eae847 to
c5f47bf
Compare
| fix_bazel_paths() | ||
| """ Resolve symlinks from local jobs, then try fixing path from remote executors """ | ||
| resolve_symlinks() | ||
| fix_bazel_paths() |
There was a problem hiding this comment.
Okay, whats the explanation here? Why the change of order? If possible, resolve, than fall back to replacing?
There was a problem hiding this comment.
Exactly, we should not shoot ourselves in the foot, if we can use the syslinks
| r"\/sandbox\/processwrapper-sandbox\/\S*\/execroot\/": "/execroot/", | ||
| START_PATH + r"\/worker\/build\/[0-9a-fA-F]{16}\/root\/": "", | ||
| START_PATH + r"\/[0-9a-fA-F]{32}\/execroot\/": "", |
There was a problem hiding this comment.
I love the new explanations, but I think I'm gonna need a bit more info as to whether we've truly reproduced these with the new regexes. In particular, by omitting START_PATH, you seem to produce absolute paths when symlinking fails, whereas the old regexes seem to produce relative paths always. Do we need to do this?
There was a problem hiding this comment.
We have not reproduced what these regex did. Only the middle one is reused, just without the START_PATH; this is most likely a remote executor-specific setting, our testing solution did not produce such a START_PATH. We should inquire more about these regexes. For now, it seems like for local runs, they are unnecessary; for remote jobs, it could be setup-dependent.
furtib
force-pushed
the
fix_plist_paths
branch
from
6ecd13b to
60fb29c
Compare
furtib
force-pushed
the
fix_plist_paths
branch
from
60fb29c to
6a8ef77
Compare
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
So to summarize, we're kind of stuck on this patch because much of the plist preprocessing depends on paths coming from buildbarn remote agents, which we don't have in the Github CI as of now. I suggest that we commit some buildbarn-generated-plists (as seen in #75, we can run it locally but not yet as a github action), and just do a plain python unit test on whether we can appropriately postprocess it.
furtib
force-pushed
the
fix_plist_paths
branch
from
6a8ef77 to
f038f51
Compare
3 participants
Why:
Paths in .plist (and yaml) files have not been correctly updated to point to the original file; they pointed to a /home/.cache/ folder.
What:
I have modified the CodeChecker wrapper Python script so as not to modify the file paths listed in the list files, and resolve the symlinks that way. Also I have removed a line disabling the symlink resolution if the checker is not calng-tidy.
Addresses:
#65
This test only fixes the single codechecker job rule, not the distributed one!