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

Added PHP 5.6 and HHVM to travis.yml #139

Merged
merged 1 commit into from Jul 14, 2014
Merged

Added PHP 5.6 and HHVM to travis.yml #139

merged 1 commit into from Jul 14, 2014

Conversation

Nyholm
Copy link
Contributor

@Nyholm Nyholm commented Jul 14, 2014

I fixed the build matrix. We want to reduce the number of redundant runs

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) when pulling cf4ccc6 on Nyholm:patch-1 into d1aa384 on craue:master.

@craue
Copy link
Owner

craue commented Jul 14, 2014

They are not really redundant but I'm fine with reducing them, while I'd still keep PHP 5.3 with symfony 2.1 to cover the minimum requirements.

You've chosen symfony 2.3 to run against all PHP versions because it's a LTS release?

Could you as well avoid calling php vendor/bin/coveralls -v in case of HHVM as it seems to not run well with code coverage analysis? I guess
sh -c 'if [ "${TRAVIS_PHP_VERSION}" != "hhvm" ]; then php vendor/bin/coveralls -v; fi;' should do.

@Nyholm
Copy link
Contributor Author

Nyholm commented Jul 14, 2014

Thank you for quick reply. Yes, it is because it is LTS.

I've updated the PR now.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7f23a1b on Nyholm:patch-1 into d1aa384 on craue:master.

@craue
Copy link
Owner

craue commented Jul 14, 2014

I just noticed that you removed quotes from some shell script variable values, but not all. Could you consistently quote them all again and squash the commits?

- SYMFONY_VERSION="2.4.*" SENSIO_FRAMEWORK_EXTRA_BUNDLE_VERSION="3.*"
- SYMFONY_VERSION="2.5.*" SENSIO_FRAMEWORK_EXTRA_BUNDLE_VERSION="3.*"
- SYMFONY_VERSION="dev-master" MIN_STABILITY="dev"
- SYMFONY_VERSION=2.3.*
Copy link
Owner

Choose a reason for hiding this comment

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

I should've annotated specific lines initially. 😏 So please quote 2.3.* here and all other SYMFONY_VERSION values as well.

@craue
Copy link
Owner

craue commented Jul 14, 2014

Can you squash all commits into just one?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a07a452 on Nyholm:patch-1 into d1aa384 on craue:master.

I fixed the build matrix. We want to reduce the number of redundant runs
@Nyholm
Copy link
Contributor Author

Nyholm commented Jul 14, 2014

I just noticed that you removed quotes from some shell script variable values, but not all. Could you consistently quote them all again and squash the commits?

Sorry, I've miss-read you completely. (last minute fix before I left the office..)

I've updated the PR and squashed the commits.

Why do you want me to quote the SYMFONY_VERSION?

@craue
Copy link
Owner

craue commented Jul 14, 2014

Not SYMFONY_VERSION itself, but the values, just like you did. It's perfect now, thank you.

@Nyholm
Copy link
Contributor Author

Nyholm commented Jul 14, 2014

Not SYMFONY_VERSION itself, but the values, just like you did. It's perfect now, thank you.

Thank you for your patience with me =)

But why do you want the values quoted?

matrix:
allow_failures:
- php: hhvm
- env: SYMFONY_VERSION="dev-master" MIN_STABILITY="dev"
Copy link
Owner

Choose a reason for hiding this comment

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

Argh, one more typo here. 😆 Leading dash has to be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? I want it to be allowed to fail on HHVM or sf dev-master. If I remove the dash it will only be allowed to fail when using HHVM and sf dev-master.

See the build matrix on Travis. This is the intended result.

Copy link
Owner

Choose a reason for hiding this comment

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

This wasn't the case in the previous Travis runs, so I thought it's a typo. Why would you allow failures with master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can not rely on that symfony master branch is stable all the time. What if they introduce a bug that makes our tests fail?

Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather take this as an indicator that the bundle might be broken by a change in the next Symfony release. But you're right, one will still notice this in the build details.

@craue
Copy link
Owner

craue commented Jul 14, 2014

But why do you want the values quoted?

These are strings and could contain spaces, so they should be quoted.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d367c98 on Nyholm:patch-1 into d1aa384 on craue:master.

@Nyholm
Copy link
Contributor Author

Nyholm commented Jul 14, 2014

But why do you want the values quoted?

These are strings and could contain spaces, so they should be quoted.

Okey. Thanks.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d367c98 on Nyholm:patch-1 into d1aa384 on craue:master.

craue added a commit that referenced this pull request Jul 14, 2014
Added PHP 5.6 and HHVM to travis.yml
@craue craue merged commit bbd6c80 into craue:master Jul 14, 2014
@craue
Copy link
Owner

craue commented Jul 14, 2014

Thank you.

@Nyholm
Copy link
Contributor Author

Nyholm commented Jul 14, 2014

Thank you

@Nyholm Nyholm deleted the patch-1 branch July 14, 2014 18:07
@craue craue added this to the 3.0.0 milestone Jul 30, 2014
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