Skip to content

Conversation

broadwaylamb
Copy link
Contributor

@broadwaylamb broadwaylamb commented Jul 9, 2019

When absolute paths that needed to be substituted by PathSanitizingFileCheck contained characters that could be part of regexp syntax, substitution didn't actually take place.

For example, substitution like BUILD_DIR='/Users/johnsmith/swift-source/Ninja-RelWithDebInfoAssert+swift-DebugAssert' didn't result in anything because of the '+' in the path, which caused the test suite to fail.

This is an attempt to fix it.

This commit also adds the --dry-run flag to PathSanitizingFileCheck which was used to detect and debug this issue, but why not let it stay.

cc @drodriguez @compnerd

@broadwaylamb broadwaylamb changed the title [utils] Fix PathSanitizingFileCheck to support paths with weird chars [utils] Fix PathSanitizingFileCheck to support paths with weird characters Jul 9, 2019
@broadwaylamb broadwaylamb changed the title [utils] Fix PathSanitizingFileCheck to support paths with weird characters [utils] Fix PathSanitizingFileCheck to support paths with "weird" characters Jul 9, 2019
@drodriguez
Copy link
Contributor

Yes, it seems that I introduced a bug by not escaping the resulting pattern. str.replace() should only do literal matching and that's correct, but I think this will break the Windows compatibility. Let's check.

@swift-ci please test Windows platform

@drodriguez
Copy link
Contributor

(the Windows build has failing tests, but I will check the differences against the base branch)

@broadwaylamb broadwaylamb force-pushed the path-sanitizing-file-check-substitution branch from 7fc1755 to fb52743 Compare July 9, 2019 21:54
When absolute paths that need to be substituted by PathSanitizingFileCheck contained characters that could be part of regexp syntax, substitution didn't actually take place.

For example, substitution like BUILD_DIR='/Users/johnsmith/swift-source/Ninja-RelWithDebInfoAssert+swift-DebugAssert' didn't result in anything because of the '+' in the path.

This is an attempt to fix it.

This commit also adds the --dry-run flag to PathSanitizingFileCheck which was used to detect and debug this issue, but why not let it stay.
@broadwaylamb broadwaylamb force-pushed the path-sanitizing-file-check-substitution branch from fb52743 to 9fbdfde Compare July 9, 2019 21:56
@drodriguez
Copy link
Contributor

@swift-ci please smoke test

@drodriguez
Copy link
Contributor

@swift-ci please test Windows platform

@drodriguez
Copy link
Contributor

The Windows errors were present in master before this PR. Merging.

Thanks a lot!

@drodriguez drodriguez merged commit 9e4c054 into swiftlang:master Jul 10, 2019
@broadwaylamb broadwaylamb deleted the path-sanitizing-file-check-substitution branch July 10, 2019 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants