Closed Rylan12 closed 3 years ago
Thanks for doing this work @Rylan12, I appreciate the sheer speed & willingness to help ππ».
Now, you may only specify one of the top-level options per run. If this is not desirable it can be fixed, but I just assumed that doing that was the intention.
Would this still make it possible for users to pass brew autoupdate start 18000 --debug
? I'm sort of using the --debug
flag as a dirty hack to launch Autoupdate immediately rather than waiting for the first invocation, mostly because it speeds up testing significantly but also because some people have asked to be able to start it immediately rather than waiting. By default the new(ish) default behaviour is to wait before running for the first time.
I have not tested this extensively, though, so it's probably worth doing some testing to make sure nothing is super broken internally.
At some point I intend to look into setting up a CI system to run autoupdate
with immediate effect and then see if brew
is updated. I've always been more than happy to test things locally extensively before pushing them but as you & other maintainers contribute here and there that's going to become less of a practical workflow for everyone involved. I'm happy to poke at this locally though and make sure it's still behaving as I'd expect.
Finally, I'd like to recommend using subcommands instead of options for start, stop, delete, and status. This would bring this command more in-line with the analytics, completions, and services commands
Looking over the syntax/style of bundle
earlier now this tap is part of Homebrew proper I agree it makes sense to follow that style, and had started thinking loosely about how to implement it. I need to sort of catch up on the way bundle
is structured and make sure I understand it properly but yeah, no disagreements here that it makes sense to progressively adjust things to that style.
This would require deprecating the old versions, though, so it would take a little bit o effort (I'd be happy to help out if desired).
Would it be possible to alias one command onto the other, essentially making it seamless for the user?
Would this still make it possible for users to pass
brew autoupdate start 18000 --debug
? I'm sort of using the--debug
flag as a dirty hack to launch Autoupdate immediately rather than waiting for the first invocation, mostly because it speeds up testing significantly but also because some people have asked to be able to start it immediately rather than waiting. By default the new(ish) default behaviour is to wait before running for the first time.
Yes, this should still work. --debug
(and --verbose
, --quiet
, and --help
) are all global options that all V2 commands can use. So no need to specify it explicitly unless you want to give it a custom description (the default is "Display any debugging information.").
At some point I intend to look into setting up a CI system to run
autoupdate
with immediate effect and then see ifbrew
is updated. I've always been more than happy to test things locally extensively before pushing them but as you & other maintainers contribute here and there that's going to become less of a practical workflow for everyone involved. I'm happy to poke at this locally though and make sure it's still behaving as I'd expect.
I think this shouldn't be too difficult to do. I think if you start by checking git rev-parse HEAD
, then do git -C $(brew --repository) reset --hard HEAD^
, you can easily checkout the previous commit. Then run brew autoupdate --start 18000 --debug
, maybe wait a few second, and then compare the new git rev-parse HEAD
that might do it. A problem might be, though, that the upstream repo is updated in that timeframe... I'll think about it.
Looking over the syntax/style of
bundle
earlier now this tap is part of Homebrew proper I agree it makes sense to follow that style, and had started thinking loosely about how to implement it. I need to sort of catch up on the waybundle
is structured and make sure I understand it properly but yeah, no disagreements here that it makes sense to progressively adjust things to that style.
π
Would it be possible to alias one command onto the other, essentially making it seamless for the user?
Yeah, I don't see why not. The help text could only list one type but behind the scenes they could both be used I'm pretty sure. Of course, if you want to free up the old options (which you probably wouldn't need to do), you would probably want to deprecate it. I don't think a deprecation would be that difficult, but also probably not necessary if you want to support both for the near future.
Should I look into switching to prefer using brew autoupdate start
over brew autoupdate --start
or just stick with this for now?
Yes, this should still work. --debug (and --verbose, --quiet, and --help) are all global options that all V2 commands can use.
That's good ππ». I keep going back and forth on RunAtLoad
; we/I removed it in https://github.com/Homebrew/homebrew-autoupdate/commit/a7de771abcf60d17284ca127384acb3d8899f6d2 because Apple has become grouchy about its usage over the years, but I also feel like the expected behaviour of an autoupdate
tool is that it starts from the moment you launch it, not it starts and then doesn't do its first update for 24 hours. I re-added it in https://github.com/Homebrew/homebrew-autoupdate/commit/12777968a402534760eeab0803aa7ab31d94933b so I could test locally immediately with a single flag.
A problem might be, though, that the upstream repo is updated in that timeframe... I'll think about it.
An alternative could be reading the log file, which as long as the autoupdate
command was launched & executed would be generated and have some content, even if that content was Already up-to-date
. That would show successful execution without needing to necessarily check the git repo actually moved?
I don't think a deprecation would be that difficult, but also probably not necessary if you want to support both for the near future.
I slightly lean towards supporting start
and --start
for example simply because the latter has been the syntax here for 6 years and I feel a little unkind suddenly starting to shout at users for using flags that have remained predictable that long. It depends on complexity; if it adds significant complexity/burden I'd be open to changing it wholesale rather than going down the alias route.
Okay I pushed a commit where I changed to use the subcommand syle instead. If you think this is more confusing or you would rather stick to the existing way, that's totally fine with me. Don't feel compelled to incorporate stuff you disagree with or don't understand just because I worked on it.
Here is the updated help text:
$ brew help autoupdate
Usage: brew autoupdate subcommand [interval] [options]
An easy, convenient way to automatically update Homebrew.
This script will run brew update in the background once every 24 hours (by
default) until explicitly told to stop, utilising launchd.
brew autoupdate start [interval] [options]:
Start autoupdating either once every interval hours or once every 24
hours. Please note the interval has to be passed in seconds, so 12 hours would
be brew autoupdate start 43200. Pass --upgrade or --cleanup to
automatically run brew upgrade and/or brew cleanup respectively. Pass
--enable-notification to send a notification when the autoupdate process has
finished successfully.
brew autoupdate stop:
Stop autoupdating, but retain plist & logs.
brew autoupdate delete:
Cancel the autoupdate, delete the plist and logs.
brew autoupdate status:
Prints the current status of this tool.
brew autoupdate version:
Output this tool's current version.
--upgrade Automatically upgrade your installed
formulae. If the Caskroom exists locally
Casks will be upgraded as well. Must be
passed with start.
--cleanup Automatically clean brew's cache and logs.
Must be passed with start.
--enable-notification Send a notification when the autoupdate
process has finished successfully, if
terminal-notifier is installed & found.
Note that currently a new experimental
notifier runs automatically on macOS Big
Sur, without requiring any external
dependencies. Must be passed with start.
-d, --debug Display any debugging information.
-q, --quiet Make some output more quiet.
-v, --verbose Make some output more verbose.
-h, --help Show this message.
Don't feel compelled to incorporate stuff you disagree with or don't understand just because I worked on it.
I appreciate that attitude, thanks for being so flexible. Nothing you've suggested thus far hasn't been a harmonisation of this tap's syntax with the rest of the similar ones, services
et al., though so I have no issues with it at all. Ultimately it makes sense for all the similar taps to have a mostly similar style both internally and for end users, not least because it means if I get hit by a bus 2 years down the line this tap won't be like interacting with an ancient relic of the past for the rest of the team.
Give me some time today to play around with this locally and run all the commands I normally would to check it over, make sure it's behaving as I'd expect compared to the current syntax, etc but in principle I both like & appreciate the work you've put in here. I'll try and get back to you before the end of today but it's my partner's birthday tomorrow so it might end up being Sunday afternoon sometime potentially. Won't leave it hanging beyond the weekend either way.
thanks for being receptive to this @DomT4.
I've mellowed with age ππΈ. In all seriousness it makes sense & ensures similar-tap behaviours are consistent, which long-term will confuse users less than the "cost" of starting to change the style/structure a little now.
I'm having some issues locally with the --
commands still being functional:
~> brew autoupdate --version
Usage: brew autoupdate subcommand [interval] [options]
An easy, convenient way to automatically update Homebrew.
This script will run brew update in the background once every 24 hours (by
default) until explicitly told to stop, utilising launchd.
brew autoupdate start [interval] [options]:
Start autoupdating either once every interval hours or once every 24
hours. Please note the interval has to be passed in seconds, so 12 hours would
be brew autoupdate start 43200. Pass --upgrade or --cleanup to
automatically run brew upgrade and/or brew cleanup respectively. Pass
--enable-notification to send a notification when the autoupdate process has
finished successfully.
brew autoupdate stop:
Stop autoupdating, but retain plist & logs.
brew autoupdate delete:
Cancel the autoupdate, delete the plist and logs.
brew autoupdate status:
Prints the current status of this tool.
brew autoupdate version:
Output this tool's current version.
--upgrade Automatically upgrade your installed
formulae. If the Caskroom exists locally
Casks will be upgraded as well. Must be
passed with start.
--cleanup Automatically clean brew's cache and logs.
Must be passed with start.
--enable-notification Send a notification when the autoupdate
process has finished successfully, if
terminal-notifier is installed & found.
Note that currently a new experimental
notifier runs automatically on macOS Big
Sur, without requiring any external
dependencies. Must be passed with start.
-d, --debug Display any debugging information.
-q, --quiet Make some output more quiet.
-v, --verbose Make some output more verbose.
-h, --help Show this message.
Error: invalid option: --version
Same for --status
, --start
, --delete
, etc.
The commands themselves seem to be behaving as you'd expect:
~> brew autoupdate start --debug 3600
Homebrew will now automatically update every 1 hours, or on system boot.
successfully made autoupdate
start and immediately launch itself, updating hourly after as expected.
Ah, you're right. It's treating --version
as a flag rather than a named argument. You can get it to work as expected by using brew autoupdate -- --version
, but obviously that's not really a solution.
I don't think we have a built-in way of treating unrecognized options as named args, but that would be nice. Another option might be to add the flags as possibilities but not to show them in the help... I'll need to take a look to see if that's even possible.
That might mean that we'll need to choose only one option for long-term support if we want to keep this as a Ruby V2 function. If that's the case, I'll let you make that decision. I think there's an argument for and against both so I wouldn't stress too much about it. Here are my initial pros/cons that popped into my head, ignore them completely if you'd like:
brew autoupdate --help
or look at the README.If you end up preferring to move permanently to the arguments without the dashes, we can definitely set up some deprecation warnings to warn users that a change is coming before it actually does. I'll be happy to help out with this if that's desired.
I'll try to get back to you soon about whether there's a relatively clean way to handle having both or if we'll need to choose one.
Aye, I'd spent a half hour or so earlier trying to work out if the problem is something related to this tap or something relaxed to Homebrew's handling of this style of syntax, and I couldn't find an easy way to fix the issue without veering off from the intended path forwards of dual-support here. I'd sort of hoped there was a way locally for us to trim the --
from the user's input and convert it to our new style, but I'm not even sure brew is seeing the passed argument when delivered in the --
format.
I also did a search around GitHub for brew autoupdate
, which I kind of wish I hadn't because doing so has done a good job showing me how many people have scripted this tool to launch either as part of their shell configurations or as independent "setup brew"-style scripts with the assumption of the --
syntax being a safe assumption π.
Still think moving to the V2 syntax is worthwhile, because long-term it'll make it easier for people to contribute across the whole Homebrew organisation if they desire rather than contributing to services
, command-not-found
, etc but then coming to contribute here and finding a whole different style. I have a very strong appreciation of consistency. Just a case of how do we get there in a safe, least-breakage-possible manner.
if I'm right in assuming that commands aren't run that regularly, users may miss the deprecation messages and not run the command until they've been totally migrated
Yeah, this is part of the internal debate I've had with myself over the last few months of not really knowing how much the new notifier is being tested because the command execution is basically intended to be untouchable until users manually stop/restart the command. Rolling out new features or changes on something that is essentially "Fire and Forget" has its challenges. Mentally I'm working towards a future of storing the options autoupdate
was launched with somewhere, and then if we need to we can manually stop/update/restart the command but that's not a short-term goal or something that would assist existing users.
I'll try to get back to you soon about whether there's a relatively clean way to handle having both or if we'll need to choose one.
Thanks @Rylan12. Again, I appreciate the work you've put into this. I'm sure it's become something more of a project than you necessarily foresaw, my apologies. If it comes down to it I'd prefer to change the syntax to be consistent and throw in some kind of deprecation warning, but yeah, I share the concern around users very rarely interacting with the command by design.
Good news, @DomT4: I think I've got a version working that supports both subcommands and options.
I've needed to get a little creative with the argument handling so this command is a little bit unique now. Let me know if there are any parts that you'd like me to explain (via either GitHub or code comments).
I've done some testing locally so I think everything should be okay, but I'd still appreciate it if you would give it a go locally to make sure there's nothing obvious that I missed. Even if it's just that there's no error message (or an incorrect/misleading one) shown on an invalid argument combination.
That sounds like great news to me @Rylan12, thank you for investigating further! If there's ever a Homebrew meet-up in future I definitely owe you a drink or two at this point π. Does this add any sort of deprecation notice for the --
or is that a situation better left for a little further down the line? I'll pull this locally tomorrow afternoon and play around with it a bit and get back to you before the end of day.
That sounds like great news to me @Rylan12, thank you for investigating further!
My pleasure! Always happy to help out with this kind of stuff.
Does this add any sort of deprecation notice for the
--
or is that a situation better left for a little further down the line?
Right now there's no deprecation notice, but one can certainly be added if you'd like to actively deprecate and eventually remove those flags. I was working on the assumption that the plan was to support both indefinitely (at least for now) but we can certainly start a deprecation process if you'd like.
I'll pull this locally tomorrow afternoon and play around with it a bit and get back to you before the end of day.
Great, thanks!
My apologies for the slight delay on testing. Pulled it locally today and it looks great to me, handles both the --
and the new syntax seemingly with no issues. Command ran & launched the notification as expected, and also stopped/deleted/displayed the status/displayed the version as expected.
Very much like the fact you've hidden the --
from the help
but left it silently available to prevent breaking existing scripts/etc, that's a really nice & well thought out way of gently encouraging users to use the new syntax & perhaps pick up some existing users along the way whenever they check the help text or such.
I'm happy with this as-is, with an eye to perhaps starting a deprecation process a little further down the road if it looks necessary. If you're happy here I'm happy for us to π’ this. Thank you again for all the work here, the end product is a significant improvement over the current code/syntax style for sure.
I'm happy with this as-is, with an eye to perhaps starting a deprecation process a little further down the road if it looks necessary.
Makes sense to me. Don't hesitate to reach out (GitHub/slack/wherever) at that point if I can be helpful.
If you're happy here I'm happy for us to π’ this.
Great! I'm happy! π’
Thanks for working through this with us!
Shipped ππΎ. Looking forwards to working on future PRs together, really enjoyed the productive process on this one and the end product is something that'll make long-term maintainership & contributor involvement easier.
Likewise! π₯³
This PR converts the
autoupdate
command to a Ruby V2 command.A few things to point out:
brew autoupdate --start --status --version
. Now, you may only specify one of the top-level options per run. If this is not desirable it can be fixed, but I just assumed that doing that was the intention.--start
. It will also complain about any arguments without--start
. When using with--start
, the order doesn't matter. These can be changed if desired.I've also added a new description which I took directly fro the README. Here's the output of
brew help autoupdate
:There is some bold/underline that shows up in the terminal but not here, FYI. I wasn't sure how descriptive to be in the usage string, so I settled on
brew autoupdate subcomand [interval] [options]
. This can, of course, be changed.I have not tested this extensively, though, so it's probably worth doing some testing to make sure nothing is super broken internally.
Finally, I'd like to recommend using subcommands instead of options for
start
,stop
,delete
, andstatus
. This would bring this command more in-line with theanalytics
,completions
, andservices
commands and might make the argument handling a tad simpler. That would mean callingbrew autoupdate start 12345 --upgrade
orbrew autoupdate status
. This would require deprecating the old versions, though, so it would take a little bit o effort (I'd be happy to help out if desired). Totally up to you, though, I won't really push for it as it may very well be a personal preference.Edit: actually, it's probably already able to use the subcommand argument handling but with the
--
. I'll need to think about that.