Skip to content

Swift: Build incompatible OS diagnostic on all platforms.#13437

Closed
sashabu wants to merge 1 commit into
mainfrom
sashabu/windows
Closed

Swift: Build incompatible OS diagnostic on all platforms.#13437
sashabu wants to merge 1 commit into
mainfrom
sashabu/windows

Conversation

@sashabu

@sashabu sashabu commented Jun 12, 2023

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions github-actions Bot added the Swift label Jun 12, 2023
@sashabu sashabu marked this pull request as ready for review June 12, 2023 16:31
@sashabu sashabu requested review from a team as code owners June 12, 2023 16:31

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How is this / can this be tested?

Comment thread swift/BUILD.bazel
Comment on lines +83 to +85
"//conditions:default": [
":incompatible-os",
],

@redsun82 redsun82 Jun 13, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't this be something like

pkg_filegroup(
    name = "extractor-pack-arch",
    srcs = select({
        "@platforms//os:windows": [],
        "//conditions:default": [
            ":extractor",
            ":swift-test-sdk-arch",
        ],
    }) + select({
        "@platforms//os:macos": [
            ":xcode-autobuilder",
        ],
        "//conditions:default": [
            ":incompatible-os",
        ],
    }),
    visibility = ["//visibility:public"],
)

or should this be postponed to a follow-up PR where the whole extractor-pack can be built on Windows (including a Windows version of extractor-pack-generic, probably with just an autobuild.cmd)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was planning on doing this in a follow-up PR, but see my comment to Geoff below: I'm now leaning towards adding all the commits here instead.

@sashabu

sashabu commented Jun 13, 2023

Copy link
Copy Markdown
Contributor Author

How is this / can this be tested?

I tested locally on Windows with bazel build //swift/tools/autobuilder-diagnostics:incompatible-os. Before this PR, it fails; with this PR it succeeds.

This is probably not useful on its own, but will be required for emitting nice error messages on Windows. The error messages will have an integration test when implemented.

If you (or anyone else) are not comfortable merging this without automated testing, I'm happy to hold off and put everything in the same PR. That might be a good idea in light of Paolo's comment as well - perhaps in my aim to have small, self-contained PRs, I've gone too far and sent out half of a PR!

@sashabu sashabu marked this pull request as draft June 13, 2023 20:30
@geoffw0

geoffw0 commented Jun 14, 2023

Copy link
Copy Markdown
Contributor

If you (or anyone else) are not comfortable merging this without automated testing

No, I'm happy at this stage to understand how this has been and can be tested. It will be good to have automated coverage later.

@sashabu

sashabu commented Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

If you (or anyone else) are not comfortable merging this without automated testing

No, I'm happy at this stage to understand how this has been and can be tested. It will be good to have automated coverage later.

I'll close this in favour of #13447 which puts this in the context of the rest of the extractor pack (which has an internal PR with tests).

@sashabu sashabu closed this Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants