Skip to content

[Windows] Fix windows breakage caused by lit.cfg change #33062

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
Jul 23, 2020
Merged

[Windows] Fix windows breakage caused by lit.cfg change #33062

merged 1 commit into from
Jul 23, 2020

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented Jul 22, 2020 •
edited
Loading

Caused by a change in: #32903

@artemcm artemcm requested a review from compnerd July 22, 2020 23:41
@artemcm
Copy link
Contributor Author

artemcm commented Jul 22, 2020

@swift-ci test

compnerd
compnerd reviewed Jul 22, 2020
View reviewed changes
test/lit.cfg Outdated
if platform.system() == 'Linux':
stdlib_dir += config.target_sdk_name + "/" + run_cpu
if platform.system() == 'Linux' || platform.system() == 'Windows':
stdlib_dir += config.target_sdk_name + path_separator + run_cpu
Copy link
Member

Choose a reason for hiding this comment

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

Using os.path.join is better IMO, though you could just keep this as is I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot better, thanks for the pointer.

@artemcm
Copy link
Contributor Author

artemcm commented Jul 22, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 870e80d2d40abba242b6d0651bb833f1b0308b6c

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 870e80d2d40abba242b6d0651bb833f1b0308b6c

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

@swift-ci Please test Windows platform

@compnerd
Copy link
Member

@swift-ci please test Windows platform

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

@swift-ci Please test OS X platform

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

Looks like the original issue was slightly different, it is referring to an "Unrecognized escape code" inside the json map, not referring to incorrect path separators.

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - e42b8f780736ce60953ae7d59b719e31b41d5026

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - e42b8f780736ce60953ae7d59b719e31b41d5026

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

@swift-ci please test Windows platform

@compnerd
Copy link
Member

compnerd commented Jul 23, 2020 •
edited
Loading

I believe that there are two categories of failures, some related to the stdlib and others related to the path encoding.

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

I believe that there are two categories of failures, some related to the stdlib and others related to the path encoding.

I believe the path encoding failure is caused by the fact that on Linux and macOS, having a comma after the last element of a JSON list is somehow considered to be a valid termination sequence, but on Windows it is not.

@compnerd
Copy link
Member

Commas after the last element in a list are not valid json: https://www.json.org/json-en.html

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

Actually it seems that having any line ending with a comma is not valid on Windows? Not necessarily even after the last element in a list...

@compnerd
Copy link
Member

More likely its line endings ... Windows uses \r\n rather than \n.

@compnerd
Copy link
Member

%ononesupport_module ... that is going to have \ in it which is an escape character. The string you are encoding is the cause of the issue.

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

It is created as:
ononesupport_module = os.path.join(stdlib_dir, "SwiftOnoneSupport.swiftmodule")
and the corresponding line in the lit test that uses the substitution ends with ":
// RUN: echo "\"modulePath\": \"%ononesupport_module\"" >> %/t/inputs/map.json
Are you referring to the \ used to escape the "?

The error seems to be pointing directly at:

T:\swift\test-windows-x86_64\ScanDependencies\Output\explicit-module-map-forwarding-module.swift.tmp/inputs/map.json:10:2: error: Unrecognized escape code

},

 ^

Which is separating second and third list elements in the generated JSON list...

@compnerd
Copy link
Member

compnerd commented Jul 23, 2020 •
edited
Loading

The path that is generated from os.path.join there will contain \ which JSON will interpret as an escape sequence even in the quoted string.

From the logs:

: 'RUN: at line 29';   echo "\"modulePath\": \"t:\swift\lib/swift/windows/SwiftOnoneSupport.swiftmodule\"," >> T:/swift/test-windows-x86_64/ScanDependencies/Output/explicit-module-map-forwarding-module.swift.tmp/inputs/map.json

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

I'm confused. Here's the most recent failure log: https://ci-external.swift.org/job/swift-PR-windows/5448/consoleText

The resulting path generated by os.path.join is seen in the log in:

$ ":" "RUN: at line 29"
$ "echo" ""modulePath": "t:\swift\lib\swift\windows\x86_64\SwiftOnoneSupport.swiftmodule""

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

Ah I see it now in:

: 'RUN: at line 29';   echo "\"modulePath\": \"t:\swift\lib\swift\windows\x86_64\SwiftOnoneSupport.swiftmodule\"" >> T:/swift/test-windows-x86_64/ScanDependencies/Output/explicit-module-map-forwarding-module.swift.tmp/inputs/map.json

@compnerd
Copy link
Member

I have a fix, I'll upload it in a second.

@compnerd
Copy link
Member

#33066 should take care of it

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - b1c97a1eddb2ff90b14a91044f4196c1479dc836

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - b1c97a1eddb2ff90b14a91044f4196c1479dc836

@artemcm
Copy link
Contributor Author

artemcm commented Jul 23, 2020

@swift-ci please test Windows platform

@artemcm artemcm mentioned this pull request Jul 23, 2020
Closed
@compnerd compnerd merged commit 3530c81 into swiftlang:master Jul 23, 2020
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
@artemcm @swift-ci @compnerd