Skip to content

Fix 2 Driver tests on Windows #20172

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
Nov 12, 2018

Conversation

gmittert
Copy link
Contributor

Most of the driver tests currently don't work on Windows, but these two
were relatively fixable.

  • specifically target OSX in order to produce generate-dSYM
  • alternatively match the full path of dsymutil, which on Windows is
    given as "C:\....\dsymutil.exe" verify-debug-info ...

Would it be useful to have a DEBUG_NONDARWIN etc that is the same as the DEBUG check but doesn't check for generate-dSYM and doesn't pass -target?

@jrose-apple
Copy link
Contributor

Thanks for looking at this. I don't think this is necessarily the right change for actions.swift, which is already XFAILed on Linux and FreeBSD. I'd rather split it up into the target-agnostic tests and the target-specific tests—and then yes, a DEBUG_NONDARWIN (or DEBUG_NON_DSYM) check would make sense.

@gmittert
Copy link
Contributor Author

gmittert commented Oct 31, 2018

So if I'm understanding correctly, leave actions.swift as is, but also xfail on windows, then add another test which doesn't check for dsym and doesn't xfail?

I mostly just fixed these so I'd have a passing test on Windows to add in a test for #20140, but if I were to go through and look at a few more, is one large PR easier, or is several each with a handful of tests easier?

@gmittert gmittert force-pushed the WindowsActionsTest branch 2 times, most recently from 83acc10 to f29b3ff Compare November 5, 2018 19:34
@gmittert
Copy link
Contributor Author

gmittert commented Nov 5, 2018

Updated to xfail on windows and split the actions test into dsym and non dsym.

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Sorry for the slow feedback!

@@ -165,5 +158,5 @@
// WHOLE-MODULE: 3: compile, {0, 1, 2}, object
// WHOLE-MODULE: 4: link, {3}, image

// RUN: %swiftc_driver -driver-print-actions -g %S/Inputs/main.swift %S/../Inputs/empty.swift %s -module-name actions -force-single-frontend-invocation 2>&1 | %FileCheck %s -check-prefix=WHOLE-MODULE -check-prefix=WHOLE-MODULE-DEBUG
// RUN: %swiftc_driver -driver-print-actions -g %S/Inputs/main.swift %S/../Inputs/empty.swift %s -module-name actions -force-single-frontend-invocation 2>&1 | %FileCheck %s -check-prefix=WHOLE-MODULE
// WHOLE-MODULE-DEBUG: 5: generate-dSYM, {4}, dSYM
Copy link
Contributor

Choose a reason for hiding this comment

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

We're no longer using this FileCheck line, so you can just remove it. Actually, the invocation isn't really adding anything either at this point.


// XFAIL: freebsd, linux, win32

// RUN: %swiftc_driver -driver-print-actions %s 2>&1 | %FileCheck %s -check-prefix=BASIC
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I don't think this file should include any of the actions that don't use -g, since the tests are the same. And then I think it's okay for this file to use an explicit -target x86_64-apple-macosx10.9 like you had originally, so that we can remove the XFAILs. That's a compromise, but I think it's a reasonable one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me, I've removed them.

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@jrose-apple jrose-apple self-assigned this Nov 8, 2018
@swift-ci

This comment has been minimized.

@swift-ci
Copy link
Contributor

swift-ci commented Nov 9, 2018

Build failed
Swift Test Linux Platform
Git Sha - 06f6f4ab1152bfb3a94689eb6a946ee895336aca

@jrose-apple
Copy link
Contributor

/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift/test/Driver/actions-dsym.swift:71:11: error: MULTI: expected string not found in input
// MULTI: 6: link, {1, 3, 5}, image
          ^
<stdin>:7:1: note: scanning from here
6: swift-autolink-extract, {1, 3, 5}, autolink
^
<stdin>:8:2: note: possible intended match here
7: link, {1, 3, 5, 6}, image
 ^

Looks like there's a -target missing somewhere.

@gmittert
Copy link
Contributor Author

gmittert commented Nov 9, 2018

/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift/test/Driver/actions-dsym.swift:71:11: error: MULTI: expected string not found in input
// MULTI: 6: link, {1, 3, 5}, image
          ^
<stdin>:7:1: note: scanning from here
6: swift-autolink-extract, {1, 3, 5}, autolink
^
<stdin>:8:2: note: possible intended match here
7: link, {1, 3, 5, 6}, image
 ^

Looks like there's a -target missing somewhere.

Oof yeah, that should be removed anyways since it's not being passed -g

Most of the driver tests currently don't work on Windows, but these two were relatively fixable.
- Moved the dsym checks to actions-dsym and ignore that on windows
- alternatively match the full path of dsymutil, which on Windows is
  given as "C:\....\dsymutil.exe" verify-debug-info ...
@gmittert
Copy link
Contributor Author

gmittert commented Nov 9, 2018

Alright, updated and tested locally against all of Windows, MacOS, and Linux.

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Nov 9, 2018

Build failed
Swift Test Linux Platform
Git Sha - 06f6f4ab1152bfb3a94689eb6a946ee895336aca

@swift-ci
Copy link
Contributor

swift-ci commented Nov 9, 2018

Build failed
Swift Test OS X Platform
Git Sha - 06f6f4ab1152bfb3a94689eb6a946ee895336aca

@jrose-apple jrose-apple merged commit d8e4a8f into swiftlang:master Nov 12, 2018
@jrose-apple
Copy link
Contributor

jrose-apple commented Nov 12, 2018

Thanks, sorry for all the delay!

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