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

deps: Use CLI::Parser to parse args #5587

Merged
merged 1 commit into from
Jan 24, 2019

Conversation

GauthamGoli
Copy link
Contributor

  • 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 tests with your changes locally?

#1860

@MikeMcQuaid MikeMcQuaid merged commit b4d02d0 into Homebrew:master Jan 24, 2019
@blogabe
Copy link

blogabe commented Jan 24, 2019

@MikeMcQuaid I could be wrong, but I believe this change unexpectedly breaks some behavior. For those of us that are beginning to host formulae with options on our own taps, we were able to execute the following command to list all the dependencies for a formula with requested options:
brew deps -n --include-build --include-requirements <personaltap/formula> <formula options>

I was fiddling with this last night and it worked. This morning, and I assume after this change was introduced, the above command outputs Error: invalid options: <formula_options>

I'm hoping we can re-introduce this functionality back based on your comments in Homebrew/homebrew-core#31510 being open to PRs to help facilitate the recent push in building from source and formula w/ options to personal taps?

@MikeMcQuaid
Copy link
Member

I'm hoping we can re-introduce this functionality back based on your comments in Homebrew/homebrew-core#31510 being open to PRs to help facilitate the recent push in building from source and formula w/ options to personal taps?

I want to facilitate that but in this case the formula options passthrough was rather accidental and not documented behaviour. I can't see a way of restoring it that doesn't effectively disable the options parser here. I'm not sure I understand your use-case, though, so perhaps I can suggest another workaround?

@blogabe
Copy link

blogabe commented Jan 24, 2019

Thank you, Mike. I used the above command to help sort out if a particular formula option added too many dependencies and weighed down my binary. It allowed me to balance the trade off between a desired vs necessary option and how that impacted the overall number of dependencies.

FWIW, I did test this out with the older version of deps.rb and it worked as expected. If there's any way you can keep your changes, but still allow for formula options to be included to list out a complete list of dependencies, that would be great and much welcomed.

@MikeMcQuaid
Copy link
Member

If there's any way you can keep your changes, but still allow for formula options to be included to list out a complete list of dependencies, that would be great and much welcomed.

Sorry, that's not possible. We'll review a PR that does that but I don't want the option parser work to block on the changing of undocumented behaviour.

@MikeMcQuaid
Copy link
Member

@blogabe @RandomDSdevel I'm sad and confused to see that I was preemptively blocked from that organisation without having any interactions there. I actively support and encourage the creation of portage-brew-, personally; I think forks are healthy things for open source projects.

@blogabe
Copy link

blogabe commented Jan 25, 2019

Sorry to hear about that @MikeMcQuaid. Maybe @RandomDSdevel can comment. I haven't admin'ed on the fork other than commenting on some issues and trying to publicize the project.

@RandomDSdevel
Copy link
Contributor

@MikeMcQuaid, @blogabe:

@blogabe @RandomDSdevel I'm sad and confused to see that I was preemptively blocked from that organisation without having any interactions there. I actively support and encourage the creation of portage-brew-, personally; I think forks are healthy things for open source projects.

Sorry to hear about that @MikeMcQuaid. Maybe @RandomDSdevel can comment. I haven't admin'ed on the fork other than commenting on some issues and trying to publicize the project.

     I wanted some assurance that portage-brew could make policy and development decisions independently of Homebrew upstream proper. Now that you've implied I have it, I'll unblock you. Can we keep further discussion over there in that organization, though, please? I don't want to run off on an unmanageable tangent here.

@MikeMcQuaid
Copy link
Member

Now that you've implied I have it, I'll unblock you.

@RandomDSdevel Thanks.

Can we keep further discussion over there in that organization, though, please?

Well, I couldn't comment there until now 😕. I'm not planning on submitting any particular thoughts or feedback there (although I did read through the repo) but I just wanted you to know my door is open in terms of making it easier for your to run your project.

@RandomDSdevel
Copy link
Contributor

RandomDSdevel commented Jan 29, 2019

@blogabe, CC @MikeMcQuaid:

     Getting back on topic, I see that #5643 introduces a 'formula_options' function into 'Library/Homebrew/cli_parser.rb' (after moving it there from its original location in 'install.rb,') so maybe that could be used here to restore the handling of formula options to 'brew deps' (that is, at least, if I'm correctly interpreting that the intent of that code is for it to be used by other commands when they need to parse formula options?)

@MikeMcQuaid
Copy link
Member

     Getting back on topic, I see that #5643 introduces a 'formula_options' function into 'Library/Homebrew/cli_parser.rb' (after moving it there from its original location in 'install.rb,') so maybe that could be used here to restore the handling of formula options to 'brew deps' (that is, at least, if I'm correctly interpreting that the intent of that code is for it to be used by other commands when they need to parse formula options?)

If someone adds it to the relevant location to deps.rb in a pull request and adjusts the documentation I'l likely merge it.

@MikeMcQuaid MikeMcQuaid mentioned this pull request Jan 30, 2019
6 tasks
@lock lock bot added the outdated PR was locked due to age label Feb 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Feb 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants