Use Swift 5.4 for Windows job on GitHub Actions

[MaxDesiatov]

I've updated the Windows job on GitHub Actions to use a release build of Swift 5.4, and also updated the dependency in Package.swift and Package.resolved.

Additionally, all GitHub Actions triggers were updated to prevent jobs from running twice on pull requests created from branches in the same repositories.

[google-cla]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[MaxDesiatov]

@googlebot I signed it!

[MaxDesiatov]

@compnerd would you have a moment to have a look?

[compnerd]

This shouldn't be needed

Suggested change

	pull_request:
	branches: "*"
	pull_request:

[compnerd]

Suggested change

	pull_request:
	branches: "*"
	pull_request:

[compnerd]

Suggested change

	pull_request:
	branches: "*"
	pull_request:

[compnerd]

Can you split this out into a separate change please?

[MaxDesiatov]

It looked like old version of SAP could fail to build with Swift 5.4 on Windows for some reason. But I've reverted the update in Package.swift and Package.resolved for now as you've requested. Curious to see if CI passes on Windows without updating the SAP dependency. Could you approve it for another CI run? I'm not able to trigger it with a sole push.

[compnerd]

Ah, I wonder if its the MSVCRT -> CRT renaming that you are referring to. I think that we could just do something like:

#if os(Windows)
#if canImport(CRT)
import CRT
#else
import MSVCRT
#endif
#endif

[MaxDesiatov]

As far as I understand, this was fixed in the latest version of SAP, the current locked version fails to build on Windows with these kind of errors:

swift-argument-parser\Sources\ArgumentParser\Usage\HelpGenerator.swift:299:13: error: cannot find type 'CONSOLE_SCREEN_BUFFER_INFO' in scope

Would you like me to bring back the SAP update to this PR to fix that?

[MaxDesiatov]

I've confirmed in my fork https://github.com/MaxDesiatov/swift-benchmark/pull/2 that it won't build with the old snapshot currently used in this repository.
I've created a separate PR on this repo for SAP update, just in case. But that will fail too, as it did in my fork. #90

So I think that SAP update and CI update to Swift 5.4 should be included in this same PR for CI to be all green, unless I'm missing something?

[compnerd]

It can be made to work - s-a-p uses canImport to import CRT without the fallback for MSVCRT and that changed in the 5.4 release. I suppose that its not worth the effort to split out the changes, but in general, it is preferable to stage the versions before bumping the release IMO.

[MaxDesiatov]

it is preferable to stage the versions before bumping the release IMO.

I totally agree, but I'm still a bit confused as to how to land these two changes in separate PRs and to keep their respective CI status green at the same time. For now I'm bringing back the update to s-a-p version, but keeping it as a separate commit. That would preserve the separation after merge if you don't squash it. Ready for review and a CI run.

[compnerd]

I was thinking that we could inject a CRT module that just does @_exported import MSVCRT. But again, I'm not too happy with that either. Sorry for the churn here. I think that the original idea of just bumping it together is fine.

[MaxDesiatov]

Thanks! Would it be possible to tag a new version so that other libraries targeting Windows could depend on that instead of the main branch?

[MaxDesiatov]

@regexident I see you've tagged the previous release, would you be able to tag a new one with this change included?

[regexident]

@MaxDesiatov I did? On mobile right now so can't check, but I doubt it since I'm just a random contributor like you. 😄

[MaxDesiatov]

Well, this is weird...

[compnerd]

@MaxDesiatov I've tagged 0.1.1; I didn't see a reason for any thing bigger in terms of a version bump, the only change was for newer Swift releases on WIndows.

[MaxDesiatov]

Fantastic, thanks! 0.1.1 works perfectly fine for this 👍

[regexident]

Oh, interesting. I stand corrected then. 😅