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 outdated & upgrade commands #312

Merged
merged 3 commits into from
Mar 14, 2021
Merged

Conversation

rgoldberg
Copy link
Contributor

Fix outdated & upgrade commands.

Resolves #111 #245 #248 #252 #282 #306

Possibly #76 #92 #247 #249 #290

MasKit/Commands/Outdated.swift Outdated Show resolved Hide resolved
MasKit/Commands/Outdated.swift Outdated Show resolved Hide resolved
@rgoldberg rgoldberg force-pushed the outdated branch 2 times, most recently from 4ff363c to b5a8553 Compare January 18, 2021 01:38
@rgoldberg
Copy link
Contributor Author

I force pushed some minor changes 14 hours ago, but the Jenkins/pr-merge check hasn't completed. How long does it normally take? Might something have gone wrong? 14 hours seems a long time, but maybe it's normal…

@rgoldberg
Copy link
Contributor Author

rgoldberg commented Jan 19, 2021

The most recent Jenkins/pr-merge build failed, but previous builds succeeded.

The logs say that the Commandant build failed.

The only differences in the most recent force push were in CHANGELOG.md, which should not affect the Commandant build, right?

Supposedly, there's more info in the following file, but I don't know how to see the file:

/var/folders/8l/wsf0fx0x015dtgb_pwjyj3zr0000gr/T/carthage-xcodebuild.RDNixk.log

@rgoldberg
Copy link
Contributor Author

rgoldberg commented Jan 19, 2021

Whenever I build, changes are made to spacing in various files that I haven't edited (presumably because the spacing violates various formatting rules).

Can I commit those spacing changes in this PR, so people don't keep getting files changed on them? Or should I create a new issue & PR?

There's also a bug in the InfoCommandSpec displays app details check: the expected value for Released is Jan 7, 2019, but on my machine, the value is 2019-01-07. Either:

  1. the default medium date style must be changed locally in the test function
  2. the system's default medium date style must be used to calculate the expected output, instead of hardcoding it to Jan 7, 2019
  3. the InfoCommand must use a deterministic date format instead of one that is system configuration dependent
  4. something else is causing my problem other than a system-wide medium date format that is different than what the test expects (but I don't know what that cause would be)

Which do you prefer?

@rgoldberg
Copy link
Contributor Author

rgoldberg commented Feb 13, 2021

The file spacing is no longer changed by the build, presumably because of the change from swiftlint autocorrect --format to swiftlint autocorrect, but the Info_command__displays_app_details test date format error still occurs.

@rgoldberg
Copy link
Contributor Author

rgoldberg commented Feb 15, 2021

Hi @phatblat, glad you’re recovering.

Sorry to bother you, but is there anything that I (or anyone else) must do to get the Jenkins/pr-merge check to run?

I pushed commits 2 days ago, and it still hasn’t run…

@phatblat
Copy link
Member

@rgoldberg sorry for the delay getting to this PR. The Jenkins server these checks run on is a mac mini I have hosted at MacStadium. It's been offline for a while due to an OS upgrade that failed.

The CI just runs several of the scripts checked into the project, so as long as these are working for you and me, we can ignore Jenkins for now.

I will take a closer look at these changes and test them out myself tonight, but at a quick glance, I think this is a great idea. This tool has been using a private CommerceKit API to figure out which apps have pending updates but that API stopped returning anything some time back. I've dug around in a debugger several times looking for where this functionality moved to but haven't been successful.

Thanks for the PR! I will get back with any suggestions this week.

@phatblat phatblat self-assigned this Feb 18, 2021
@phatblat phatblat added this to the Unreleased milestone Feb 18, 2021
@rgoldberg
Copy link
Contributor Author

Thanks, @phatblat. Glad you're recovering. No problem about delay. Take whatever time you need to stay healthy, and to ensure the PR is good.

I haven't looked throughout mas to see whether there are other commands that can be similarly fixed.

I tried to write tests, but got cryptic errors, so I decided to wait on tests until I communicated with you.

@phatblat
Copy link
Member

Sorry about the changing format. I'd rather have that as a separate script but it was easy to add the --format flag to swiftlint and didn't think about the ramifications.

On the "InfoCommandSpec displays app details" test failures, I think a deterministic date format (option 2) would be ideal
since the output could be used by other CLI tools.

I'm curious, what locale setting do you have to get that format? That "yyyy-MM-dd" format (ISO-8601 minus the time) is perfect for the new format. You can find this formatter in AppInfoFormatter.

@rgoldberg
Copy link
Contributor Author

rgoldberg commented Feb 18, 2021

No problem about formatting. I just didn't want to cause build or git history issues by changing things without asking. I thought that maybe I had missed some setup instructions that would have prevented the file reformatting.

I set all 4 date formats in System Preferences > Language & Region > Advanced… > Dates to "yyyy-MM-dd" a long time ago.

I'll have to read about the Apple DateFormatter to learn about how to set it programmatically, as I don't normally code in Swift or for Apple APIs.

Should I open a new issue & PR for the "InfoCommandSpec displays app details" date format change, or just include it in this PR in a separate commit?

@phatblat
Copy link
Member

phatblat commented Feb 19, 2021

Yes, please create a separate PR for this date formatting change. That way it will be easier to track to see which version it changed in.

@rgoldberg
Copy link
Contributor Author

I'll create a new issue & PR for the date format change after we finish with the current PR, as I don't want to have to manage multiple branches. The second PR will be simple, so easy to finish after this one.

@phatblat
Copy link
Member

I managed to get the CI server back online tonight but builds are failing due to some issues I still need to clean up. So ignore the failing check for now.

@rgoldberg
Copy link
Contributor Author

Will do. Please let me know whenever I need to do anything to improve / finish the PR.

@phatblat
Copy link
Member

Wow, this works better than the App Store's "Updates" section! The App Store app doesn't show any of the 5 pending updates.

↪ mas outdated
1462114288 Grammarly for Safari (9.27 -> 9.26)
803453959 Slack (4.12.2 -> 4.13.0)
Warning: Identifier 927292435 not found in store. Was expected to identify iStat Mini.
411643860 DaisyDisk (4.12.1 -> 4.12)
1475897096 Jira (122.0.0 -> 123.1.0)
Warning: Identifier 781577745 not found in store. Was expected to identify Dumper.
Warning: Identifier 924726344 not found in store. Was expected to identify Deliveries.
1246284741 Install macOS High Sierra (13.6.02 -> 10.13.6)

Looked into the apps which weren't found:

  • iStat Mini - no longer in app store
  • Dumper - no longer in app store
  • Deliveries - has a different app store identifier now: 290986013

There might be a way to map apps like Deliveries that release updates as a new app to the one, but these are technically not an upgrade so the only thing we might be able to do is to have a more informative warning message.

The macOS installers have a weird versioning scheme which looks like it doesn't match the version reported by the iTunes Search API. This is a very minor issue as these are not usually kept around.

@phatblat
Copy link
Member

Weird, both Grammarly and DaisyDisk are newer versions on my system than the app store reports.

  • 1462114288 Grammarly for Safari (9.27 -> 9.26)
  • 411643860 DaisyDisk (4.12.1 -> 4.12)

I installed them from the app store some time ago. Not sure how this can happen unless installing beta versions direct from the developer (which these aren't). I'll keep digging on the reason for this.

@phatblat
Copy link
Member

Running the upgrade command attempted to upgrade all 8 of the items listed by the outdated command, even the ones that output a warning.

↪ mas upgrade
Upgrading 8 outdated applications:
Grammarly for Safari (9.27), Slack (4.12.2), iStat Mini (1.1), DaisyDisk (4.12.1), Jira (122.0.0), Dumper (1.2.1), Deliveries (3.2.4), Install macOS High Sierra (13.6.02)
==> Downloading Grammarly for Safari
==> Installed Grammarly for Safari
==> Downloading Slack
==> Installed Slack
==> Downloading iStat Mini
==> Installed iStat Mini
==> Downloading DaisyDisk
==> Installed DaisyDisk
==> Downloading Jira Cloud by Atlassian
==> Installed Jira Cloud by Atlassian
==> Downloading Dumper
==> Installed Dumper
==> Downloading Deliveries 3
==> Installed Deliveries 3
==> Downloading macOS High Sierra
==> Installed macOS High Sierra

Afterwards outdated lists 6 apps. Slack and Jira were the only two successfully upgraded.

↪ mas outdated
1462114288 Grammarly for Safari (9.27 -> 9.26)
Warning: Identifier 927292435 not found in store. Was expected to identify iStat Mini.
411643860 DaisyDisk (4.12.1 -> 4.12)
Warning: Identifier 781577745 not found in store. Was expected to identify Dumper.
Warning: Identifier 924726344 not found in store. Was expected to identify Deliveries.
1246284741 Install macOS High Sierra (13.6.02 -> 10.13.6)

Is there a way to exclude the items that generate a warning from the upgrade command?

MasKit/Commands/Upgrade.swift Outdated Show resolved Hide resolved
@rgoldberg
Copy link
Contributor Author

Weird, both Grammarly and DaisyDisk are newer versions on my system than the app store reports.

I installed them from the app store some time ago. Not sure how this can happen unless installing beta versions direct from the developer (which these aren't). I'll keep digging on the reason for this.

Maybe a version was released to the store, but was later retracted…

@rgoldberg
Copy link
Contributor Author

The Jenkins/pr-merge timed out, but I'm not sure why…

Copy link
Member

@phatblat phatblat 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 all your great work on this! 💯

@phatblat phatblat merged commit 5539145 into mas-cli:master Mar 14, 2021
@phatblat phatblat mentioned this pull request Mar 14, 2021
@rgoldberg rgoldberg deleted the outdated branch March 15, 2021 00:19
@rgoldberg
Copy link
Contributor Author

@phatblat No problem. Glad to help. Thanks for maintaining / working on mas.

All but one issue that I referenced in the commit message are still open, so someone should go through the list and close the applicable issues. Some issues will definitely be fixed by this PR, some other issues might be fixed, but I'm not 100% sure as I didn't have the time to verify each.

@phatblat
Copy link
Member

Thanks for reminding me. Once I get #327 completed I'll ask on each issue for confirmation.

@rgoldberg
Copy link
Contributor Author

The issues listed after Resolves should all be fixed by this PR. Those listed after Possibly are ones I'm not 100% sure about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mas outdated doesn't work
3 participants