Skip to content

[Frontend] Diagnostic translations re-org #32828

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 3 commits into from
Jul 14, 2020
Merged

[Frontend] Diagnostic translations re-org #32828

merged 3 commits into from
Jul 14, 2020

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Jul 10, 2020

  • Move YAML files into dedicated diagnostics/ directory
  • Make sure that translations are included in both toolchain and regular build
  • Add a missing diagnostics component at the end of the default translations path

@xedin xedin requested a review from edymtt July 10, 2020 21:43
@xedin
Copy link
Contributor Author

xedin commented Jul 10, 2020

/cc @HassanElDesouky

@xedin xedin force-pushed the translation-placement branch from 7adc185 to b425d5b Compare July 10, 2020 21:47
@xedin
Copy link
Contributor Author

xedin commented Jul 10, 2020

@swift-ci please build toolchain

1 similar comment
@xedin
Copy link
Contributor Author

xedin commented Jul 10, 2020

@swift-ci please build toolchain

@xedin xedin force-pushed the translation-placement branch from ca72826 to 76e931f Compare July 11, 2020 00:14
@xedin
Copy link
Contributor Author

xedin commented Jul 11, 2020

@swift-ci please build toolchain

@xedin xedin mentioned this pull request Jul 11, 2020
Merged
@xedin xedin force-pushed the translation-placement branch from 76e931f to a82b9fd Compare July 11, 2020 04:25
@xedin
Copy link
Contributor Author

xedin commented Jul 11, 2020

@swift-ci please build toolchain

@xedin
Copy link
Contributor Author

xedin commented Jul 11, 2020

Looks like both toolchains succeeded but didn't post the results as a comment.

@xedin
Copy link
Contributor Author

xedin commented Jul 11, 2020

@swift-ci please smoke test

@xedin
Copy link
Contributor Author

xedin commented Jul 12, 2020

@swift-ci please smoke test macOS platform

xedin
xedin commented Jul 12, 2020
View reviewed changes
localization/CMakeLists.txt Outdated
@@ -0,0 +1,21 @@
# First, let's create a directory where diagnostics are going to be placed
make_directory(share/swift/diagnostics)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like Windows CI doesn't like this because it fails with ninja: error: 'share/swift/diagnostics', needed by 'localization/CMakeFiles/copy-diagnostics', missing and no known rule to make it. @compnerd what is the cross-platform way of doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at https://github.com/apple/swift/blob/bf47403162fc53281f242da50649fda5b9a193f5/stdlib/cmake/modules/SwiftSource.cmake#L742 and https://cmake.org/cmake/help/latest/command/add_custom_command.html, my suggestion would be to remove this line and instead have add_custom_command create the directory -- e.g.

...
COMMAND "${CMAKE_COMMAND}" -E make_directory "${output}"
COMMAND "${CMAKE_COMMAND}" -E copy_if_different "${input}/*.yaml" "${output}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, let try that!

@xedin
Copy link
Contributor Author

xedin commented Jul 13, 2020

@swift-ci please smoke test Windows platform

1 similar comment
@xedin
Copy link
Contributor Author

xedin commented Jul 13, 2020

@swift-ci please smoke test Windows platform

@xedin xedin force-pushed the translation-placement branch from 3487171 to 0ad9b1a Compare July 13, 2020 18:57
@xedin
Copy link
Contributor Author

xedin commented Jul 13, 2020

@swift-ci please smoke test Windows platform

compnerd
compnerd approved these changes Jul 13, 2020
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.

We discussed the change offline and iterated a bit on it. Thanks for cleaning this up!

@xedin
Copy link
Contributor Author

xedin commented Jul 13, 2020

Windows build is running here - https://ci-external.swift.org/job/swift-PR-windows/5055/console, for some reason it hasn't been reported, maybe it would be once it finishes, let's see...

@xedin
Copy link
Contributor Author

xedin commented Jul 13, 2020

It failed with:

S:\jenkins\workspace\swift-PR-windows\lldb\source\Target\SwiftLanguageRuntimeDynamicTypeResolution.cpp(642,31): error: no viable conversion from 'llvm::Optional<SwiftASTContextReader>' to 'lldb_private::SwiftASTContextReader'
    if (SwiftASTContextReader reader = instance->GetScratchSwiftASTContext())
                              ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@xedin
Copy link
Contributor Author

xedin commented Jul 13, 2020

@swift-ci please build toolchain

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2020

@swift-ci please build toolchain

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2020

@swift-ci please smoke test Windows platform

@swift-ci
Copy link
Contributor

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 0ad9b1ab37f0f4eb7f2ddd7435dc4c90a3a28fd0

Install command
tar zxf swift-PR-32828-386-ubuntu16.04.tar.gz
More info

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2020

Ok, looks like swift_install_component can't just use diagnostics it needs a full path.

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2020

Ah no, actually it has ${diagnostics} which is much mistake :/

@xedin xedin force-pushed the translation-placement branch from 0ad9b1a to 91cd0f0 Compare July 14, 2020 05:24
@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2020

@swift-ci please build toolchain

@swift-ci
Copy link
Contributor

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 91cd0f0

Install command
tar zxf swift-PR-32828-388-ubuntu16.04.tar.gz
More info

@swift-ci
Copy link
Contributor

macOS Toolchain
Download Toolchain
Git Sha - 91cd0f0

Install command
tar -zxf swift-PR-32828-557-osx.tar.gz --directory ~/

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2020

Both toolchain got en.yaml installed! :) Let's run the tests now and we should be done. Thanks to @compnerd and @edymtt for all the help!

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2020

@swift-ci please smoke test

1 similar comment
@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2020

@swift-ci please smoke test

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2020

@swift-ci please smoke test Windows platform

@xedin xedin merged commit b327dbf into swiftlang:master Jul 14, 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

@edymtt edymtt edymtt 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.
4 participants
@xedin @swift-ci @compnerd @edymtt