-
Notifications
You must be signed in to change notification settings - Fork 44
vcpkg: Adds initial overview #1030
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
vcpkg: Adds initial overview #1030
Conversation
📝 WalkthroughWalkthroughAdds a new design document Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
eskultety
left a comment
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 have to say, compared to Conan, vcpkg definitely gives me far less shivers.
| </details> | ||
|
|
||
| While it contains enough data to populate a SBOM it has to be parsed which is | ||
| inherently error-prone. There does not seem to be any way to get around this. |
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.
Why do we need to rely on stdout for SBOM populating? If there's some registry data available locally, don't we get the cmake portfile recipe with every depedency? That would be a more reliable source for SBOM contents IMO.
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 main concerns are transient dependencies (admittedly they seem to be somewhat rare), and any potential deviation which the tool makes from a list of versions and cares to report. Building a list of transitive dependencies is not that big of a task, but making sure that what was downloaded matches the spec is more complicated, that is why I reluctantly recommend parsing logs (while fully understanding how brittle this is).
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.
that is why I reluctantly recommend parsing logs
This is too error prone and unstable to back our "accurate SBOM" claim I'm afraid. I think we need to consider a different approach.
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.
Just to capture what I discussed over a private channel on this topic - could we maybe leverage CMake to create our own little cmake script that would import a given portfile.cmake and see if there's a way to access the signature (e.g. https://learn.microsoft.com/en-us/vcpkg/maintainers/functions/vcpkg_from_github) attributes/variables in CMake's context and dump them in a JSON/YAML format that would allow us to easily consume and process the crucial set of data?
docs/design/vcpkg.md
Outdated
| is not immediately clear how big this problem is, if it is a big problem then | ||
| sources will have to be injected into a new $VCPKG_ROOT on a build system. |
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'm unclear on this bit, can you elaborate? Do you mean similarly to cargo and some toolchain pinning, but in this case incompatible cache structuring?
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.
No. By default the sources end up in $VCPKG_ROOT which also contains port definitions, versions database and cmake scripts necessary for vcpkg to operate. If, for any reason, someone would use a clean version of vcpkg from, say, our image then we would at the very least need to link buildtrees within it to buildtrees generated during fetch. If, however, one could reuse effectively entire vcpkg root directory from the fetch phase then this is not a problem.
docs/design/vcpkg.md
Outdated
| Following the precedent set in other package managers vcpkg should be used as | ||
| an external tool. It will need to be downloaded and bootstrapped first, then | ||
| caches locations will need to be set, after which package's dependencies | ||
| could be resolved. |
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.
So, since vcpkg is OSS (kudos Microsoft!), can we have a look at their implementation for inspiration, see what it is they do with vcpkg install --only-download and assess if reimplementing the fetching logic is totally impractical from our POV?
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.
We would need to not only re-implement fetch logic, but also provide correct repository structure for a vcpkg instance down the road. As always -- this is possible, but not very practical since we would need to maintain it. And since vcpkg relies heavily on cmake wouldn't that also mean that we would need to re-implement cmake functionality?
795f290 to
2d1b789
Compare
2d1b789 to
bbaf7b5
Compare
9b9b6f3 to
6d5085c
Compare
6d5085c to
eaf20b0
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/design/vcpkg.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Konflux kflux-prd-rh03 / on-pull-request
- GitHub Check: Build container image and run integration tests on it
taylormadore
left a comment
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.
LGTM. The investigation was thorough and the design appears comprehensive.
This commit adds initial overview of vcpkg (C/C++ package manager) and briefly discusses how its support could be implemented. vcpkg: https://learn.microsoft.com/en-us/vcpkg/ Signed-off-by: Alexey Ovchinnikov <aovchinn@redhat.com>
eaf20b0 to
2b39546
Compare
eskultety
left a comment
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.
A solid starting point.
This commit adds initial overview of vcpkg (C/C++ package manager) and briefly discusses how its support could be implemented.
vcpkg: https://learn.microsoft.com/en-us/vcpkg/
A PoC for cmake scripts processing on Hermeto's side: ec38dff
Maintainers will complete the following section
Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:
/ok-to-test(as is the standard for Pipelines as Code)