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

Create add and remove subcommands for brew bundle #832

Closed

Conversation

johndbritton
Copy link
Member

As discussed in #818

Add two subcommands brew bundle add and brew bundle remove which will add or remove a specified formula to the Brewfile. When the --global option is specified the formula is added or removed from the global Brewfile

I've figured out how to add new subcommands and also figured out how to read in the Brewfile and check if the requested formula is already in the Brewfile.

I'm looking for suggestions on how to add to the Brewfile and write it out to disk. My hope is that there is some in memory way to manipulate the Brewfile and call dump. I looked at Bundle::Dumper but that seems to operate based on what's installed on the system, not what was parsed from the Brewfile.

Additionally, looking for feedback on the command line syntax for how we should handle formulae that are not :brew but are :cask, :tap, :mas, or any other type. For example brew bundle add cask atom.

cmd/bundle.rb Outdated Show resolved Hide resolved
Comment on lines +11 to +13
type = :brew # default to brew
name = args.first # only support one formula at a time for now
options = {} # we don't currently support passing options
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we handle different types of formulae? Is there an existing pattern for how you'd like this handled?

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 it's fine to just support brew for now and people can extend it later if desired. As long as it's documented it can raise UsageError which will print the help text.

Comment on lines 25 to 27
# need some help / pointers here
# is it possible to use Bundle::Dsl to create an in memory representation
#of the brewfile that we read, add an entry and then dump that back to the file?
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where I could use the most help

Copy link
Member

Choose a reason for hiding this comment

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

This is the existing dumper code for this:

def formulae
@formulae ||= begin
@formulae = formulae_info
sort!
end
end
def dump(describe: false, no_restart: false)
requested_formula = formulae.select do |f|
f[:installed_on_request?] || !f[:installed_as_dependency?]
end
requested_formula.map do |f|
brewline = ""
brewline += desc_comment(f[:desc]) if describe && f[:desc]
brewline += "brew \"#{f[:full_name]}\""
args = f[:args].map { |arg| "\"#{arg}\"" }.sort.join(", ")
brewline += ", args: [#{args}]" unless f[:args].empty?
brewline += ", restart_service: true" if !no_restart && BrewServices.started?(f[:full_name])
brewline += ", link: #{f[:link?]}" unless f[:link?].nil?
brewline
end.join("\n")
end

It's probably possible to do that but you may have more luck trying to inject your desired formulae state into the existing logic (overriding @formulae somehow).

@jacobbednarz
Copy link
Member

I'm looking for suggestions on how to add to the Brewfile and write it out to disk.

From my vague recollection, I can't remember anywhere that bundle didn't only care about what was installed on the system. What about slurping in the --file value (handling the default) and appending to the appropriate section? I don't think we need to be too tricky with it; something as simple as grouping/detecting and appending to that section seems more than enough here but open to thoughts on it.

Additionally, looking for feedback on the command line syntax for how we should handle formulae that are not :brew but are :cask, :tap, :mas, or any other type. For example brew bundle add cask atom.

It looks like other parts of the ecosystem are moving to flags which seem suitable to use here too. For example brew bundle add 1password --cask would create a cask "1password". Thoughts?

@jacobbednarz
Copy link
Member

Actually, I lie. There is list which seems to cover pretty much what I outlined above.

parsed_entries = Bundle::Dsl.new(Brewfile.read(global: global, file: file)).entries
Bundle::Lister.list(
parsed_entries,
all: all, casks: casks, taps: taps, mas: mas, whalebrew: whalebrew, brews: brews,
)

@colindean
Copy link
Member

I think we need to handle the case where there are options to add. From a fresh brew bundle dump on my dev machine:

brew "paulp/extras/coursier", args: ["HEAD"]
brew "sshfs", link: false
brew "neovim", args: ["HEAD"]
brew "md5sha1sum", link: false

Will this also support adding a tap to the Brewfile, perhaps with --tap?

cmd/bundle.rb Outdated Show resolved Hide resolved
cmd/bundle.rb Outdated Show resolved Hide resolved
Comment on lines +11 to +13
type = :brew # default to brew
name = args.first # only support one formula at a time for now
options = {} # we don't currently support passing options
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 it's fine to just support brew for now and people can extend it later if desired. As long as it's documented it can raise UsageError which will print the help text.

Comment on lines 21 to 22
# this could also be a noop, or print a friendly message
raise RuntimeError if entry.name == name
Copy link
Member

Choose a reason for hiding this comment

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

Yup, friendly message would be nice.

Comment on lines 25 to 27
# need some help / pointers here
# is it possible to use Bundle::Dsl to create an in memory representation
#of the brewfile that we read, add an entry and then dump that back to the file?
Copy link
Member

Choose a reason for hiding this comment

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

This is the existing dumper code for this:

def formulae
@formulae ||= begin
@formulae = formulae_info
sort!
end
end
def dump(describe: false, no_restart: false)
requested_formula = formulae.select do |f|
f[:installed_on_request?] || !f[:installed_as_dependency?]
end
requested_formula.map do |f|
brewline = ""
brewline += desc_comment(f[:desc]) if describe && f[:desc]
brewline += "brew \"#{f[:full_name]}\""
args = f[:args].map { |arg| "\"#{arg}\"" }.sort.join(", ")
brewline += ", args: [#{args}]" unless f[:args].empty?
brewline += ", restart_service: true" if !no_restart && BrewServices.started?(f[:full_name])
brewline += ", link: #{f[:link?]}" unless f[:link?].nil?
brewline
end.join("\n")
end

It's probably possible to do that but you may have more luck trying to inject your desired formulae state into the existing logic (overriding @formulae somehow).

@johndbritton
Copy link
Member Author

I can't figure out a way to write out a Brewfile from an instance of Bundle::Dsl, so I'm considering a different approach.

Would it be reasonable for brew bundle add and brew bundle remove to add the specified formulae to the list specified in the Brewfile, install those items, and then dump the new state to the Brewfile?

# parse the relevant Brewfile
dsl = Bundle::Dsl.new(Brewfile.read(global: global, file: file))
# check each of the entries in the specified Brewfile
dsl.entries.each do |entry|
# raise an error if the entry already exists in the Brewfile
# this could also be a noop, or print a friendly message
opoo "'#{name}' already exists in Brewfile."
raise RuntimeError if entry.name == name
end
dsl.brew(name, options)
# execute brew bundle
# execute brew bundle dump

I had originally planned to keep updating the Brewfile separate from actually doing the install but I had assumed that there was a straightforward way to update the Brewfile. It seems that we're always just using dump to output the formulae that are currently installed on the system.

I'm open to other ideas / suggestions if anyone has opinions.

@MikeMcQuaid
Copy link
Member

Would it be reasonable for brew bundle add and brew bundle remove to add the specified formulae to the list specified in the Brewfile, install those items, and then dump the new state to the Brewfile?

I think the main issue I'd see with this is if the user has other things that are installed that would also be dumped to the Brewfile.

It seems that we're always just using dump to output the formulae that are currently installed on the system.

Yup.

Something I said in #818:

Adding and removing feels like a step below what you want, right? Presumably you want a way of keeping your (--global?) Brewfile in sync with your installations. How about if there was something like brew bundle install wget --global --dump or something similar that ran brew install for you and, if it passed, redumped the global Brewfile automatically?

How about something like that? brew bundle [install] wget --global --dump and brew bundle [install] wget --dump to write to the global and local Brewfiles respectively (and the install is optional, as already).

@BrewTestBot
Copy link
Member

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.

@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants