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

(spotify) Remove Windows Store version #2263

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pauby
Copy link
Member

@pauby pauby commented Jul 28, 2023

Description

Detects and removes the Windows Store version of Spotify if it's already installed and the --force option is used. Warn otherwise.

Motivation and Context

If the Windows Store version is installed, the package will not install.

How Has this Been Tested?

Run in Windows 10 with the Microsoft Store version of Spotify installed.

Screenshot (if appropriate, usually isn't needed):

Run without the --force option.

image

Run with the --force option:

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Migrated package (a package has been migrated from another repository)

Checklist:

  • My code follows the code style of this repository.
  • My change requires a change to documentation (this usually means the notes in the description of a package).
  • I have updated the documentation accordingly (this usually means the notes in the description of a package).
  • I have updated the package description and it is less than 4000 characters.
  • All files are up to date with the latest Contributing Guidelines
  • The added/modified package passed install/uninstall in the chocolatey test environment.
  • The changes only affect a single package (not including meta package).

@AppVeyorBot
Copy link

❌ Package verification failed, please review the Appveyor Logs and the provided Artifacts before requesting a human reviewer to take a look.

@pauby pauby force-pushed the enhance/spotify-remove-windows-store branch from a6b0933 to eacfe9e Compare July 28, 2023 17:48
@AppVeyorBot
Copy link

❌ Package verification failed, please review the Appveyor Logs and the provided Artifacts before requesting a human reviewer to take a look.

@pauby
Copy link
Member Author

pauby commented Jul 29, 2023

AppVeyor is failing for to a time out after running the scheduled task. I didn't changed this part of the code so I'm going to assume it's normal?

if ($env:ChocolateyForce) {
#when you remove a package, you don't remove it per architecture. You just remove it for all architectures.
Write-Warning 'Attempting to remove Spotify installed from the Microsoft Store.'
Remove-AppxPackage -Package $installedAppXPackage[0].PackageFullName
Copy link
Member

Choose a reason for hiding this comment

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

IMO, we should not remove the Microsoft Store application in this package.

It is still two different applications, which goes against having one software for each package.

IMO, I think we should throw an exception instead, no matter what, possibly with details on how the user can uninstall the application manually or add a new package that only has the purpose of removing the Store version of Spotify (that we possibly can depend on in this package).

Copy link
Member Author

Choose a reason for hiding this comment

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

A package parameter? Warn them but if they add the package parameter it also uninstalls. Seems like the best of both worlds.

Copy link
Member

Choose a reason for hiding this comment

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

hmm. Add it behind a package parameter 🤔.

Yeah, could work as well. Let's go with that instead. I still do think a separate package could be beneficial though, but we can go with the parameter for now.

Copy link
Member

Choose a reason for hiding this comment

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

Please fix the indentation used on the lines changed to follow the editor config file.
The editor config file specifies that indentations should only be two spaces.

Copy link
Member Author

@pauby pauby Aug 29, 2023

Choose a reason for hiding this comment

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

The indentation is inconsistent. I'll update the file to match 2 spaces.

@AdmiringWorm
Copy link
Member

AppVeyor is failing for to a time out after running the scheduled task. I didn't changed this part of the code so I'm going to assume it's normal?

I'm honestly not sure. It could be that appveyor doesn't allow scheduled tasks to be created...

@github-actions
Copy link

Dear contributor,

As this PR seems to have been inactive for 30 days after changes or additional information
was requested, I've automatically closed it.
If you feel the changes are still valid, please re-open the PR once all changes or additional information
that was requested has been added.
Thank you for your contribution.

@github-actions github-actions bot closed this Oct 22, 2023
@pauby pauby reopened this Oct 24, 2023
@pauby
Copy link
Member Author

pauby commented Oct 24, 2023

I've reopened this PR as I simply haven't had time to work on it.

@pauby pauby removed the Unresolved label Oct 24, 2023
@AppVeyorBot
Copy link

❌ Package verification failed, please review the Appveyor Logs and the provided Artifacts before requesting a human reviewer to take a look.

@corbob
Copy link
Contributor

corbob commented Oct 24, 2023

AppVeyor is failing for to a time out after running the scheduled task. I didn't changed this part of the code so I'm going to assume it's normal?

For what it's worth, I attempted to install the version of this package in CCR, and it seemed to do that same thing for me, as soon as I manually uninstalled the Spotify store app, then it didn't timeout and installed without issue. Depending on what AppVeyor is using, this could be the same thing (I would have thought it'd be on a Server OS without the Store, but maybe not 🤷 )

@AdmiringWorm
Copy link
Member

AppVeyor is failing for to a time out after running the scheduled task. I didn't changed this part of the code so I'm going to assume it's normal?

For what it's worth, I attempted to install the version of this package in CCR, and it seemed to do that same thing for me, as soon as I manually uninstalled the Spotify store app, then it didn't timeout and installed without issue. Depending on what AppVeyor is using, this could be the same thing (I would have thought it'd be on a Server OS without the Store, but maybe not 🤷 )

It is reporting that it is running on Microsoft Windows Server 2016 Datacenter (https://ci.appveyor.com/project/chocolateycommunity/chocolatey-packages/builds/48351047#L134). So it shouldn't be a problem with the Windows Store. It may be possible that Spotify does not run on a Server, or it only supports the absolute latest (I honestly do not know).

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

Successfully merging this pull request may close these issues.

4 participants