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

Allow specifying pkg target: #14393

Closed
wants to merge 1 commit into from
Closed

Conversation

sambostock
Copy link

@sambostock sambostock commented Jan 20, 2023

One such target is CurrentUserHomeDirectory. See installer -help.

Required for Homebrew/homebrew-cask#139807

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Currently, we always use the `/` target, but other volumes can be
provided as targets, as well as special targets like
`CurrentUserHomeDirectory`. See `installer -help` for more info.

In particular, if installing to `CurrentUserHomeDirectory` it seems
reasonable to assume we wouldn't need `sudo`.

pkg "MyFancyPkg/Fancy.pkg", target: "CurrentUserHomeDirectory"

uninstall pkgutil: "my.fancy.package"
Copy link
Author

Choose a reason for hiding this comment

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

I copied both these fixtures from the with-installable fixture, but I simplified the uninstall stanza, since all the extra stuff there isn't relevant to the specs these are used in.

Comment on lines +114 to +115
"-target", "CurrentUserHomeDirectory"],
sudo: false,
Copy link
Author

Choose a reason for hiding this comment

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

I'm making an assumption here, but I feel like it's reasonable. If we're installing in the home directory, isn't it an error if sudo is required?

@sambostock
Copy link
Author

Trying this out with the cask in Homebrew/homebrew-cask#139807, it looks like uninstall pkgutil: also needs the ability to specify a target, although that appears to work differently...

@@ -65,7 +65,8 @@ def run_installer(command: nil, verbose: false, **_options)
"USER" => User.current,
"USERNAME" => User.current,
}
command.run!("/usr/sbin/installer", sudo: true, args: args, print_stdout: true, env: env)
sudo = target != "CurrentUserHomeDirectory"
Copy link
Author

Choose a reason for hiding this comment

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

Should we expect the cask to pass paths as strings, but special values like CurrentUserHomeDirectory as a Symbol?

Suggested change
sudo = target != "CurrentUserHomeDirectory"
sudo = target != :CurrentUserHomeDirectory

@bevanjkay
Copy link
Member

bevanjkay commented Jan 23, 2023

@sambostock One thing to consider here is that the installer command doesn't allow you to pass a path to -target but rather a volume. It does work for targeting the user folder ~/Library as opposed to /Library (with "CurrentUserHomeDirectory"), but you can't pass a full path. But for various items, installers can choose where they put things, so you don't get full control over what is happening.

@MikeMcQuaid MikeMcQuaid requested a review from a team January 23, 2023 13:29
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Code looks good but from a product perspective I'd like to have discussion in https://github.com/Homebrew/homebrew-cask/pull/139807/files#r1084060179 before we merge this.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Feb 14, 2023
@MikeMcQuaid
Copy link
Member

@Homebrew/cask thoughts here?

@github-actions github-actions bot removed the stale No recent activity label Feb 14, 2023
@miccal
Copy link
Member

miccal commented Feb 14, 2023

I agree with this comment: Homebrew/homebrew-cask#139807 (comment)

Also, we have two other Casks (that I can think of) that use a pkg installer and are plugins for OBS, and neither of these require this change:

https://github.com/Homebrew/homebrew-cask-drivers/blob/master/Casks/obs-ios-camera-source.rb

https://github.com/Homebrew/homebrew-cask-drivers/blob/master/Casks/droidcam-obs.rb

@vitorgalvao
Copy link
Member

It’s been too many years to recall exactly, but I think using a target with PKG was considered and dropped way back when. The vast majority of PKGs work by writing to specific locations, which in turn makes developers only write their PKGs that way. PKGs are relatively niche and awkward to support programatically, customising their installation options is nicher still. Unless extensive testing has been done in all available casks, this might be one of those features that will take more effort to support and work around than it’s worth. At the very least we’d need an option to say “this target cannot be customised” and in the case of PKGs I wouldn’t be surprised it that ended up applying to most of them.

I’d be wary of adding the feature without a very clear “this is experimental and may be removed at any point” warning. Because it will likely see little use, it would have to be present for a good while or someone would have to test the most popular PKG casks.

@MikeMcQuaid
Copy link
Member

Thanks all. Given the above: passing on this, sorry.

@sambostock sambostock deleted the pkg-target branch March 6, 2023 03:54
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants