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.

Sign up for GitHub

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

Jump to bottom

adding vcpkg as a submodule to speed up CI #367

Closed
wants to merge 8 commits into from
Closed

adding vcpkg as a submodule to speed up CI #367

wants to merge 8 commits into from

Conversation

glhewett
Copy link
Contributor

@glhewett glhewett commented Sep 9, 2023 •
edited
Loading

This PR will do two things:

  1. Adds vcpkg as a submodule will help us to lock in the version of our dependencies requiring us to never build new dependencies. This is a step in minimizing the changes that require to recapture cache.
  2. Reworked the cache key to minimize dependencies rebuilds by using an older cache to prime the new cache. i.e. if the vcpkg.json changes, it will use recent relevant cache to start rebuilding a new cache. (Changes to one of the two vcpkg.json will cause a new cache to be created)

@glhewett glhewett self-assigned this Sep 9, 2023
@glhewett glhewett marked this pull request as ready for review September 12, 2023 16:48
@bifurcation bifurcation mentioned this pull request Oct 17, 2023
Closed
@glhewett glhewett force-pushed the glh-vcpkg-submodule branch 2 times, most recently from c9d8cfd to 6daaffc Compare October 19, 2023 05:40
bifurcation
bifurcation requested changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@bifurcation bifurcation left a comment

Choose a reason for hiding this comment

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

Overall, this looks pretty good. Just a few minor things I would like tuned up.

One general comment: It would probably speed things up to set VCPKG_BUILD_TYPE to release, which will prevent vcpkg building both debug and release versions of the dependencies.

.github/workflows/main_ci.yml Outdated Show resolved Hide resolved
.github/workflows/main_ci.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@glhewett
Copy link
Contributor Author

Overall, this looks pretty good. Just a few minor things I would like tuned up.

One general comment: It would probably speed things up to set VCPKG_BUILD_TYPE to release, which will prevent vcpkg building both debug and release versions of the dependencies.

I don't think it as simple as just setting an env/cmake variable. I tried both, and they did not work for me. I found a stack overflow that suggested creating a new triplet file, which I can look into in the future. Since the cache is working as expected and the downloads are already pretty fast, I don't think this really improve the speed of the builds.

https://stackoverflow.com/questions/52578975/getting-vcpkg-to-build-only-release-version

@glhewett
Copy link
Contributor Author

Regarding the MACOSX_DEPLOYMENT_TARGET, it looks like it is not working 100%. I am seeing logs like the following:

ld: warning: object file (../../../vcpkg_installed/x64-osx/debug/lib/libcrypto.a(libcrypto-lib-aes-x86_64.o)) was built for newer macOS version (12.7) than being linked (10.11)
ld: warning: object file (../../../vcpkg_installed/x64-osx/debug/lib/libcrypto.a(libcrypto-lib-aes_cfb.o)) was built for newer macOS version (12.7) than being linked (10.11)

I search for how I could fix that...
microsoft/vcpkg#10038

responding to bifurcation's change requests
f12fc01 
* hoisting more environmental variables
* fixing matrix to support MACOSX_DEPLOYMENT_TARGET
* using deployment target in cache key
* building on macos13
@bifurcation bifurcation mentioned this pull request Oct 24, 2023
Merged
@glhewett glhewett marked this pull request as draft October 24, 2023 19:14
@bifurcation bifurcation mentioned this pull request Oct 24, 2023
Merged
@bifurcation
Copy link
Contributor

Replaced by #395, #391, #389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bifurcation bifurcation bifurcation approved these changes

Assignees

@glhewett glhewett

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@glhewett @bifurcation