brew icon indicating copy to clipboard operation
brew copied to clipboard

Support 'login-items' as a first-class citizen of the CLI

Open vraravam opened this issue 10 months ago • 9 comments

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

  1. First cut of moving login_item from under the uninstall block to the top-level
  2. Fix issues found when running brew update and brew upgrade
  3. Incorporate login_items into info command output (not yet verified)
  4. added login_items option for reinstall command

vraravam avatar Apr 12 '25 03:04 vraravam

Could someone please help? I am not sure if I am going in the right direction or not. Would love to get some feedback. This PR is trying to solve the #19333 ticket.

vraravam avatar Apr 12 '25 10:04 vraravam

@vraravam Looks good so far! Anything specific you need help with?

Thanks @MikeMcQuaid - I am first trying to cover all scenarios/commands from the CLI perspective, and finally will proceed to the actual implementation of the osascript calls. Hope that's fine with you?

Also, I'm trying to ensure that all code that I write have tests - if I miss something (since this is my first time contributing to this codebase, i don't know the contributing guidelines/rules), please do point me in that direction as well.

Based on the CLI commands, do you recon that I am missing any? I have tried with install, reinstall and uninstall.

vraravam avatar Apr 14 '25 07:04 vraravam

@vraravam Sounds good to me! When CI is 🟢 let us know and we can give this a more thorough review. CC @homebrew/cask folks for any thoughts before that, too.

MikeMcQuaid avatar Apr 14 '25 11:04 MikeMcQuaid

* how will this work on Linux

For now, I'm only thinking of supporting macos. If needed, and if someone else can take up the work for linux, we can collaborate (I don't have a linux box to test this on)

* more importantly: how can maintainers know the values are correct? The uninstall blocks are tested by CI, but how will this be tested?

That's the reason the onus on correctness is envisioned to be with the cask's author and not at the framework/tool maintainer. This is similar to how the uninstall stanza's login-items is currently implemented.

vraravam avatar Apr 15 '25 06:04 vraravam

That's the reason the onus on correctness is envisioned to be with the cask's author and not at the framework/tool maintainer. This is similar to how the uninstall stanza's login-items is currently implemented.

If we systematically add a login_item that is incorrect, does that result in a broken login item being added to the system?

bevanjkay avatar Apr 15 '25 06:04 bevanjkay

If we systematically add a login_item that is incorrect, does that result in a broken login item being added to the system?

My assumption is the osascript will return an error code / message if an incorrect login item is tried to be added.

vraravam avatar Apr 15 '25 06:04 vraravam

That's the reason the onus on correctness is envisioned to be with the cask's author and not at the framework/tool maintainer.

Not sure who the tool maintainer is in this case, but the most common scenario for casks is:

  • someone finds a cool new tool
  • makes a pull request to add a cask
  • PR gets merged
  • author is never seen again
  • others, mostly maintainers, fix the cask on updates. Often after users report it broken.

So you can see why I'd want to ensure the CI catches faulty entries here like it does for the uninstall step.

SMillerDev avatar Apr 15 '25 08:04 SMillerDev

i see your point @SMillerDev . I am only banking on the precedent scenario of how the uninstall handles this scenario. If you have any other suggestions, I'm all ears.

vraravam avatar Apr 15 '25 09:04 vraravam

Would love some help in debugging the broken tests. can someone please point me in the general direction of what's wrong?

vraravam avatar Apr 24 '25 03:04 vraravam

@vraravam I can see that you've been chipping away at this PR, and just wanted to ask an additional query here before it goes too deep. I personally feel that the login_item declaration may be more suitable to be considered an "artifact" (defined as "something to install") rather than setting it up as a separate DSL itself. Unless there is a specific reason you haven't gone down this route?

bevanjkay avatar Aug 11 '25 03:08 bevanjkay

@vraravam I can see that you've been chipping away at this PR, and just wanted to ask an additional query here before it goes too deep. I personally feel that the login_item declaration may be more suitable to be considered an "artifact" (defined as "something to install") rather than setting it up as a separate DSL itself. Unless there is a specific reason you haven't gone down this route?

hi @bevanjkay - The only reason I went this route is that login_item was already defined as a nested item(?) under the uninstall stanza/DSL. If I changed it to an artifact, then there can be recipes where an artifact does not necessarily translate to being a login item. The simplest change that I could see was to elevate login_item from under uninstall to being at the top-level (ie sibling) of uninstall.

btw - I have actually stalled, and would appreciate some help from the community on this PR. I plan to restart and catch up (logically) with code patterns that I had missed (just did rebases till now in the past couple of months).

IF someone with more in-depth knowledge could help, that would be really appreciated.

vraravam avatar Aug 11 '25 04:08 vraravam