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

Fix: vcpkg binaries were not being cached on Mac #8571

Merged
merged 1 commit into from Jan 14, 2021

Conversation

orudge
Copy link
Contributor

@orudge orudge commented Jan 14, 2021

Motivation / Problem

vcpkg was being cloned into the /tmp directory on Mac, and this was somehow breaking the caching.

Description

We now re-clone vcpkg into the standard directory. Once the Mac build agents are updated to a more recent version of vcpkg we can drop this step.

cd /tmp
git clone https://github.com/Microsoft/vcpkg
cd /usr/local/share/vcpkg
ls -A1 | xargs rm -rf
Copy link
Member

Choose a reason for hiding this comment

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

why not just go into /usr/local/share and do rm -rf vcpkg ? After that you can clone it again with vcpkg instead of .?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I think that's what I did originally (when first doing the arm64 branch) and the permissions on /usr/local/share didn't allow me to. I suppose I could do a sudo rm -rf etc then recreate it, but this way ultimately worked out a bit simpler and avoided sudo.

https://github.com/actions/virtual-environments/blob/main/images/macos/provision/core/vcpkg.sh shows the chmod in opreation.

That said, I see now the repository there just had a shallow clone, so perhaps a git fetch --unshallow is all I need to do instead...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, replaced now with a git fetch which looks to be working.


- name: Prepare vcpkg (with cache)
uses: lukka/run-vcpkg@v6
with:
vcpkgDirectory: '/tmp/vcpkg'
vcpkgDirectory: '/usr/local/share/vcpkg'
doNotUpdateVcpkg: false
Copy link
Member

Choose a reason for hiding this comment

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

Can this also be put back on true in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do need it to update to the git changeset ID we specified (the git fetch won't check out the changeset we need of course), so I think it still needs to stay as it is. It's caching fine in my test environment.

Copy link
Member

@TrueBrain TrueBrain Jan 14, 2021

Choose a reason for hiding this comment

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

So you only fetch, not checkout, fair. In that case would you mind extending the comment above to indicate this should also be set back to true once the base image is updated?
I only mention it, as this is now different from the Windows target, and that is going to confuse is in the future :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment updated.

Copy link
Member

Choose a reason for hiding this comment

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

Is the same not needed for the ci-build?
(or, what is an updated vcpkg needed for?)

Copy link
Contributor Author

@orudge orudge Jan 14, 2021

Choose a reason for hiding this comment

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

No, the updated vcpkg is only needed for arm64 builds on x86_64 (or building x86_64 on arm64), and we don't currently build arm64 on the CI.

@orudge orudge merged commit bc8f347 into OpenTTD:master Jan 14, 2021
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