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

Add possibility to set vcpkg ref #5837

Merged
merged 8 commits into from
Apr 2, 2023

Conversation

matthiakl
Copy link
Member

@matthiakl matthiakl commented Mar 31, 2023

Type of change
New feature

Issue(s) closed
Fixes #5836

New behavior
Added a file vcpkg_ref to manually select a vcpkg version to use. ATM it is set to the commit of the last successful binary cache, so it should at least allow building pull request which points to a working archive.

Additional context
Once the packages are fixed, vcpkg_ref can be changed to an empty file to re-enable the standard runner version.

@matthiakl matthiakl added the building & packaging Building, packaging, continuous integration, appdata, cmake label Mar 31, 2023
@matthiakl matthiakl self-assigned this Mar 31, 2023
@matthiakl matthiakl marked this pull request as ready for review March 31, 2023 22:38
@tothxa
Copy link
Member

tothxa commented Apr 1, 2023

Looks cool. Too bad I don't understand how this caching thing works, so I don't think I should be the one to approve it.

Do I get it right that this could be quite useful in the long run too, because with this in place we would be able to pin vcpkg versions, so not only would we be more resistant to random breakages, but also we could spare a lot of rebuilding the cache?

One possible improvement: Could the pinned vcpkg commit be stored in a repo variable instead of committing it into the repo on every update?

@matthiakl
Copy link
Member Author

Could the pinned vcpkg commit be stored in a repo variable instead of committing it into the repo on every update?

Thanks for pointing this out. I was thinking about a way to store the ref outside the source, but didn't know about repo variables. I changed it now, however I do not have the rights in this repo to create one.
If you have, could you create one with the name VCPKG_REF and set it to pull/30546/merge (otherwise @Noordfrees might help?) Then relaunch the workflow.

@matthiakl
Copy link
Member Author

Do I get it right that this could be quite useful in the long run too, because with this in place we would be able to pin vcpkg versions, so not only would we be more resistant to random breakages, but also we could spare a lot of rebuilding the cache?

Yes, we can pin it to a working version to keep reusing existing caches until we update the ref.

Too bad I don't understand how this caching thing works

Let me try to explain the concept:

  • vcpkg builds all packages and dependencies from source and stores these packages in VCPKG_DEFAULT_BINARY_CACHE
  • this cache is then stored outside the workflow, so that the next run of vcpkg sees that all packages are already built and just installs them
  • to track which cache is valid for a run we generate a key which consists of the following parts:
    • Hash of install-dependencies.sh to detect changes in the dependencies
    • Revision of vcpkg to detect changes in package versions
    • The architecture (x86/x64)
  • if a cache key already exists on master, pull request runs can find it

@tothxa
Copy link
Member

tothxa commented Apr 1, 2023

Could the pinned vcpkg commit be stored in a repo variable instead of committing it into the repo on every update?

Thanks for pointing this out. I was thinking about a way to store the ref outside the source, but didn't know about repo variables. I changed it now, however I do not have the rights in this repo to create one. If you have, could you create one with the name VCPKG_REF and set it to pull/30546/merge (otherwise @Noordfrees might help?) Then relaunch the workflow.

I don't have the right to access repo vars either.

Thanks for the explanation. I've also read about caching in the workflow docs.

@Noordfrees
Copy link
Member

👍

If you have, could you create one with the name VCPKG_REF and set it to pull/30546/merge (otherwise @Noordfrees might help?)

done

@Noordfrees
Copy link
Member

Doesn't seem to work :(
Quote from the settings page:

Anyone with collaborator access to this repository can use these secrets and variables for actions. They are not passed to workflows that are triggered by a pull request from a fork.

@matthiakl
Copy link
Member Author

Doesn't seem to work :(

That's a shame. It also doesn't seem to take the variables from the respective fork, as I also added it to mine...
I can propose two solutions:

  1. The easy way: Revert that commit and have it stored in a file inside the repo
  2. The complex way: Store the version from the repo variable inside a cache itself (when running on master), then pull request builds can read it from there.

@tothxa
Copy link
Member

tothxa commented Apr 1, 2023

1. The easy way: Revert that commit and have it stored in a file inside the repo

I vote for this. I didn't expect that variables would be protected so hard.

There's another thing I don't understand though. The vcpkg commit IDs in the cache IDs (6f7b416c7 for the good one, b81bc3a83 for the failed one) are different from any commit IDs I see in the vcpkg repo. The PR was 69efe9c, and I can't see any b81bc3a in the history of their master either. Can this be because of the shallow checkout? But then there wouldn't have been cache hits in the builds.

2. The complex way: Store the version from the repo variable inside a cache itself (when running on master), then pull request builds can read it from there.

Do you mean creating a new cache that only stores the vcpkg commit ID?

@matthiakl
Copy link
Member Author

microsoft/vcpkg@b81bc3a83 is the current version used in the runner images (also see Windows2022-Readme.md)
6f7b416c7 is the hash of the pull request which is created by Github to simulate the merge. Instead of pull/30546/merge I could use pull/30546/head to point to the latest commit on the pull request branch.

Do you mean creating a new cache that only stores the vcpkg commit ID?

Yes, to transfer the information into pull request workflows. But for now we can use the file approach.

@tothxa
Copy link
Member

tothxa commented Apr 1, 2023

microsoft/vcpkg@b81bc3a83 is the current version used in the runner images (also see Windows2022-Readme.md)

Ah, I see, it's on page 3. My mistake, I thought the other commits were older.

6f7b416c7 is the hash of the pull request which is created by Github to simulate the merge. Instead of pull/30546/merge I could use pull/30546/head to point to the latest commit on the pull request branch.

Let's hope we won't often have to rely on PRs... I believe it would be best to always store the commit hash.

So for now, we may use microsoft/vcpkg@69efe9c (vcpkg master after merging microsoft/vcpkg#30546).

How often should we update it later? Stay on one as long as it works?

@matthiakl
Copy link
Member Author

Ok, it should run through now. The workflow already picked up the correct version

HEAD is now at 69efe9cc2 update runtime version (#30546)

How often should we update it later? Stay on one as long as it works?

We can think about creating a workflow which runs once a week on the latest commit. If it runs through it could automatically create a PR to update the version.

@Noordfrees
Copy link
Member

How often should we update it later? Stay on one as long as it works?

We can think about creating a workflow which runs once a week on the latest commit.

👍

If it runs through it could automatically create a PR to update the version.

Why not just commit it directly?

@matthiakl
Copy link
Member Author

Why not just commit it directly?

That would be too easy 😆. Of course this is also possible.

@tothxa
Copy link
Member

tothxa commented Apr 1, 2023

We can think about creating a workflow which runs once a week on the latest commit.

Well, if you want to update so often and automatically, would it be possible to try the latest deployed one?

But do we really want commits changing a minor config file almost every week? (OK, I know, I'm the one on Debian oldstable… ;)

Copy link
Member

@tothxa tothxa left a comment

Choose a reason for hiding this comment

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

It's through all cache uses now without problems.

Thank you for fixing it.

@Noordfrees Can we merge it as it is to be able to get all PRs green? I guess the auto-update can go in a separate PR. That will be harder to validate.

@tothxa
Copy link
Member

tothxa commented Apr 1, 2023

We can think about creating a workflow which runs once a week on the latest commit.
If it runs through it could automatically create a PR to update the version.

Why not just commit it directly?

And otherwise, when it fails, open a bug, so that we don't miss when manual action may be needed.

@matthiakl
Copy link
Member Author

After merging to master, I suggest to wait for the "Build vcpkg packages" job to finish, before merging into other branches, so they can then use the new cache.

@Noordfrees Noordfrees merged commit c1264d1 into widelands:master Apr 2, 2023
35 checks passed
@matthiakl matthiakl deleted the 5836-vcpkg_ref branch April 14, 2023 08:56
@Noordfrees Noordfrees added this to the v1.2 milestone Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building & packaging Building, packaging, continuous integration, appdata, cmake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yet another msvc dependency problem
3 participants