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

cask/installer: ensure config_path exists. #14340

Merged

Conversation

MikeMcQuaid
Copy link
Member

I encountered this when installing microsoft-edge.

Perhaps HOMEBREW_INSTALL_FROM_API related CC @Rylan12

I encountered this when installing `microsoft-edge`.

Perhaps `HOMEBREW_INSTALL_FROM_API` related.
@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Jan 7, 2023
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

Rylan12
Rylan12 previously requested changes Jan 8, 2023
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

I don't think this will work. It doesn't create the proper metadata subdirectory which means that Homebrew won't acknowledge the installed cask. Try running brew list microsoft-edge after doing an API install:

$ brew install microsoft-edge
==> Downloading https://officecdn-microsoft-com.akamaized.net/pr/03adf619-38c6-4249-95ff-4a01c0ffc962/MacAutoupdate/MicrosoftEdge-108.0.1462.76.pkg
Already downloaded: /Users/rylanpolster/Library/Caches/Homebrew/downloads/51b73235a15423fc77a44e175eb47e39a1ce928d21c98ddecc1202f6fcc3b857--MicrosoftEdge-108.0.1462.76.pkg
==> Installing Cask microsoft-edge
==> Running installer for microsoft-edge; your password may be necessary.
Package installers may write to any location; options such as `--appdir` are ignored.
installer: Package name is Microsoft Edge
installer: choices changes file '/private/tmp/choices20230108-38897-10plymd.xml' applied
installer: Upgrading at base path /
installer: The upgrade was successful.
🍺  microsoft-edge was successfully installed!

$ brew list microsoft-edge
Error: Cask 'microsoft-edge' is not installed.

The line that needs to be added is:

savedir = @cask.metadata_subdir("Casks", timestamp: :now, create: true)

It probably makes the most sense for this to go in its own method that's called by save_caskfile and save_config_file.


It's also worth noting that since we no longer download the cask source files from the API unless needed, most cask installs won't have the case source written to the .metadata directory. If this is something we want to make sure to have, we'll probably have to do a source-file download every time.

@Rylan12
Copy link
Member

Rylan12 commented Jan 8, 2023

I pushed a change implementing how I think it should be fixed. Happy to hear your thoughts. I've tested this on microsoft-edge and it seems to work.

@MikeMcQuaid
Copy link
Member Author

It's also worth noting that since we no longer download the cask source files from the API unless needed, most cask installs won't have the case source written to the .metadata directory.

Good point, didn't think of that. I think that's fine for now but may be something we reconsider later.

@MikeMcQuaid MikeMcQuaid merged commit 92ec55f into Homebrew:master Jan 9, 2023
@MikeMcQuaid MikeMcQuaid deleted the cask_installer_config_path_create branch January 9, 2023 13:57
@MikeMcQuaid
Copy link
Member Author

Thanks @Rylan12!

@github-actions github-actions bot added the outdated PR was locked due to age label Feb 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants