fetch/build formula from source when modified #41397

dunn · 2015-07-06T02:46:32Z

Is performance something to worry about here?

dunn · 2015-07-06T02:49:33Z

Oh, forgot to add the override flag @DylanLacey mentioned.

dunn · 2015-07-06T02:52:27Z

Nevermind, --force-bottle still works to override it.

dunn · 2015-07-06T02:58:07Z

  1) Error:
BottleHookTests#test_has_no_bottle:
NoMethodError: undefined method `is_modified?' for #<BottleHookTests::FormulaDouble:0x007f9203054078>
    /usr/local/Library/Homebrew/formula_installer.rb:65:in `pour_bottle?'

This is weird since formula_installer requires formula. I must be missing something.

DylanLacey · 2015-07-06T04:57:52Z

Oh bugger. This'll teach me to write an implementation on a RailsCamp and forget to send a PR >.>

dunn · 2015-07-06T05:16:29Z

I mean if yours is better I'd be happy to close this in favor of it.

xu-cheng · 2015-07-06T11:56:55Z

Library/Homebrew/formula.rb

+    old_wd = Pathname.pwd
+    Dir.chdir path.dirname
+    diff = Utils.popen_read("git", "diff", "--diff-filter=M", "--name-only", "#{path}")
+    Dir.chdir old_wd


You can do:

path.parent.cd do diff = ... end

Another thing that you need to check is formula file may be outside git repo.

Another thing that you need to check is formula file may be outside git repo.

That was why I added the exitstatus check; git should return non-zero in those cases. Are there cases where that's insufficient?

DylanLacey · 2015-07-07T00:48:09Z

@dunn I don't believe so; I pushed it as https://github.com/Homebrew/homebrew/pull/41433/files. I just was chastising myself for having and implementing a reasonable idea, then not actually going 100% through forgetfulness and lack of tests :P

tdsmith · 2015-07-07T01:41:21Z

I do think it's important that a solution announces "Formula modified locally; building from source" or similar.

dunn · 2015-07-07T03:37:08Z

Updated; but let me know if this should be closed in favor of @DylanLacey's.

xu-cheng · 2015-07-07T08:12:42Z

Library/Homebrew/formula.rb

+  def modified?
+    path.parent.cd do
+      diff = Utils.popen_read("git", "diff", "--diff-filter=M", "--name-only", "#{path}")
+      return !diff.empty? && $?.exitstatus == 0


return is redundant

dunn · 2015-07-07T22:17:06Z

Still need help interpreting the test failure, I'm afraid.

MikeMcQuaid · 2015-07-08T12:57:17Z

Library/Homebrew/formula.rb

+      diff = Utils.popen_read("git", "diff", "--diff-filter=M", "--name-only", "#{path}")
+      !diff.empty? && $?.exitstatus == 0
+    end
+  end


Also add this method here: https://github.com/Homebrew/homebrew/blob/master/Library/Homebrew/test/test_bottle_hooks.rb#L6-L10

dunn · 2015-07-09T21:19:52Z

OK, I think that covers everything.

dunn · 2015-07-09T23:50:05Z

Not getting those test failures locally.

MikeMcQuaid · 2015-07-10T15:03:08Z

I currently see this message printed twice (it should only be output called once). Once that's fixed we're good to go.

dunn · 2015-07-10T20:17:49Z

Sorry, where are you seeing doubled output?

Also, added .rb to the ohai, since it's the file that's been modified.

MikeMcQuaid · 2015-07-10T20:41:24Z

If I run brew install wget after modifying the formula.

dunn · 2015-07-10T21:56:46Z

Oh, I was testing with brew install when and it doesn't happen for that formula. Bizarre; I'll investigate.

dunn · 2015-07-10T22:04:03Z

It makes sense that we get the message twice for formulae like libtorrent which call pour_bottle?, but I'm not sure why wget does it.

dunn · 2015-07-10T22:43:49Z

Anyway, fixed!

MikeMcQuaid · 2015-07-11T13:31:37Z

Library/Homebrew/cmd/fetch.rb

@@ -32,6 +32,11 @@ def fetch
  def fetch_bottle? f
    return true if ARGV.force_bottle? && f.bottle
    return false unless f.bottle && f.pour_bottle?
+    if formula.file_modified?
+      ohai "#{formula.full_name}.rb is modified; fetching source"


formula.path.to_s.gsub("#{HOMEBREW_PREFIX}/", '') might be a bit better (particularly for stuff from taps).

On narrow tmux/screen panes the path will get cut off, but I think that's fine:

Maybe go for PATH is modified! and then putting the rest fitting on the next line? What do you think?

MikeMcQuaid · 2015-07-11T13:32:24Z

@dunn This is looking awesome. A few more tweaks; almost there!

closes Homebrew#36068

dunn · 2015-07-11T21:06:07Z

Three changes:

Fixed the git diff to detect committed as well as staged changes
changed the ohai to opoo (I think it should stand out a bit from all the other ohais, but either way is OK, really)
Modified the message as below:

closes Homebrew#36068 Closes Homebrew#41397. Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>

dunn mentioned this pull request Jul 6, 2015

Suggested feature: Automagically build bottled brews if their formula has been edited #36068

Closed

dunn force-pushed the no-bottle branch 2 times, most recently from 6418a3c to 9e3057c Compare July 6, 2015 04:57

dunn force-pushed the no-bottle branch from 9e3057c to 77841d1 Compare July 6, 2015 05:02

xu-cheng reviewed Jul 6, 2015
View reviewed changes

DylanLacey mentioned this pull request Jul 7, 2015

Default to not using bottles for edited formula. #41433

Closed

dunn force-pushed the no-bottle branch from 77841d1 to f9c7317 Compare July 7, 2015 03:36

xu-cheng reviewed Jul 7, 2015
View reviewed changes

dunn force-pushed the no-bottle branch from f9c7317 to cd6ad34 Compare July 7, 2015 16:43

MikeMcQuaid reviewed Jul 8, 2015
View reviewed changes

dunn force-pushed the no-bottle branch from cd6ad34 to bf09806 Compare July 9, 2015 17:03

dunn force-pushed the no-bottle branch from bf09806 to 56e4e20 Compare July 9, 2015 22:11

dunn force-pushed the no-bottle branch from 56e4e20 to 839025f Compare July 10, 2015 20:17

dunn force-pushed the no-bottle branch from 839025f to 442d272 Compare July 10, 2015 22:12

MikeMcQuaid reviewed Jul 11, 2015
View reviewed changes

dunn force-pushed the no-bottle branch from 442d272 to 4c4b10c Compare July 11, 2015 17:30

fetch/build formula from source when modified

ec395de

closes Homebrew#36068

dunn force-pushed the no-bottle branch from 4c4b10c to ec395de Compare July 11, 2015 21:02

dunn added the maintainer feedback label Jul 19, 2015

dunn closed this in 1134222 Jul 19, 2015

dunn deleted the no-bottle branch July 19, 2015 03:57

woodruffw pushed a commit to woodruffw-forks/homebrew that referenced this pull request Jul 22, 2015

fetch/build formula from source when modified

a190b95

closes Homebrew#36068 Closes Homebrew#41397. Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>

Homebrew locked and limited conversation to collaborators Jul 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fetch/build formula from source when modified #41397

fetch/build formula from source when modified #41397

dunn commented Jul 6, 2015

dunn commented Jul 6, 2015

dunn commented Jul 6, 2015

dunn commented Jul 6, 2015

DylanLacey commented Jul 6, 2015

dunn commented Jul 6, 2015

xu-cheng Jul 6, 2015

dunn Jul 6, 2015

DylanLacey commented Jul 7, 2015

tdsmith commented Jul 7, 2015

dunn commented Jul 7, 2015

xu-cheng Jul 7, 2015

dunn commented Jul 7, 2015

MikeMcQuaid Jul 8, 2015

dunn commented Jul 9, 2015

dunn commented Jul 9, 2015

MikeMcQuaid commented Jul 10, 2015

dunn commented Jul 10, 2015

MikeMcQuaid commented Jul 10, 2015

dunn commented Jul 10, 2015

dunn commented Jul 10, 2015

dunn commented Jul 10, 2015

MikeMcQuaid Jul 11, 2015

dunn Jul 11, 2015

MikeMcQuaid Jul 11, 2015

MikeMcQuaid commented Jul 11, 2015

dunn commented Jul 11, 2015

fetch/build formula from source when modified #41397

fetch/build formula from source when modified #41397

Conversation

dunn commented Jul 6, 2015

dunn commented Jul 6, 2015

dunn commented Jul 6, 2015

dunn commented Jul 6, 2015

DylanLacey commented Jul 6, 2015

dunn commented Jul 6, 2015

xu-cheng Jul 6, 2015

Choose a reason for hiding this comment

dunn Jul 6, 2015

Choose a reason for hiding this comment

DylanLacey commented Jul 7, 2015

tdsmith commented Jul 7, 2015

dunn commented Jul 7, 2015

xu-cheng Jul 7, 2015

Choose a reason for hiding this comment

dunn commented Jul 7, 2015

MikeMcQuaid Jul 8, 2015

Choose a reason for hiding this comment

dunn commented Jul 9, 2015

dunn commented Jul 9, 2015

MikeMcQuaid commented Jul 10, 2015

dunn commented Jul 10, 2015

MikeMcQuaid commented Jul 10, 2015

dunn commented Jul 10, 2015

dunn commented Jul 10, 2015

dunn commented Jul 10, 2015

MikeMcQuaid Jul 11, 2015

Choose a reason for hiding this comment

dunn Jul 11, 2015

Choose a reason for hiding this comment

MikeMcQuaid Jul 11, 2015

Choose a reason for hiding this comment

MikeMcQuaid commented Jul 11, 2015

dunn commented Jul 11, 2015