Skip to content

Conversation

vraravam
Copy link

  • 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?

  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 vraravam force-pushed the login-item branch 17 times, most recently from c97b261 to eaba8a8 Compare April 12, 2025 09:42
@vraravam
Copy link
Author

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 vraravam marked this pull request as draft April 12, 2025 10:07
@vraravam vraravam force-pushed the login-item branch 4 times, most recently from 7db01f2 to ca5e83a Compare April 13, 2025 00:50
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.

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

@vraravam
Copy link
Author

vraravam commented Apr 14, 2025

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

@MikeMcQuaid
Copy link
Member

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

homepage "https://brew.sh/"

app "Caffeine.app"
login_items "Caffeine.app"
Copy link
Member

Choose a reason for hiding this comment

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

I think the dsl here should be login_item, and repeated if there are multiple.
This will keep it consistent with the other artifacts, that aren't pluralised.

Copy link
Author

Choose a reason for hiding this comment

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

do you mean like how multiple language blocks are supported in the DSL?

Copy link
Member

Choose a reason for hiding this comment

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

I'm referring to other artifacts such as app, binary, artifact, font, qlplugin, etc..

Copy link
Author

Choose a reason for hiding this comment

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

If we look at the everything.rb DSL, there are other stanzas that are pluralized as well maybe?

Copy link
Member

Choose a reason for hiding this comment

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

All of the artifact stanzas (which I think a login item will most closely relate to) are not pluralised.


suite
app
pkg
installer
binary
manpage
colorpicker
dictionary
font
input_method
internet_plugin
keyboard_layout
prefpane
qlplugin
mdimporter
screen_saver
service
audio_unit_plugin
vst_plugin
vst3_plugin

For context, where multiple items doc the same type are allowed and provided the stanza is repeated rather than passing an array to the stanza.

==> Downloading file:.*caffeine.zip
Already downloaded: .*--caffeine.zip
==> Uninstalling Cask local-caffeine
==> Skipping processing of login_items
Copy link
Member

Choose a reason for hiding this comment

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

I have a concern that there may be side effects of skipping the uninstallation of login_items.
I understand having the option to automatically register a login_item, but is the idea here to be able to skip uninstalling them? The reason we generally aim to remove as much as possible with brew uninstall (which also runs with brew upgrade) is to not leave things behind that aren't connected to the application anymore.

What happens if the login_item changes between versions?

Copy link
Author

Choose a reason for hiding this comment

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

Agree - I was also facing the same conundrum. This is predominantly for the usecase where the user has chosen to not uninstall by using --no-login-items CLI switch. By default, the registered login items will be uninstalled (I think that's how I have coded it?)

@vraravam vraravam force-pushed the login-item branch 3 times, most recently from 741c696 to 58cfc5c Compare June 14, 2025 11:07
@vraravam vraravam force-pushed the login-item branch 3 times, most recently from 436f85f to 8deaad1 Compare August 1, 2025 07:04
@bevanjkay
Copy link
Member

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

@vraravam
Copy link
Author

vraravam commented Aug 11, 2025

@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 vraravam force-pushed the login-item branch 3 times, most recently from d934f36 to 7b5649d Compare August 16, 2025 01:45
@vraravam vraravam force-pushed the login-item branch 2 times, most recently from b3df5ff to a2956ad Compare August 19, 2025 14:14
@vraravam vraravam force-pushed the login-item branch 5 times, most recently from c846954 to 4d6a915 Compare September 5, 2025 22:49
@vraravam vraravam force-pushed the login-item branch 4 times, most recently from 78c112e to 6f75d50 Compare September 13, 2025 23:27
@vraravam vraravam force-pushed the login-item branch 2 times, most recently from a84b1a3 to af24c14 Compare September 16, 2025 09:16
…the top-level

Fix issues found when running brew cli commands (upgrade, update, install)
Incorporate 'login_items' into 'info' command output (not yet verified)
Incorporate 'login_items' option for 'reinstall' command

WIP: Adding placeholder for using osascript to register/unregister login items
Mark for deprecation
Trying to fix broken unit test
Fix ruby version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants