Skip to content

[windows] Allow only exe capitalization in tests. #27510

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 8, 2019
Merged

[windows] Allow only exe capitalization in tests. #27510

merged 1 commit into from
Oct 8, 2019

Conversation

drodriguez
Copy link
Contributor

@drodriguez drodriguez commented Oct 3, 2019 •
edited
Loading

Clang 8 or 9 seems to have changed from EXE to exe. Allow the new
capitalization (which is not important in Windows).

With these changes, there will still be failures in:

  • IRGen/multithread_module.swift: Failure during linking.
  • Driver/driver-compile.swift: An uppercased path is lowercased in the output.
  • Driver/options.swift: The swift.exe driver cannot find a valid stdlib (while swiftc.exe can).

@drodriguez drodriguez requested a review from compnerd October 3, 2019 01:03
@drodriguez
Copy link
Contributor Author

With #27475

@swift-ci please test Windows platform

@drodriguez
Copy link
Contributor Author

I don't think the code is being built with the previous PR, so it will fail building LLDB.

@drodriguez drodriguez mentioned this pull request Oct 3, 2019
Merged
@compnerd
Copy link
Member

compnerd commented Oct 3, 2019

@swift-ci please test Windows platform

@compnerd
Copy link
Member

compnerd commented Oct 3, 2019

FWIW, I'm in favour of just dropping the capital.

@compnerd
Copy link
Member

compnerd commented Oct 3, 2019

@swift-ci please smoke test

compnerd
compnerd approved these changes Oct 3, 2019
View reviewed changes
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Id prefer the "incompatible" change of using the lower case only.

@drodriguez
[windows] Allow EXE and exe capitalizations in tests.
70b1d91 
Clang 8 or 9 seems to have changed from EXE to exe. Allow both
capitalizations (which is not important in Windows) as a workaround.

Maybe in the future we can remove the uppercase option.
@drodriguez drodriguez force-pushed the windows-exe-capitalization branch from 02d32f6 to 70b1d91 Compare October 3, 2019 18:10
@drodriguez drodriguez changed the title [windows] Allow EXE and exe capitalizations in tests. [windows] Allow only exe capitalization in tests. Oct 3, 2019
@drodriguez
Copy link
Contributor Author

@swift-ci please test Windows platform

2 similar comments
@drodriguez
Copy link
Contributor Author

@swift-ci please test Windows platform

@drodriguez
Copy link
Contributor Author

@swift-ci please test Windows platform

@drodriguez
Copy link
Contributor Author

From the last Windows testing:

Failing Tests (3):
    Swift(windows-x86_64) :: Parse/confusables.swift
    Swift(windows-x86_64) :: IRGen/multithread_module.swift
    Swift(windows-x86_64) :: Driver/driver-compile.swift

I was expecting options.swift which fails for me locally, and I wasn't expecting confusables.swift, which doesn't. I think this can be merged and we can deal with the rest in different PRs as we find solutions.

PD: I also removed the uppercased EXE from the code.

@drodriguez
Copy link
Contributor Author

@swift-ci please smoke test

@compnerd
Copy link
Member

compnerd commented Oct 7, 2019

@swift-ci please smoke test Linux platform

@drodriguez
Copy link
Contributor Author

@swift-ci please test Linux platform

@swift-ci
Copy link
Contributor

swift-ci commented Oct 7, 2019

Build failed
Swift Test Linux Platform
Git Sha - 02d32f62e7863736b59231d08b257b7385b4671f

@compnerd
Copy link
Member

compnerd commented Oct 7, 2019

@swift-ci please smoke test Linux platform

@compnerd compnerd merged commit e903595 into swiftlang:master Oct 8, 2019
@drodriguez drodriguez deleted the windows-exe-capitalization branch October 8, 2019 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drodriguez @compnerd @swift-ci