Skip to content

[windows] Fix paths in test to work for Windows. #26037

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

Merged
merged 1 commit into from
Jul 10, 2019

Conversation

drodriguez
Copy link
Contributor

No description provided.

@drodriguez drodriguez requested a review from compnerd July 9, 2019 21:36
@drodriguez
Copy link
Contributor Author

@swift-ci please smoke test

@drodriguez
Copy link
Contributor Author

@swift-ci please test Windows platform

@compnerd
Copy link
Member

The Windows failure here has already been addressed.

@drodriguez drodriguez merged commit 4b0757b into swiftlang:master Jul 10, 2019
@drodriguez drodriguez deleted the windows-fix-driver-sdk-test branch July 10, 2019 00:27
// OSX: -L {{[^ ]*}}/Inputs/clang-importer-sdk/usr/lib/swift
// OSX: -rpath {{[^ ]*}}/Inputs/clang-importer-sdk/usr/lib/swift
// OSX: -L {{[^ ]*}}/Inputs/clang-importer-sdk{{/|\\\\}}usr{{/|\\\\}}lib{{/|\\\\}}swift
// OSX: -rpath {{[^ ]*}}/Inputs/clang-importer-sdk{{/|\\\\}}usr{{/|\\\\}}lib{{/|\\\\}}swift
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there something we could do here that's isn't…you know, this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢 I tried, but I cannot make it work.

In #25546 I tried to find all the things that look like Windows absolute paths in the output, and transform them to use POSIX slashes. That worked for many tests, but I had to revert in #25595 because it was also triggering false positives, and breaking other tests (check the comments in that last PR for a clear example).

In #24940 I have an approach that should be safer, but only applies to paths relative to SOURCE_DIR or BUILD_DIR. It would not had help in this case.

I tried to use a couple more approaches, but sadly FileCheck is not very flexible, so I don't have a lot of tools. PathSanitizingFileCheck seems like a good place to make the fix (and some fixes are there to avoid many problems in Windows), but it has to work in every case, and I haven't found the right solution yet. Ideally I want a solution that doesn't involve changing the tests from the POSIX paths, but at this point, I think I will be happy with a solution, even if it involves marking the paths somehow.

A solution that I would not like is going through all the tools and standardinzing on POSIX paths everywhere. But that's going to be finding 1000 needles in a very large haystack. This might not be possible and might mean changes in LLVM, which is unfortunate. It also means that Windows will show "foreign" paths (they still work in most cases, but they look wrong). I would actually prefer standardizing paths in Windows to not have both slashes, but give the right Windows slashes.

If you have any ideas, we are really interested in hearing them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a [[/]] pattern to FileCheck which expands to this? It'd be better than nothing.

Even if we don't adopt that specific solution, can you at least file a bug about the need to replace this with something better? I don't want this to just quietly spread through half of our test suite.

This might not be possible and might mean changes in LLVM, which is unfortunate.

Has LLVM solved this problem? I'm guessing they don't have {{/|\\\\}}s all over the place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: https://bugs.swift.org/browse/SR-11103

Adding that pattern to FileCheck might be possible, but sadly in some cases the test check for strings with their escaped characters, and in those cases the directory separators are escaped themselves, so some times one has to actually include a pattern like {{/|\\\\\\\\}} instead (the POSIX separators appears unescaped, while the Windows separator has to be look for escaped, but also one has to escape those slashes for the regular expression).

In LLVM test suite, when I looked at it, I found two things: they don’t check against paths as much as the Swift test suite does, and when they do, they add the patterns (for example https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/dsymutil/X86/basic-lto-linking-x86.test#L19).

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.

3 participants