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

Update the cached list of available Xcodes if it's more than 24 hours old #226

Merged
merged 4 commits into from
Oct 14, 2022
Merged

Update the cached list of available Xcodes if it's more than 24 hours old #226

merged 4 commits into from
Oct 14, 2022

Conversation

rpendleton
Copy link
Contributor

@rpendleton rpendleton commented Oct 3, 2022

This fixes #202, although I made a few assumptions that I'm open to discussion about.

Specifically:

  1. I wasn't sure how widespread the cache updating should be. My changes hook in to the existing shouldUpdate property, but I could see value in doing this differently depending on the scenario. (For example, you might not need to update the list of Xcodes to install a version that's already known. That would be more complex though.)

  2. I wasn't sure whether it made sense to change the format of the cache file (and whether that would need migrations), so rather than doing that, I decided to just check modification dates of the cache file on disk. An alternative might be to store the modification date in the cache file or some other configuration/preference file.

  3. Although it's unlikely, reading the attributes of a file can technically fail. If that happens (or if the modified date is otherwise missing), I made it so the cached list will load anyway, and shouldUpdate will return false. I'm not sure whether this makes the most sense, but I figured it was better than repeatedly requesting data from the server.

If any of those assumptions seem like bad ones, I'm open to alternatives.

@rpendleton rpendleton requested a review from a team as a code owner October 3, 2022 08:28
@rpendleton
Copy link
Contributor Author

In my first assumption, I mentioned that an alternative for xcodes install ... might be to only update the list if the requested version is missing.

After looking at the code a bit, it looks like that wouldn't actually be hard to implement since all of the necessary information is already available. It would also solve #104, which would be helpful if someone tries to install a version of Xcode that was released within a few hours of it being released.

I haven't tested it yet, but I'm thinking XcodeInstaller.downloadXcode would just need to be updated to something like this:

if self.xcodeList.shouldUpdate || self.xcodeList.availableXcodes.first(withVersion: version) == nil {

Do you have a preference on what approach is taken? Should xcodes list always update old lists and xcodes install only update if a version is missing? Or should both commands always update if the list is old, but with the addition that xcodes install also updates if the version is missing?

@MattKiazyk
Copy link
Contributor

@rpendleton i think we should be smart as we can in that only update the list when we need to and not all the time.

Should xcodes list always update old lists and xcodes install only update if a version is missing?

Is a good assumption we can take

@rpendleton
Copy link
Contributor Author

rpendleton commented Oct 4, 2022

I've pushed a change that:

  • updates the cache if it's missing or old when running xcodes list
  • updates the cache if it's missing the requested version when running xcodes install
  • fixes a regression in the 1.0.0 release that causes xcodes install <version> to fail after a clean install

It ended up being a little bit more complicated than what I previously mentioned, because the list of available Xcodes was actually being checked in two different places depending on the data source. The first check was actually throwing an unavailableVersion error before it even had a chance to update the list, which is what caused the regression.

I cleaned that logic up a bit so that the list of available Xcodes is only checked in one place regardless of the data source, and only after the list of available Xcodes has had a chance to update.

(If you'd prefer, I can separate out the fix for that regression into a separate PR, but there would likely be conflicts between that PR and this one.)

Copy link
Contributor

@MattKiazyk MattKiazyk left a comment

Choose a reason for hiding this comment

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

@rpendleton 👏

Thanks for the update - it works great and is a fantastic addition!

@rpendleton
Copy link
Contributor Author

rpendleton commented Oct 9, 2022

For some reason, GitHub thought there was a conflict in the renamed main.swift/App.swift from #223 (even though a manual git merge didn't detect any actual conflicts), so I've merged main into this branch so that GitHub will stop saying there are conflicts.

@MattKiazyk MattKiazyk merged commit 9fb3593 into XcodesOrg:main Oct 14, 2022
@MattKiazyk MattKiazyk added the enhancement New feature or request label Oct 14, 2022
@rpendleton rpendleton deleted the update-cache-periodically branch October 14, 2022 05:20
juanjonol added a commit to juanjonol/Xcode-Update that referenced this pull request Apr 7, 2023
Since `xcodes` v1.1.0 this is no longer required, because it's automatically done if more than 24 hours has passed since the last `xcodes list`.

- XcodesOrg/xcodes#226

This reverts commit e2f54e5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically call update if list is old
2 participants