tap: run readall when tapping. #396

MikeMcQuaid · 2016-06-23T18:32:26Z

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

This will prevent tapping an tap with syntax errors from causing issues
for users.

Fixes #58.

xu-cheng · 2016-06-23T18:40:59Z

Library/Homebrew/tap.rb

@@ -221,10 +221,11 @@ def install(options = {})

    begin
      safe_system "git", *args
+      safe_system "brew", "readall", name


We should use HOMEBREW_BREW_FILE

I guess it doesn't hurt to check the aliases of the to-be-installed tap, too. Thus, I'd suggest:

safe_system HOMEBREW_BREW_FILE, "readall", "--aliases", name

MikeMcQuaid · 2016-06-30T21:47:16Z

Have addressed feedback.

UniqMartin · 2016-06-30T23:28:01Z

I guess this is the first time an integration test tries to run a brew subprocess and our test environment isn't really prepared to handle that. I'm afraid this PR is more involved than one would expect.

MikeMcQuaid · 2016-07-03T19:58:28Z

improved error message output on readall/tap failure
fixed the integration tests handling

UniqMartin · 2016-07-04T06:32:48Z

Library/Homebrew/test/test_integration_cmds.rb

    Bundler.with_original_env do
-      ENV["HOMEBREW_BREW_FILE"] = HOMEBREW_PREFIX/"bin/brew"
+      ENV["HOMEBREW_BREW_FILE"] = cmd_exec.join " "


I don't think this is going to work and, more importantly, I don't think we want to do this. HOMEBREW_BREW_FILE should be a path to the bin/brew script, not a loose collection of arguments to be interpreted by a shell. I think fixing nested brew invocations in integration tests definitely needs some more thought and likely a separate PR.

It does work. Given it's affecting only the integration tests and they pass I personally don't see the problem. Do you have a suggestion for another approach?

(note tests are now 💚)

One problem is that these instances re-use the same test identifier and thus overwrite each other's coverage data when running with --coverage. There might be more issues, but to be honest, I haven't spent much time thinking about this.

Assuming this is what cmd_id_from_args/HOMEBREW_INTEGRATION_TEST are doing: the arguments here will be handled correctly.

And the coverage looks correct from local inspection.

Yes, that's what cmd_id_from_args is for. The nested brew readall spawned from brew tap doesn't go through this code again (unless I'm missing something). Thus it re-uses the same ID and gets its result overwritten by the brew tap process that finishes later.

If this worked as it probably should, I'd expect brew tests --coverage --only=integration_cmds/tap to have nonzero coverage for cmd/readall.rb, but that's not something I can observe locally.

I see what you mean, will check this out further.

MikeMcQuaid · 2016-07-04T17:32:21Z

Reworked this so it doesn't run a brew subprocess.

UniqMartin · 2016-07-05T14:51:51Z

Library/Homebrew/readall.rb

+
+module Readall
+  class << self
+    def valid_ruby_syntax?(ruby_files)


Would you mind adding a check that ruby_files is a Queue instance or alternatively convert it to a queue if it isn't already one? I think it's not obvious on the call site that a Queue needs to be supplied and if an Array is used, then the code will try to use it (which is not thread-safe), will fortunately fail, but with a rather cryptic error message:

TypeError: no implicit conversion of true into Integer

UniqMartin · 2016-07-05T15:17:20Z

Big 👍 on the readall refactoring. Would you mind splitting the commit into multiple commits? I think this will make the Git history much clearer and should be relatively easy to do. Suggestion:

brew readall refactoring.
Use of the refactored readall in brew tap.

MikeMcQuaid · 2016-07-08T16:38:25Z

Addressed all commits, split into two commits.

UniqMartin · 2016-07-08T16:49:38Z

Spotted one minor style issue (didn't notice any others, but maybe RuboCop sees more), thus a final re-run of RuboCop on the modified files before pulling this would be appreciated.

Other than that, it looks good to me. Thanks for addressing everything! 👍 when 🍏.

This will prevent tapping an tap with syntax errors from causing issues for users. Fixes #58.

On systems prior to 10.9, formulae that use CVS as a download source check whether the installed Xcode already provides CVS to avoid adding a dependency on the `cvs` formula. Unfortunately, if no Xcode is installed the check fails with undefined method `<' for nil:NilClass causing the formula to become unloadable. This in turn causes some taps to be untappable since #396 added the `readall` check on `tap`. Closes #508.

* readall: move readall logic to new class. * tap: run readall when tapping. This will prevent tapping an tap with syntax errors from causing issues for users. Fixes Homebrew#58.

On systems prior to 10.9, formulae that use CVS as a download source check whether the installed Xcode already provides CVS to avoid adding a dependency on the `cvs` formula. Unfortunately, if no Xcode is installed the check fails with undefined method `<' for nil:NilClass causing the formula to become unloadable. This in turn causes some taps to be untappable since Homebrew#396 added the `readall` check on `tap`. Closes Homebrew#508.

BrewTestBot added the in progress Maintainers are working on this label Jun 23, 2016

xu-cheng reviewed Jun 23, 2016
View reviewed changes

UniqMartin reviewed Jul 4, 2016
View reviewed changes

UniqMartin reviewed Jul 5, 2016
View reviewed changes

MikeMcQuaid added 2 commits July 8, 2016 20:24

readall: move readall logic to new class.

fb29d07

tap: run readall when tapping.

f4e7f5e

This will prevent tapping an tap with syntax errors from causing issues for users. Fixes #58.

MikeMcQuaid merged commit 4da9905 into Homebrew:master Jul 9, 2016

MikeMcQuaid deleted the tap-readall branch July 9, 2016 12:51

BrewTestBot removed the in progress Maintainers are working on this label Jul 9, 2016

Homebrew locked and limited conversation to collaborators May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tap: run readall when tapping. #396

tap: run readall when tapping. #396

MikeMcQuaid commented Jun 23, 2016 •

edited

Loading

xu-cheng Jun 23, 2016

UniqMartin Jun 24, 2016

MikeMcQuaid commented Jun 30, 2016

UniqMartin commented Jun 30, 2016

MikeMcQuaid commented Jul 3, 2016

UniqMartin Jul 4, 2016

MikeMcQuaid Jul 4, 2016

MikeMcQuaid Jul 4, 2016

UniqMartin Jul 4, 2016

MikeMcQuaid Jul 4, 2016

MikeMcQuaid Jul 4, 2016

UniqMartin Jul 4, 2016

MikeMcQuaid Jul 4, 2016

MikeMcQuaid commented Jul 4, 2016

UniqMartin Jul 5, 2016

MikeMcQuaid Jul 5, 2016

UniqMartin commented Jul 5, 2016

MikeMcQuaid commented Jul 8, 2016

UniqMartin commented Jul 8, 2016

tap: run readall when tapping. #396

tap: run readall when tapping. #396

Conversation

MikeMcQuaid commented Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jun 30, 2016

UniqMartin commented Jun 30, 2016

MikeMcQuaid commented Jul 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jul 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UniqMartin commented Jul 5, 2016

MikeMcQuaid commented Jul 8, 2016

UniqMartin commented Jul 8, 2016

MikeMcQuaid commented Jun 23, 2016 •

edited

Loading