[Utils] Sanitize %t with PathSanitizingFileCheck #84843
Conversation
This adds sanitizing of the expansion of the `%t` temp dir to PathSanitizingFileCheck. Because the expansion of this path is different for each test case, lit.cfg cannot use the expanded version. Instead it relies on lit expanding the `%t` substring. This requires path normalization to occur in PathSanitizingFileCheck instead of lit.cfg. All strings matching the expansion of `%t` are now replaced with `TMP_DIR`. This is especially useful when source files are created in `%t` using `split-file`. Also sorts the patterns to sanitize based on length, to guarantee that patterns that are substrings of some other pattern are always tested after the longer patterns has already been tested. Otherwise the longer pattern may never match.
hnrklssn
requested review from
artemcm,
bnbarham,
hamishknight,
jckarter and
rintaro
as code owners
|
@swift-ci please smoke test |
| shell_quote(lit.util.abs_path_preserve_drive(config.swift_src_root).replace("\\", "/")), | ||
| shell_quote('%t'), | ||
| shell_quote(config.cmake_binary_dir), | ||
| shell_quote(config.swift_src_root), |
There was a problem hiding this comment.
This feels like its breaking developer setups. The abs_path_preserve_drive is critical to ensure that the correct drive is referenced
There was a problem hiding this comment.
normalize_if_path does the same thing but in PathSanitizingFileCheck, since %t can't be passed to abs_path_preserve_drive before test execution.
There was a problem hiding this comment.
Not sure I follow; I don't see the call to it there.
There was a problem hiding this comment.
This one `DEPCHANGE-DAG: SDKDependencies.swift` used to match because the expansion of the `%t` temp dir contained the substring "SDKDependencies.swift", but this is now sanitized to `TMP_DIR`.
This one `CHECK-NEXT: cxx-overlay-source-lookup.swift` used to match because the expansion of the `%t` temp dir contained the substring "cxx-overlay-source-lookup.swift", but this is now sanitized to `TMP_DIR`. The actual source file name is `client.swift`, created by `split-file` inside the `%t` directory.
Updates 3 additional tests broken by the TMP_DIR addition to PathSanitizingFileCheck.
This makes sure TMP_DIR is sanitized correctly on Windows despite output using `\\` as path separator.
|
@swift-ci please smoke test |
| // | ||
| // DEPCHANGE-DAG: SdkLib.swiftinterface | ||
| // DEPCHANGE-DAG: ExportedLib.swiftinterface | ||
| // DEPCHANGE-DAG: SDKDependencies.swift |
There was a problem hiding this comment.
@bnbarham Turns out this matcher doesn't actually match a file name, just a substring in the expansion of the %t path. There are other matchers in this file that do still match a file name like intended, so I'm not sure if this is just a mistake in the test, or indicative of a regression.
There was a problem hiding this comment.
Probably more of a @nkcsgexi, but this seems fine as the test is written - we're not building SDKDependencies.swift here so it makes sense it's not in the dependencies. Though I'm unclear if it was meant to be building SDKDependencies.swift here or not (ie. %s rather than - in the run line above)
| # literal (r'\\/') for our platform-dependent slash regex. | ||
| stdin = re.sub(re.sub(r'\\/' if sys.version_info[0] < 3 else r'/', | ||
| slashes_re, re.escape(pattern)), | ||
| slashes_re, re.escape(normalize_if_path(pattern))), |
There was a problem hiding this comment.
@compnerd This is the call to normalize_if_path
There was a problem hiding this comment.
Right, but I don't see the call to the function inside the implementation of normalize_if_path.
There was a problem hiding this comment.
Ah I see. It basically does the same thing, with some extra on top. Here's what the implementation of abs_path_preserve_drive looks like: https://github.com/llvm/llvm-project/blob/27d8441f8282c740903529d8a6b73401fc6c17fa/llvm/utils/lit/lit/util.py#L124
We only want to path normalize a string (and replace backslashes with forward slashes if on Windows) if it's actually a path. But checking if there's a path with that name doesn't work if the path is a substring of an actual path. Additionally check whether the dirname points to something on the file system. If it does, it's likely meant to be interpreted as a path.
|
@swift-ci please smoke test |
3 participants
This adds sanitizing of the expansion of the
%ttemp dir toPathSanitizingFileCheck. Because the expansion of this path is different
for each test case, lit.cfg cannot use the expanded version. Instead it
relies on lit expanding the
%tsubstring. This requires pathnormalization to occur in PathSanitizingFileCheck instead of lit.cfg.
All strings matching the expansion of
%tare now replaced withTMP_DIR. This is especially useful when source files are created in%tusingsplit-file.Also sorts the patterns to sanitize based on length, to guarantee that
patterns that are substrings of some other pattern are always tested
after the longer patterns has already been tested. Otherwise the longer
pattern may never match.