Add new pour_bottle? DSL #47888

MikeMcQuaid · 2016-01-09T11:10:26Z

I dislike the previous one I created because it’s pretty opaque to users; they don’t know why a bottle is failing to pour. Replace it instead with one that outputs a message.

I’ll obviously add comments/tests once people 👍 the general approach.

xu-cheng · 2016-01-09T11:27:24Z

Library/Homebrew/formula.rb

+    # TODO
+    def pour_bottle?(reason, &block)
+      @pour_bottle_reason = reason
+      @pour_bottle_block = block


Some suggestions:

We should allow no reason. The current pour_bottle? nil looks weird.

If no reason is given, it may be a good idea to offer a default one.

Instead of introducing new pour_bottle_block variable, we can store it in pour_bottle? as define_method(: pour_bottle?, &block). As result, Formula#pour_bottle? should be tagged as private instead of deprecated. And you don't need to check pour_bottle_block existence in formula_installer. In general, it should be help to simplify the code and make it more readable.

We should allow no reason. The current pour_bottle? nil looks weird.

Agreed it's weird. I'd like to us avoid having no reason as it's user-hostile but I've thought of the right reason for this case.

Instead of introducing new pour_bottle_block variable, we can store it in pour_bottle? as define_method

Cool idea, will try it out.

I think there should always be a reason. I cannot think of a situation where we would want to omit it and which default reason might apply then. But if there's a good example, I'd like to hear it.

DomT4 · 2016-01-09T12:44:12Z

Agree with the comment Xu made but this is a lot better (and more obvious) than the existing system.

dunn · 2016-01-09T15:41:06Z

Library/Formula/percona-server.rb

  # shared with the mysql and mariadb formulae.
  def datadir
    @datadir ||= (var/"percona").directory? ? var/"percona" : var/"mysql"
  end

-  def pour_bottle?
+  pour_bottle? do "The bottle needs a #{var}/mysql datadir (yours is #{var}/percona)."


Is do supposed to be at the end of this line?

bfontaine · 2016-01-09T23:36:49Z

I’ve a style issue with a method that ends with a ? but takes arguments.

Edit: @UniqMartin reminded me that we have things like String#start_with? that take arguments. The proposed DSL method here is to change things, not to ask about some state, hence my issue. I won’t actively fight this if other maintainers are happy though.

UniqMartin · 2016-01-09T23:57:35Z

Library/Formula/gcc.rb

@@ -61,8 +61,8 @@ def osmajor

  # The bottles are built on systems with the CLT installed, and do not work
  # out of the box on Xcode-only systems due to an incorrect sysroot.
-  def pour_bottle?
-    MacOS::CLT.installed?
+  pour_bottle? "The bottle needs the Xcode CLT to be installed." do


I find “Xcode CLT” to be massively confusing. Apple refers to them as “Command Line Tools for Xcode” and we usually just refer as “CLT” to them. Can we make that “The bottle needs the Command Line Tools to be installed.” instead (here and in all other formulae with the same condition)?

UniqMartin · 2016-01-10T00:42:26Z

I like the idea of always providing a reason a lot. In the hopes that this PR gets merged, I already took the liberty to comment on minor stuff that should end up in the final implementation, one way or another.

However, the pour_bottle? <reason> { <condition> } DSL looks really weird to me. It also currently limits us to a single condition, if I'm not mistaken. Maybe we can make it look something like this:

bottle_needs do
  satisfy { OS::Mac::CLT.installed? }
  fail_message "The bottle needs the Command Line Tools to be installed."
end

The same formula could easily have a second bottle_needs block then:

bottle_needs do
  satisfy { HOMEBREW_PREFIX.to_s == "/usr/local" }
  fail_message "The bottle needs the prefix to be /usr/local (yours is #{HOMEBREW_PREFIX})."
end

I'm not entirely happy with the DSL keywords bottle_needs, satisfy, and fail_message, so naming suggestions are more than welcome.

MikeMcQuaid · 2016-01-10T18:19:13Z

I’ve a style issue with a method that ends with a ? but takes arguments.

I think it's fine as ? indicates the return value will be boolean rather than arguments not being used. I've seen ? used with arguments pretty often.

MikeMcQuaid · 2016-02-11T20:46:00Z

I've updated this based on some of the comments but left the DSL mostly intact as I think it's good for the use-case we need it for.

UniqMartin · 2016-02-11T21:58:28Z

I still like the general idea and see it as an improvement. At the same time I continue to have a hard time to wrap my head around the proposed DSL. I strongly feel that the order of <reason> and <condition> in pour_bottle? <reason> { <condition> } feels backwards to me. Given that we should be using this DSL rather sparingly, I think we can safely trade terseness for being more expressive and I hope if we can come up with better keywords for the suggestion in #47888 (comment) this will ultimately result in greater clarity.

Sorry if I'm kinda repeating myself, but I'm too convinced this is going to be better long-term. However, if my proposal for an alternative DSL is really not that convincing, i.e. it turns out I'm the only one who thinks like that, feel free to ignore me. (I still favor this PR as-is over doing nothing.)

UniqMartin · 2016-02-12T00:38:53Z

@MikeMcQuaid Addendum: If you want to move all core formulae to the new DSL, the recently changed fontconfig needs to be updated, too.

xu-cheng · 2016-02-12T08:10:49Z

Library/Homebrew/formula.rb

+    # If `pour_bottle?` returns `false` the user-visible reason to display for
+    # why they cannot use the bottle.
+    # @private
+    attr_reader :pour_bottle_reason


Could we have a better name for this variable? Because it's actually not pouring reason.

:bottle_sealed_reason
:cap_sealed_reason
:bottle_opener_denied_reason
:pour_bottle_blocked_reason
:pour_bottle_denied_reason
:bottle_denied_reason
:bottle_unavailable_reason
:pour_denied_reason
:pour_bottle_unavailable_reason
:bring_your_own_glass_reason

Some variation of pour_bottle_impossible or bottle_unavailable_reason if you're not wed to the pour_* wording seems okay to me.

xu-cheng · 2016-02-12T08:15:27Z

Instead of introducing new pour_bottle_block variable, we can store it in pour_bottle? as define_method(:pour_bottle?, &block). As result, Formula#pour_bottle? should be tagged as private instead of deprecated. And you don't need to check pour_bottle_block existence in formula_installer. In general, it should be help to simplify the code and make it more readable.

Repost it since it's buried in folded comment. What's your opinion of this? @MikeMcQuaid

MikeMcQuaid · 2016-02-12T09:01:10Z

@UniqMartin My reasoning for the current approach is that is resembles keg_only and resource DSLs. I'd like more @Homebrew/maintainers input on which DSL they prefer, though.

MikeMcQuaid · 2016-02-12T09:01:50Z

@xu-cheng I'm not sure I like using define_method like that; it feels odd to me to define a new class method based on a block.

xu-cheng · 2016-02-12T09:05:46Z

@xu-cheng I'm not sure I like using define_method like that; it feels odd to me to define a new class method based on a block.

🆒

As for DSL. how about something like(as similar to DSL in Requirement):

pour_bottle do
  reason ""
  satisfy do
    ...
  end
end

MikeMcQuaid · 2016-02-12T09:14:30Z

@xu-cheng Seems you agree with @UniqMartin on the DSL. I'll implement it that way 👍

ilovezfs · 2016-02-13T16:52:50Z

Library/Formula/lua.rb

@@ -12,7 +12,7 @@ class Lua < Formula
    sha256 "a84d3ebd9afa4a61b0120471e5a0dfcc670d294701a64edebd25fcc815fe76f8" => :mavericks
  end

-  def pour_bottle?
+  pour_bottle? "The bottle needs installed into /usr/local." do


s/needs/needs to be/

MikeMcQuaid · 2016-02-14T19:58:48Z

Pushed an update with the new DSL. The main issue I'm having is I cannot get it to execute in the current formula's scope. Run brew install percona-server with this branch checked-out to see what I mean. If this cannot be resolved I think it's an argument for the previous implementation (but I think it probably can be).

xu-cheng · 2016-02-15T03:58:28Z

Library/Formula/fontconfig.rb

@@ -60,7 +61,7 @@ def post_install
 --- a/fc-cache/fc-cache.c
 +++ b/fc-cache/fc-cache.c
 @@ -187,13 +187,8 @@ scanDirs (FcStrList *list, FcConfig *config, FcBool force, FcBool really_force,
- 	
+


Space cannot be removed when it's from patch.

xu-cheng · 2016-02-15T06:25:28Z

The main issue I'm having is I cannot get it to execute in the current formula's scope.

One possible solution I can think of:

class PourBottleCheck
  def initialize(formula)
    @formula = formula
  end

  def reason(reason)
    @formula.pour_bottle_check_unsatisfied_reason = reason
  end

  def satisfy(&block)
    @formula.send(:define_method, :pour_bottle?, &block)
  end
end

And use Formula#pour_bottle? directly to eval the block.

ilovezfs · 2016-02-15T07:11:57Z

@xu-cheng Isn't this a more general problem with all of the dsl methods in a formula though? Shouldn't they all have transparent access to the formula's environment, variables, etc.?

MikeMcQuaid · 2016-02-16T11:31:03Z

Updated with all comments addressed. Thanks for the sample code, @xu-cheng, worked great 👍

UniqMartin · 2016-02-16T17:23:47Z

Library/Homebrew/software_spec.rb

+  end
+
+  def reason(reason)
+    @formula.pour_bottle_check_unsatisfied_reason(reason)


This looks like it should have been an assignment instead.

This is an assignment using our attr_rw setter method.

Oh, right; I wasn't even aware that attr_rw is a custom thing. Thanks for the clarification!

I think it's better to use attr_accessor rather than attr_rw here. The reason is attr_rw is majorly used as DSL method in the codebase. It's more straightful to use assignment here.

Also any chance to have a shorter variable name here?

I think it's better to use attr_accessor rather than attr_rw here.

🆗

Also any chance to have a shorter variable name here?

Given it's not used widely or externally and there was some justified confusion about the existing name I'd rather it was super explicit.

Given it's not used widely or externally and there was some justified confusion about the existing name I'd rather it was super explicit.

I second that.

UniqMartin · 2016-02-16T17:31:29Z

Thanks for implementing the suggested DSL! I'm very happy with how it looks in the formulae.

Do you have any idea why the build failed on Jenkins with a pour_bottle?-related error, but Travis did its thing without failure until it was stopped by the timeout?

This allows there to be a user-visible description of why a bottle is not being installed.

MikeMcQuaid · 2016-02-18T10:21:32Z

Added tests, addressed comments and pushed. Thanks!

Check to see if `HEAD` is the same as what we have locally. If it is: don't bother to `git fetch`. Closes #47888. Closes #49219. Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>

xu-cheng added features maintainer feedback labels Jan 9, 2016

xu-cheng reviewed Jan 9, 2016
View reviewed changes

dunn reviewed Jan 9, 2016
View reviewed changes

UniqMartin reviewed Jan 9, 2016
View reviewed changes

xu-cheng reviewed Feb 12, 2016
View reviewed changes

ilovezfs reviewed Feb 13, 2016
View reviewed changes

xu-cheng reviewed Feb 15, 2016
View reviewed changes

UniqMartin reviewed Feb 16, 2016
View reviewed changes

MikeMcQuaid added 12 commits February 18, 2016 10:19

software_spec: add new bottle check class.

b9d6c8f

formula: add new pour_bottle DSL with reason.

0e9b861

This allows there to be a user-visible description of why a bottle is not being installed.

formula_installer: cleanup existing pour_bottle logic.

7c2883f

formula_installer: improve bottle cellar message.

804daa8

formula_installer: use new pour_bottle DSL.

3afc432

gcc: use new pour_bottle DSL.

45500b7

lua: use new pour_bottle DSL.

93e43db

percona-server: use new pour_bottle DSL.

9c2aca2

python: use new pour_bottle DSL.

c06b92e

python3: use new pour_bottle DSL.

a19e96c

kafka: use new pour_bottle DSL.

b70c469

fontconfig: use new pour_bottle DSL.

dab56e4

MikeMcQuaid closed this in d0fb5d5 Feb 18, 2016

MikeMcQuaid deleted the pour_bottle_reason branch February 18, 2016 10:43

Homebrew locked and limited conversation to collaborators Jul 10, 2016

Add new pour_bottle? DSL #47888

Add new pour_bottle? DSL #47888

Conversation

MikeMcQuaid commented Jan 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DomT4 commented Jan 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfontaine commented Jan 9, 2016

Choose a reason for hiding this comment

UniqMartin commented Jan 10, 2016

MikeMcQuaid commented Jan 10, 2016

MikeMcQuaid commented Feb 11, 2016

UniqMartin commented Feb 11, 2016

UniqMartin commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xu-cheng commented Feb 12, 2016

MikeMcQuaid commented Feb 12, 2016

MikeMcQuaid commented Feb 12, 2016

xu-cheng commented Feb 12, 2016

MikeMcQuaid commented Feb 12, 2016

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 14, 2016

Choose a reason for hiding this comment

xu-cheng commented Feb 15, 2016

ilovezfs commented Feb 15, 2016

MikeMcQuaid commented Feb 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UniqMartin commented Feb 16, 2016

MikeMcQuaid commented Feb 18, 2016