Skip to content
This repository has been archived by the owner on Nov 30, 2023. It is now read-only.

Adding vcpkg to cpp image definitions #1310

Merged
merged 9 commits into from
Feb 19, 2022

Conversation

dnastrain
Copy link
Contributor

#781

Description


This PR adds:

  • Updated base.Dockerfile to add support for Vcpkg c++ package manager
  • Updated Dockerfile to support conditional CMake re-install from an official release
  • Updated tests to verify Vcpkg is present and configured, when supported
  • Updated README.md documentation

PR Checklist


Use the check-list below to ensure your branch is ready for PR. If the item is not applicable, leave it blank.

  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code follows the code style of this project.
  • I ran the lint checks which produced no new errors nor warnings for my changes.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.

Does this introduce a breaking change?


  • Yes
  • No

If this introduces a breaking change, please describe the impact and migration path for existing applications below.

Testing


  • test-project/test.sh updated to ensure Vcpkg is configured and can run vcpkg --version
  • rebuilt the distros supported according to definition-manifest.json
  • ensured scripts invoked by the 2 Dockerfiles worked on supported distros.

Other information or known dependencies


NOTE: Due to vcpkg constraints running on arm64, vcpkg is not installed on arm64 stretch and bionic distros

@dnastrain dnastrain requested a review from Chuxel February 16, 2022 18:14
@dnastrain
Copy link
Contributor Author

hi @Chuxel , this is ready for review. (cc:@bderusha)

Copy link
Member

@Chuxel Chuxel left a comment

Choose a reason for hiding this comment

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

A few questions/comments.

containers/cpp/.devcontainer/Dockerfile Outdated Show resolved Hide resolved
containers/cpp/.devcontainer/Dockerfile Outdated Show resolved Hide resolved
containers/cpp/.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
containers/cpp/.devcontainer/base.Dockerfile Outdated Show resolved Hide resolved
containers/cpp/README.md Outdated Show resolved Hide resolved
@bderusha
Copy link
Contributor

@Chuxel Changes made based on your feedback. Ready for another pass when you get a chance.

@bderusha
Copy link
Contributor

Also, since you asked about it during the design review we wanted to just confirm for you that the base image size increases from 1.45GB to 1.50GB with this change.

@bderusha bderusha requested a review from Chuxel February 17, 2022 19:51
@bderusha
Copy link
Contributor

@Chuxel do we also need to update the definition-manifest at all given the new package dependencies the install-vcpkg script adds on some base images (all but stretch and bionic)? I don't know enough about how that file gets used to determine on my own. Let me know what you think :)

Copy link
Member

@Chuxel Chuxel left a comment

Choose a reason for hiding this comment

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

@Chuxel do we also need to update the definition-manifest at all given the new package dependencies the install-vcpkg script adds on some base images (all but stretch and bionic)? I don't know enough about how that file gets used to determine on my own. Let me know what you think :)

@bderusha Yeah that would be a good idea for anything that ends up in the image. Packages installed from apt from the main distro are covered by registrying the distro (which is already done). 3rd party dependencies, however, should be added. cmake we can just use the version that is installed... worst case we register one we're already covered by.

Otherwise LGTM!!

@bderusha
Copy link
Contributor

@Chuxel Updated the definition-manifest.

@dnastrain
Copy link
Contributor Author

@Chuxel Updated the definition-manifest.

Thx @bderusha ! We think this is ready to merge @Chuxel -- please let us know when you have the cycles.

@Chuxel
Copy link
Member

Chuxel commented Feb 19, 2022

LGTM! Merging!

@Chuxel Chuxel merged commit 54ca3a2 into microsoft:main Feb 19, 2022
@dnastrain dnastrain deleted the dnastrain/cpp-vcpkg branch March 7, 2022 15:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants