Skip to content
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.

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

Include Swift compiler version in addition to swiftlang version and clang version #2052

Merged
merged 2 commits into from Jul 10, 2017

Conversation

ikesyo
Copy link
Member

@ikesyo ikesyo commented Jul 6, 2017

That would be more comprehensible.

See also #1983 and #1984. Originally I assumed this in this comment.

@ikesyo ikesyo requested review from mdiep and BobElDevil July 6, 2017 11:02
@mdiep
Copy link
Member

mdiep commented Jul 6, 2017

I'm not sure which is the best approach here. This is certainly more comprehensible. The current implementation is probably more correct, but perhaps not meaningfully so. Outside of some very small potential edge cases, I think these should behave the same.

What do you think @BobElDevil?

@ikesyo
Copy link
Member Author

ikesyo commented Jul 6, 2017

I'm concerned about Xcode patch version updates. For example (the below should not be actual values):

  • Xcode 9.0
    • 4.0 and swiftlang-900.0.43 clang-900.0.22.8
  • Xcode 9.0.1
    • 4.0 and swiftlang-900.0.44 clang-900.0.22.9

If we use the former a build cache will still be valid, but if we use the latter will not be valid anymore.

However, it may be safer to use the latter value if we take into account a situation that there is no binary compatibility between patch version updates.

@BobElDevil
Copy link
Contributor

Yeah, incompatibility between patch versions is what I'm worried about too. So I think we should stick with the swiftlang/clang version. However I think for readability, we could take this opportunity to pull in both parts of the version string, so we can make more readable messaging. So while we still compare compiler versions, we still change it from:
"Incompatible Swift version - framework was built with swiftlang-100.0.00 clang-100.0.00.1 and the local version is swiftlang-200.0.00 clang-200.0.00.2."
to
"Incompatible Swift version - framework was built with 3.0 (swiftlang-100.0.00 clang-100.0.00.1) and the local version is 3.1 (swiftlang-200.0.00 clang-200.0.00.2.")

@mdiep
Copy link
Member

mdiep commented Jul 6, 2017

If we use the former a build cache will still be valid, but if we use the latter will not be valid anymore.

But is the latter valid? I'm not sure that we can count on compatibility between patch versions.

@ikesyo
Copy link
Member Author

ikesyo commented Jul 6, 2017

compatibility between patch versions

That will not be guaranteed. Also, I'm not sure if swiftlang/clang versions are bumped between patch versions.

@mdiep
Copy link
Member

mdiep commented Jul 7, 2017

I think we should stick with the current implementation because it's safer. But I think it'd be fine to pull in the Swift version like @BobElDevil suggested.

…iftlang and clang version)

That would be more comprehensible.
@ikesyo ikesyo force-pushed the simpler-swift-compiler-version branch from 7a74946 to 513d9b0 Compare July 10, 2017 13:37
@ikesyo ikesyo changed the title Use Swift compiler version excluding effective version (instead of swiftlang and clang version) Include Swift compiler version in addition to swiftlang version and clang version Jul 10, 2017
@ikesyo ikesyo requested review from mdiep and BobElDevil and removed request for BobElDevil July 10, 2017 13:38
@ikesyo ikesyo force-pushed the simpler-swift-compiler-version branch from 513d9b0 to 3f81593 Compare July 10, 2017 13:42
@BobElDevil BobElDevil merged commit 71c7d82 into master Jul 10, 2017
@mdiep mdiep deleted the simpler-swift-compiler-version branch July 10, 2017 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants