Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Composer lock #79

Closed

Conversation

stefanotorresi
Copy link
Contributor

resubmit of #63 targeting develop branch.

@weierophinney it worked like a charm :) the DEPS=lowest env does exactly what was being done with the package env vars, only now it looks simpler because it comes down to how the deps are defined in composer.json, rather than overriding each one of them in .travis.yml.
The PHPUnit requirement needed to be bumped up to ^4.5 because one of the tests was using a Prophecy method, which has been introduced with that version.

Let me know if you're satisfied and I'll proceed applying the same changes to all the components.

- if [[ $DEPS == 'latest' ]]; then travis_retry composer update $COMPOSER_ARGS ; fi
- if [[ $DEPS == 'lowest' ]]; then travis_retry composer update --prefer-lowest --prefer-stable $COMPOSER_ARGS ; fi
- if [[ $TEST_COVERAGE == 'true' ]]; then travis_retry composer require --dev $COMPOSER_ARGS satooshi/php-coveralls ; fi
- travis_retry composer install $COMPOSER_ARGS
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by the fact that we have this after the update commands. While I can see that it runs fine on Travis, it's confusing.

What's the rationale for doing an install after an update? Shouldn't it be the other way around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is intended as a "catch-all" scenario: the updates are only executed in special cases, and executing an install right after an update is basically a no-op. Since every line of the install block represents a different build environment, I preferred to not enclose this one in a conditional block to make it stand out as the lowest common denominator.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stefanotorresi then maybe add --lock to composer update to just update lock file if install is called in all scenarious?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@svycka good call! I forgot about that option!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@svycka apparently --lock doesn't play well with --prefer-lowest, so I'm gonna have to revert that.

@weierophinney
Copy link
Member

@stefanotorresi It looked like you were still working on this when I was prepping the 2.7 release, so I didn't get it in.

That said, I'm trying the approach with zend-test right now, and it's working brilliantly, so I'm definitely in favor! My only concern is the proliferation of test jobs; since Travis only allows 5 in parallel at any given time, this means the tests require at least two passes by Travis before completion. However, it ensures that code that purports to support multiple versions truly supports them, so it's a reasonable tradeoff in my opinion.

fix travis lowest deps build and show installed packages

fix travis coverage upload and lowest deps env

fix travis global env directive

fix travis lowest deps build

tidy .travis.yml up
@stefanotorresi
Copy link
Contributor Author

@weierophinney rebased against latest develop to include the docs deploy steps. I've made some slight changes to those too, so please make sure they still work. :)
Also see zendframework/zf-mkdoc-theme#6 which is related.

@stefanotorresi
Copy link
Contributor Author

on a side note, if we really wanted to reduce the jobs, maybe testing with lowest and latest deps on both 5.5 and 5.6 is not that useful, so we could get rid of 2 jobs, but I'm not sure it would do any good, because there would still be more than 5 jobs.

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

Successfully merging this pull request may close these issues.

3 participants