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

brew tests: filter out vendor/ #2203

Closed
wants to merge 2 commits into from

Conversation

mistydemeo
Copy link
Member

  • [x ] 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 successfully run brew tests with your changes locally?

Not sure why this doesn't consistently repro for me, but I noticed that brew tests was failing for me because the test glob was picking up test files from the bundled gems installed in test/vendor/bundle.

@reitermarkus
Copy link
Member

reitermarkus commented Feb 27, 2017

Remove test/vendor/bundle and test/.bundle. The bundled Gems have been in vendor/bundle for a few months now, but it seems that the test/.bundle/config still contains the BUNDLE_PATH for some people.

We could add test/.bundle/config to version control, to make sure it contains the same for everyone.

@mistydemeo
Copy link
Member Author

Adding it to version control or explicitly running bundle config to set the path makes sense to me. But I still think we should exclude these files, because even if the vendor bundle gets installed in the right place, this will fail if a user happens to have this old directory left around.

@MikeMcQuaid
Copy link
Member

We could add test/.bundle/config to version control, to make sure it contains the same for everyone.

Makes sense to me 👍

@MikeMcQuaid
Copy link
Member

@DomT4 Does this fix #2184 for you?

@DomT4
Copy link
Member

DomT4 commented Feb 27, 2017

@MikeMcQuaid No, same issue. To be honest I initially pinned this yesterday on an upstream problem at some level, because json have had identical issues reported to them in the past with semi-new Ruby x.Y.x releases. I noted it upstream on the long-running issue but nobody else has reported it as far as I can see, so I'm not sure what's going on entirely.

@DomT4
Copy link
Member

DomT4 commented Feb 27, 2017

If I gem install json -V outside of Bundle masking the output there's a noted failure on an element, but it's not the element I was seeing blow up tests.

linking shared-object json/ext/parser.bundle
current directory: /usr/local/var/rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/json-2.0.3/ext/json/ext/parser
make "DESTDIR=" install
/usr/local/bin/ginstall -c -m 0755 parser.bundle ./.gem.20170227-65081-2nbc27/json/ext
To see why this extension failed to compile, please check the mkmf.log which can be found here:

  /usr/local/var/rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/extensions/x86_64-darwin-16/2.4.0-static/json-2.0.3/mkmf.log

...

"clang -o conftest -I/usr/local/var/rbenv/versions/2.4.0/include/ruby-2.4.0/x86_64-darwin16 -I/usr/local/var/rbenv/versions/2.4.0/include/ruby-2.4.0/ruby/backward -I/usr/local/var/rbenv/versions/2.4.0/include/ruby-2.4.0 -I. -I/usr/local/var/rbenv/versions/2.4.0/include  -D_XOPEN_SOURCE -D_DARWIN_C_SOURCE -D_DARWIN_UNLIMITED_SELECT -D_REENTRANT    -O3 -Wno-error=shorten-64-to-32  -pipe conftest.c  -L. -L/usr/local/var/rbenv/versions/2.4.0/lib -L. -L/usr/local/var/rbenv/versions/2.4.0/lib  -fstack-protector -L/usr/local/lib     -lruby-static -framework CoreFoundation  -lpthread -lgmp -ldl -lobjc "
conftest.c:15:57: error: use of undeclared identifier 'rb_enc_raise'
int t(void) { void ((*volatile p)()); p = (void ((*)()))rb_enc_raise; return !p; }
                                                        ^
1 error generated.

@ilovezfs
Copy link
Contributor

@DomT4 does it happen if you checkout a tag? 1.1.10, 1.1.9, 1.1.8, ... wondering if this is a bisectable problem.

@DomT4
Copy link
Member

DomT4 commented Feb 27, 2017

json 1.8.6 builds without errors on the gem install at least. None of 2.0.0, 2.0.1, 2.0.2 or the latest 2.0.3 compile cleanly. On the brew side, no error thrown at 9cd5a21, 619791e, 9cce341, e59ada5, 664d0c6.

Had appeared by the time 69d1ced was cut.

@ilovezfs
Copy link
Contributor

ilovezfs commented Feb 27, 2017

OK. So you should be able to do

git checkout master
git bisect start
git checkout 664d0c6
git bisect good
git checkout 69d1ced
git bisect bad

And then tell it git bisect good or git bisect bad for each commit it asks you to test until it spits out the first bad commit.

@DomT4
Copy link
Member

DomT4 commented Feb 27, 2017

Was already working on it 😉. e40c63f is where it was introduced, merged as part of #1913.

@ilovezfs
Copy link
Contributor

🎉

@@ -57,6 +57,7 @@ def tests
ENV["SEED"] = ARGV.next if ARGV.include? "--seed"

files = Dir.glob("test/**/*_{spec,test}.rb")
.reject { |p| p =~ %r{^test/vendor} }
Copy link
Member

Choose a reason for hiding this comment

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

Make this test/vendor/bundle, since there is at least one test actually testing vendored code (plist, currently still in cask-tests).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

mistydemeo and others added 2 commits February 28, 2017 21:02
This smooths over the transition for users who have an existing
bundle config in this location due to having gems installed at the old
path.
@mistydemeo mistydemeo deleted the tests_filter_vendor branch February 28, 2017 11:43
@DomT4 DomT4 mentioned this pull request Feb 28, 2017
3 tasks
@DomT4
Copy link
Member

DomT4 commented Mar 2, 2017

Was it intentional here that the repo becomes "dirty" after brew tests is run?

cd $(brew --repo)
git fetch && git reset --hard origin/master
brew tests
brew --version
Homebrew 1.1.10-432-ge02223960-dirty

The change is:

diff --git a/Library/Homebrew/test/.bundle/config b/Library/Homebrew/test/.bundle/config
index dcf3d1c4c..e451829e9 100644
--- a/Library/Homebrew/test/.bundle/config
+++ b/Library/Homebrew/test/.bundle/config
@@ -1,3 +1,3 @@
 ---
 BUNDLE_PATH: "../vendor/bundle"
-BUNDLE_DISABLE_SHARED_GEMS: '1'
+BUNDLE_DISABLE_SHARED_GEMS: "true"

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants