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

Build improvements #858

Merged
merged 22 commits into from
May 22, 2015
Merged

Build improvements #858

merged 22 commits into from
May 22, 2015

Conversation

ruflin
Copy link
Owner

@ruflin ruflin commented May 21, 2015

Improve the Travis builds

@@ -29,9 +29,9 @@ before_script:
- if [ $ES_COMPOSER_NODEV == "no" ] && ! $(phpenv version-name | grep -q '5.3'); then sudo composer require "guzzlehttp/guzzle" "4.2.*" --dev --no-ansi --no-progress --no-interaction; fi

script:
- phpunit -c test/ --coverage-clover build/coverage/clover-unit.xml --group unit
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, if unit tests will be placed before shutdown, they potentially can hit ES server, that obviously not thing unit test is supposed to do.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I see, haven’t thought of that. The best thing would be to only install and boot up elasticsearch after the unit tests were run. Perhaps we can set environment variables for each build and for the unit tests set the ES_HOST to null.

On 21 May 2015, at 14:31, Igor Denisenko notifications@github.com wrote:

In .travis.yml #858 (comment):

@@ -29,9 +29,9 @@ before_script:

  • if [ $ES_COMPOSER_NODEV == "no" ] && ! $(phpenv version-name | grep -q '5.3'); then sudo composer require "guzzlehttp/guzzle" "4.2.*" --dev --no-ansi --no-progress --no-interaction; fi

    script:

      • phpunit -c test/ --coverage-clover build/coverage/clover-unit.xml --group unit
        Just FYI, if unit tests will be placed before shutdown, they potentially can hit ES server, that obviously not thing unit test is supposed to do.


Reply to this email directly or view it on GitHub https://github.com/ruflin/Elastica/pull/858/files#r30796730.

@ruflin
Copy link
Owner Author

ruflin commented May 22, 2015

I finally solved this issue by just ignoring dev for PHP 5.3. The part I don't get is:

composer remove "guzzlehttp/guzzel"

This works as expected and removes the package after I reply yes.

If I use the -n option and set "discard-changes" config to true or false (I tried both ...). Nothing happens. Why is the correct parameter that it automatically answers yes during the removal without my interaction?

@stof I'm sure there is a flag or config I just missed.

ruflin added a commit that referenced this pull request May 22, 2015
@ruflin ruflin merged commit 18aaa81 into master May 22, 2015
@ruflin ruflin deleted the build-improvements branch May 22, 2015 08:31
@@ -17,21 +17,16 @@ matrix:

env:
matrix:
Copy link
Contributor

Choose a reason for hiding this comment

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

you should remove this key entirely as it is empty now

@ruflin
Copy link
Owner Author

ruflin commented May 22, 2015

@stof Thx for the review. Will fix it. Any idea about the "remove" issue I mentioned above?

@stof
Copy link
Contributor

stof commented May 22, 2015

I don't know. But the first question would be to know why there are changes in the package (if it asks to discard changes, it means you did some changes first, or that the package has some weird stuff triggering changes in it on install, for instance a file listed in bin without beign executable)

@ruflin
Copy link
Owner Author

ruflin commented May 22, 2015

The problem is PHP 5.3. I still want to keep the builds alive but Guzzle is not compatible with PHP 5.3. Unfortunately only skipping the tests doesn't work as it already checks the libraries in the beginning and an exception is thrown. So my idea is that instead of making special cases for all others (means installing an extra package) I want to remove Guzzle for php including the libs in the vendor directory.

@im-denisenko
Copy link
Contributor

@ruflin I would suggest to remove guzzle completely and lets ivory/http-adapter take care about alternative transports.

As option, may be even purge entire Transport namespace and do all requests through http adapter.

What do you think?

@im-denisenko
Copy link
Contributor

Though, nevermind. It's not causes so much troubles to have yet another bc.

@ruflin
Copy link
Owner Author

ruflin commented May 25, 2015

@im-denisenko In the long term I agree that we should think about that, but currently it is not a priority. I think dropping of memcache and especially Thrift will cause some uproar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants