Allow specifying `pkg` `target:` #14393

sambostock · 2023-01-20T20:28:22Z

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`.

sambostock · 2023-01-20T20:30:25Z

...ary/Homebrew/test/support/fixtures/cask/Casks/with-pkg-target-current-user-home-directory.rb

+
+  pkg "MyFancyPkg/Fancy.pkg", target: "CurrentUserHomeDirectory"
+
+  uninstall pkgutil: "my.fancy.package"


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.

sambostock · 2023-01-20T20:31:54Z

Library/Homebrew/test/cask/artifact/pkg_spec.rb

+                         "-target", "CurrentUserHomeDirectory"],
+          sudo:         false,


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 · 2023-01-20T21:20:52Z

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...

sambostock · 2023-01-20T21:22:00Z

Library/Homebrew/cask/artifact/pkg.rb

@@ -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"


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 · 2023-01-23T02:18:13Z

@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

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 · 2023-02-14T00:07:24Z

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.

MikeMcQuaid · 2023-02-14T11:14:31Z

@Homebrew/cask thoughts here?

miccal · 2023-02-14T11:58:16Z

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 · 2023-02-14T16:35:16Z

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 · 2023-02-14T16:44:48Z

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

sambostock force-pushed the pkg-target branch from fc8ca1f to e325dc6 Compare January 20, 2023 20:28

sambostock mentioned this pull request Jan 20, 2023

Add obs-advanced-scene-switcher cask Homebrew/homebrew-cask#139807

Merged

9 tasks

sambostock commented Jan 20, 2023

View reviewed changes

MikeMcQuaid requested a review from a team January 23, 2023 13:29

MikeMcQuaid reviewed Jan 23, 2023

View reviewed changes

github-actions bot added the stale No recent activity label Feb 14, 2023

github-actions bot removed the stale No recent activity label Feb 14, 2023

MikeMcQuaid closed this Feb 14, 2023

sambostock deleted the pkg-target branch March 6, 2023 03:54

github-actions bot added the outdated PR was locked due to age label Apr 6, 2023

github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying `pkg` `target:` #14393

Allow specifying `pkg` `target:` #14393

sambostock commented Jan 20, 2023 •

edited

Loading

sambostock Jan 20, 2023

sambostock Jan 20, 2023

sambostock commented Jan 20, 2023

sambostock Jan 20, 2023

bevanjkay commented Jan 23, 2023 •

edited

Loading

MikeMcQuaid left a comment

github-actions bot commented Feb 14, 2023

MikeMcQuaid commented Feb 14, 2023

miccal commented Feb 14, 2023 •

edited

Loading

vitorgalvao commented Feb 14, 2023

MikeMcQuaid commented Feb 14, 2023


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

		uninstall pkgutil: "my.fancy.package"

	sudo = target != "CurrentUserHomeDirectory"
	sudo = target != :CurrentUserHomeDirectory

Allow specifying pkg target: #14393

Allow specifying pkg target: #14393

Conversation

sambostock commented Jan 20, 2023 • edited Loading

sambostock Jan 20, 2023

Choose a reason for hiding this comment

sambostock Jan 20, 2023

Choose a reason for hiding this comment

sambostock commented Jan 20, 2023

sambostock Jan 20, 2023

Choose a reason for hiding this comment

bevanjkay commented Jan 23, 2023 • edited Loading

MikeMcQuaid left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 14, 2023

MikeMcQuaid commented Feb 14, 2023

miccal commented Feb 14, 2023 • edited Loading

vitorgalvao commented Feb 14, 2023

MikeMcQuaid commented Feb 14, 2023

Allow specifying `pkg` `target:` #14393

Allow specifying `pkg` `target:` #14393

sambostock commented Jan 20, 2023 •

edited

Loading

bevanjkay commented Jan 23, 2023 •

edited

Loading

miccal commented Feb 14, 2023 •

edited

Loading