Add check for Xcode version compatibility #34227
Conversation
utils/build-script
Outdated
| ("12.0 beta 6", "12A8189n"), | ||
| ("12.0 GM 1", "12A7208"), | ||
| ("12.0 GM 2", "12A7209"), | ||
| ("12.0.1", "12A7300"), |
There was a problem hiding this comment.
Also these current values are an assumption, the README says 12.0b3, and therefore I assume up to 12.0.1 works
There was a problem hiding this comment.
I am little worried about having list of supported Xcode because it will be come stale overtime. Should we only provide what is support in CI?
There was a problem hiding this comment.
Yea that's a good question. I like the idea of being permissive here especially beta to beta because it's such a high cost to download a new one / unxip it if it's still supported "enough." And ideally this wouldn't get super stale because at the point support was added / removed for specific Xcode versions it would be updated. But I can see that being tough especially if apple folks are all skipping this check.
There was a problem hiding this comment.
This would be still tested on open source CI, so it will not go stale if we pick one Xcode version.
There was a problem hiding this comment.
Some thoughts:
- It seems like it'll be easier to maintain a single version, instead of a list
- Having a list seems like it'll expand the chances of one or more being incorrect, since there will be no CI to validate the list
- Ideally the single version would match the version used on CI, but at this moment in time that's a beta which most people have likely moved away from
- Having a long list is an optimization, maybe it's better to be correct first
What if we limit the list to one or two versions. For example right now we could say either 12b3 (CI), or 12.0.1 (a release). When CI and latest version match, then there'd be only one version.
There was a problem hiding this comment.
Definitely, yea if you think that's the best way to do it we can do 1, but I do think if possible we should allow as many as should actually work for flexibility, especially during times where there are so many Xcode releases
There was a problem hiding this comment.
I'm thinking we can put a comment on _SUPPORTED_XCODE_BUILDS and say something like:
These versions are community sourced. At any given time only the Xcode version used by Swift CI is officially supported. See https://ci.swift.org.
Maybe we could even separate out the current CI version of Xcode into its own constant, and if the current version doesn't match that, a warning/info notice can be shown, to highlight the potential issue in case anything goes wrong.
There was a problem hiding this comment.
I like this idea, pushed this comment. I don't immediately see anything in the repo tying us to that version, so I'm not sure we can easily setup the second constant (it is in the README, but I don't see it elsewhere)
There was a problem hiding this comment.
I figured the second constant would have to be defined here. The question is: Is it good to introduce that constant here, or not?
|
What would be the right place to document the |
|
Also, is there a specific reason it is an environment variable instead of a flag? I wrote the comment earlier related to documenting it, but I just realized, if it were a flag, there would be a natural place for documenting it. |
|
I chose an environment variable to make it easier for folks to entirely opt out of this, which it sounds like some Apple folks might want to do, without having to change their current invocations |
|
+1 for me, @shahmishal and @varungandhi-apple what do you say? |
|
The problem we are trying to solve here is to provide an easy way for new contributors to figure out they are using the supported/unsupported Xcode version. If the supported Xcode version list is not being validated daily, it will become out of date and new contributors will not benefit from this feature. My personal opinion is to have one version which matches Swift CI. |
|
That makes sense to me I was just trying to balance the burden of downloading a specific version when you may already have many other nearly identical versions that do work, as is the case with all seven versions of 12.0.0.x that currently work, which I personally had 5 of that were not beta 3, vs having it perfectly in sync with CI |
|
Note that CI has been updated to Xcode 12.2 Beta 3. @shahmishal @varungandhi-apple do either of you know in the past when the documentation was updated to new minimum versions? Does the documented minimum Xcode version update in lock step with CI updates? |
|
With that, @keith the list can be reduced to the one version. |
|
Whether we include other versions can be decided in future PRs, probably. |
This adds a check validating the current Xcode version is supported for building the current sha. This makes us fail fast in the case that you've selected too new or too old of an Xcode version that might otherwise fail very late in the build process. You can also set SKIP_XCODE_VERSION_CHECK to anything to skip this.
keith
force-pushed
the
ks/add-check-for-xcode-version-compatibility
branch
from
9cc1461 to
15937f4
Compare
|
I've updated to only include 12.2b3 for now! |
|
@swift-ci smoke test |
|
@swift-ci python lint |
|
Fixed up the python lint |
|
@swift-ci python lint |
|
@swift-ci smoke test and merge |
This allows any versions above a specific version, instead of requiring a specific list. Since it seems like many folks just disable this check entirely, this list definitely isn't kept up well. If we need to exclude a specific version, like if 1 Xcode beta didn't support it but the next did, we can add a new list using the old method to validate build numbers and disallow the bad version(s). I don't recall why we didn't do this over a list originally, nor can I find any context on why in the original discussion swiftlang#34227
This allows any versions above a specific version, instead of requiring a specific list. Since it seems like many folks just disable this check entirely, this list definitely isn't kept up well. If we need to exclude a specific version, like if 1 Xcode beta didn't support it but the next did, we can add a new list using the old method to validate build numbers and disallow the bad version(s). I don't recall why we didn't do this over a list originally, nor can I find any context on why in the original discussion swiftlang#34227
This allows any versions above a specific version, instead of requiring a specific list. Since it seems like many folks just disable this check entirely, this list definitely isn't kept up well. If we need to exclude a specific version, like if 1 Xcode beta didn't support it but the next did, we can add a new list using the old method to validate build numbers and disallow the bad version(s). I don't recall why we didn't do this over a list originally, nor can I find any context on why in the original discussion swiftlang#34227
This allows any versions above a specific version, instead of requiring a specific list. Since it seems like many folks just disable this check entirely, this list definitely isn't kept up well. If we need to exclude a specific version, like if 1 Xcode beta didn't support it but the next did, we can add a new list using the old method to validate build numbers and disallow the bad version(s). I don't recall why we didn't do this over a list originally, nor can I find any context on why in the original discussion swiftlang#34227
This allows any versions above a specific version, instead of requiring a specific list. Since it seems like many folks just disable this check entirely, this list definitely isn't kept up well. If we need to exclude a specific version, like if 1 Xcode beta didn't support it but the next did, we can add a new list using the old method to validate build numbers and disallow the bad version(s). I don't recall why we didn't do this over a list originally, nor can I find any context on why in the original discussion swiftlang#34227
This adds a check validating the current Xcode version is supported for
building the current sha. This makes us fail fast in the case that
you've selected too new or too old of an Xcode version that might
otherwise fail very late in the build process. You can also set
SKIP_XCODE_VERSION_CHECK to anything to skip this.
Fixes https://bugs.swift.org/browse/SR-13497