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

Fix cache mixed up with different swift versions #62

Merged
merged 9 commits into from
Jul 4, 2019

Conversation

fredpi
Copy link
Contributor

@fredpi fredpi commented Jul 1, 2019

Fixes #61.

With this implementation, there is a differentiation between versions like "Swift 5.0" and "Swift 5.0.1". Previously, those have been treated equally. I'm indecisive how to handle this – what do you think?

Also, with module stability coming, we should allow a Swift 6.0 project to access cached Swift 5.1 builds, right?

@Jeehut
Copy link
Contributor

Jeehut commented Jul 2, 2019

Regarding the first point, I agree that differentiating Swift 5.0 from 5.0.1 is a good idea. Regarding backwards compatibility, see my comment in #64.

Copy link
Contributor

@Jeehut Jeehut left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, looks good in the direction. But I'm not sure if we should inject the Swift version everywhere or simply use global state. While global state doesn't make sense in many cases, the Swift version is a good example for it IMO. The reason is that the Swift version really doesn't change during the entire process of running Accio commands, so injecting it everywhere is IMO more overhead and therefore less clean than global state. While some might disagree with me, I strongly believe that global state isn't always bad. Alongside a cached property which holds the results of the first access, this should be a nicer API.

@fredpi
Copy link
Contributor Author

fredpi commented Jul 2, 2019

Regarding global state of the Swift version:

The reason is that the Swift version really doesn't change during the entire process of running Accio commands

While a change of the Swift version during runtime isn't that likely, it will have severe consequences if it happens. And scenarios, where it may happen, exist: e. g., when a new Xcode Beta release is published, the user, who set their command line tools to use this new version, may trigger accio install. Because the new Xcode release includes a new Swift version, many frameworks may not be in the cache already. So, the process takes a while, the user grabs a coffee, returns to their Mac and sets the command line tools to use the previous Xcode version again, while Accio is still running.

Now, when the Swift version has been cached, as you proposed, that's fine: Although the Swift version changed during the process of building, Accio will still store the build products in the proper place. We could therefore opt to do this caching. Still I think, it's semantically better to pass it around, as it may differ from build to build (which is currently not handled, but I suggest to do so, see next paragraph).

However, some frameworks might have been built with the Swift version that was set during the process, so not all frameworks belong to the same caching folder. In such a case, the Swift version should be determined right before each build and then stored in the build product model. We could opt for such a solution, but it would of course cause even more swift --version calls.

Given that having a framework built with Swift version x in the global cache for Swift Version y, where x != y, definitely has severe consequences for multiple users accessing this shared cache, and requires manual cleanup, I think we should be pretty precise about this. Maybe, one could infer the Swift Version from the .framework quite easily, so that a swift --version check isn't required each time?

Also, as the user himself will most likely not be able to use their frameworks built by Accio, when they changed the Swift version in the process of running Accio, we could also just stop Accio at all when a Swift version change is detected and drop a corresponding error message instead of performing the caching per Swift version.

@Jeehut
Copy link
Contributor

Jeehut commented Jul 2, 2019

IMHO the Swift version shouldn't change at all during the processing of one command. That would only cause confusion and make reproducing and fixing errors a nightmare. So, I'm definitely for returning with an error if the Swift version really ever changes during a command. I didn't think of that before, I was just referring to the thread of the command itself, not the commands we additionally run in console. But it's true that people might change Swift versions during a command.

@fredpi
Copy link
Contributor Author

fredpi commented Jul 2, 2019

@Dschee Fine – I will...

  • ... check how time-consuming it is to get the Swift version using swift --version
  • ... check whether there are better options to retrieve it (e. g. via the built frameworks)
  • ... implement monitoring of the Swift version, if suitable

@fredpi
Copy link
Contributor Author

fredpi commented Jul 2, 2019

@Dschee This is what I found out:

• Getting the Swift version via the swift --version command takes ~ 0.1 s for me
• It's possible to look for the Swift Header file (~Headers/...-Swift.h) in a framework and look up the Swift version there (it's included in a comment). This takes ~ 0.001 s on my machine, but doesn't work for some frameworks like RxAtomic where this file is missing for some reason...

Now, I implemented Swift version monitoring using the performant per-framework checking mechanism, having the swift --version command as a fallback. I'm wondering whether there is a better way to get the Swift version a framework was built with, also ensuring proper compatibility with frameworks like RxAtomic.

Copy link
Contributor

@Jeehut Jeehut left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Merge at will.

@fredpi fredpi merged commit 29691fa into stable Jul 4, 2019
@fredpi fredpi deleted the work/#61-swift-version-bugfix branch July 4, 2019 10:15
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.

Cache mixes up different Swift versions
2 participants