-
Notifications
You must be signed in to change notification settings - Fork 23
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
GitHub Actions: Install composer dependencies #41
Conversation
.github/workflows/tests.yml
Outdated
uses: actions/cache@v2 | ||
with: | ||
path: vendor | ||
key: ${{ runner.os }}-php-${{ hashFiles('**/composer.lock') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as the composer.lock is not tracked in git, not sure if should use composer.json here instead?
as the docs don't mention that:
but I've seen some examples saying to use package.json.
probably running this action several times gives the answer....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's some discussion on the not committed lock file:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pointing to composer.json
would make sense if versions specified in composer.json were fixed. (no version ranges)
E.g. in case new version of zf1s/phpunit
is released, actions/cache
may still take previous version out of its cache based on the hash of composer.json
.
Does it make sense to generate and commit lockfile here? Then, when a new version of zf1s/phpunit
is released, another commit will have to be pushed to zf1s/zf1
to update the version in the lockfile.
Either way, if dependencies are locked to exact versions in composer.json
or composer.lock
is added, another commit will be necessary.
Is there any other option? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original point was that if composer.json has changes, and it's hash not used for actions/cache, the vendor would not be updated.
If you use composer.json without locking, it's expected to have a cache with a variable contents.
I'm not sure about committing a lock file here. I'll give it a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lock file for composer.json
There's a common misconception whether composer.lock
needs to be committed to the repository or not.
It can be summarized with two opposite scenarios:
- projects: keep lock in projects where it really matters (production is deployed from it):
- libraries: do not keep lock for libraries
As the current composer.json contains only one dependency, let's make it strict (no version ranges) and do not commit lock. as the lock would be just tracking phpunit dependencies, that's silly in my opinion.
And we can rethink this in the future at any time.
As for any other option: don't use actions/cache
and such problem won't exist :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please look at the changes, it's already implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw them, only confirmed that's a good idea :) Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw how much time is saved in a run when composer cache is hit?
To answer my question: I checked https://github.com/glensc/php-zf1s/actions/runs/466000519 and I could see composer install takes 5-10 seconds, when cache is not hit, so in our (simple) case with only 2 dependencies maybe it's not worth to add additional complexity here 🤔
Also something is wrong with hashFiles
: the hash is missing from the cache key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed all cache related changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Altho I personally would stick using the cache, then CI would not have to make requests outside its infrastructure. But then again I would like this merged, so all cache logic is removed now.
b0785f8
to
aaf0b7e
Compare
.github/workflows/tests.yml
Outdated
# we hash only root composer.json | ||
# https://docs.github.com/en/free-pro-team@latest/actions/reference/context-and-expression-syntax-for-github-actions#hashfiles | ||
# https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables | ||
key: ${{ runner.os }}-php-${{ hashFiles('/composer.json') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now I'm thinking perhaps the different php versions should not share composer cache:
otherwise, they end up not being ran at all, as cache created from other run, which is possibly different PHP version... which could end up shadowing some bugs.
but then again vendor should be identical in regardless what was the PHP version when composer install was invoked. there's one gotcha: composer v2 generates different autoload if platform-check is enabled (and it is by default).
@falkenhawk wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the change to be able to proceed with merging. can easily remove the commit if it's unwanted.
Extension dependencies taken from composer.json
48e2171
to
c3487d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you once again for your contributions.
Extracted composer install from #34