Skip to content
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

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

Merged
merged 1 commit into from Jul 10, 2019

Conversation

Projects
None yet
3 participants
@drodriguez
Copy link
Collaborator

commented Jul 9, 2019

No description provided.

@drodriguez drodriguez requested a review from compnerd Jul 9, 2019

@drodriguez

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 9, 2019

@swift-ci please smoke test

@drodriguez

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 9, 2019

@swift-ci please test Windows platform

@compnerd

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

The Windows failure here has already been addressed.

@drodriguez drodriguez merged commit 4b0757b into apple:master Jul 10, 2019

2 of 3 checks passed

Swift Test Windows Platform Build finished. No test results found.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

@drodriguez drodriguez deleted the drodriguez:windows-fix-driver-sdk-test branch Jul 10, 2019

// 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

This comment has been minimized.

Copy link
@brentdax

brentdax Jul 10, 2019

Collaborator

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

This comment has been minimized.

Copy link
@drodriguez

drodriguez Jul 10, 2019

Author Collaborator

😢 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.

This comment has been minimized.

Copy link
@brentdax

brentdax Jul 11, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@drodriguez

drodriguez Jul 11, 2019

Author Collaborator

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
You can’t perform that action at this time.