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

Move our big OSS dependencies to vcpkg #15855

Merged
merged 5 commits into from
Jul 19, 2024
Merged

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Aug 21, 2023

This pull request removes the following vendored open source, in favor
of getting it from vcpkg:

  • CLI11 2.4
  • jsoncpp 1.9
  • fmt 7.1.3
  • gsl 3.1 (not vendored, but submoduled--arguably worse!)

Now that Visual Studio 2022 includes a built-in workload for vcpkg, the
onboarding process is much smoother. Terminal should only require the
vcpkg workload.

I've added some build rules that detect vcpkg via VS and via the user's
environment before falling back to a location in the source tree. The CI
pipeline will fall back to installing and bootstrapping vcpkg in
dep/vcpkg if necessary.

Some OSS has not been (and will not be) migrated:

  • wyhash: ours is included directly in til/hash
  • pcg_random: we have a stripped down copy compared to vcpkg
  • stb_rect: vcpkg only ships all of STB; ours is a stripped down copy
  • chromium numerics: vcpkg does not ship Chromium, especially not this
    tiny fraction of Chromium
  • dynamic_bitset and libpopcnt: removing in A minor ConPTY refactoring: Goodbye VtEngine Edition #17510
  • interval_tree: no vcpkg equivalent

To support the needs of the inbox Windows build, I've split up our vcpkg
manifest into dependencies for all projects and dependencies just for
Terminal. To support this, we now offer a terminal feature. The vcpkg
rules in common.build.pre.props are set up to turn it on, whereas the
build rules we eventually write for the OS will not be.

Most of the work is concentrated in common.build.pre.props.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/vcpkg-manifest branch from d10a629 to fb4979f Compare August 23, 2023 00:54
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@@ -113,4 +113,7 @@
<ForcedIncludeFiles>$(SolutionDir)\bin\$(Configuration)\inc\TilFeatureStaging.h;%(ForcedIncludeFiles)</ForcedIncludeFiles>
</ClCompile>
</ItemDefinitionGroup>

<!-- For C++ projects, bring in the vcpkg targets at the end -->
<Import Project="$(VcpkgRoot)/scripts/buildsystems/msbuild/vcpkg.targets" Condition="'$(MSBuildProjectExtension)'=='.vcxproj'" />

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error

buildsystems is not a recognized word. (unrecognized-spelling)
Since we link everything statically, we don't need to copy anything.
This saves running a powershell script for every project.
-->
<VcpkgApplocalDeps>false</VcpkgApplocalDeps>

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error

Applocal is not a recognized word. (unrecognized-spelling)
Since we link everything statically, we don't need to copy anything.
This saves running a powershell script for every project.
-->
<VcpkgApplocalDeps>false</VcpkgApplocalDeps>

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error

Applocal is not a recognized word. (unrecognized-spelling)
<CAExcludePath>$(CAExcludePath);$(VcpkgInstalledDir)</CAExcludePath>
</PropertyGroup>

<Import Project="$(VcpkgRoot)/scripts/buildsystems/msbuild/vcpkg.props" Condition="'$(MSBuildProjectExtension)'=='.vcxproj'" />

Check failure

Code scanning / check-spelling

Unrecognized Spelling Error

buildsystems is not a recognized word. (unrecognized-spelling)

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@DHowett DHowett force-pushed the dev/duhowett/vcpkg-manifest branch from bc01d0c to f43076c Compare July 18, 2024 23:02
@DHowett DHowett changed the title WIP: vcpkg Move most of our OSS dependencies to vcpkg Jul 18, 2024
@DHowett DHowett changed the title Move most of our OSS dependencies to vcpkg Move our big OSS dependencies to vcpkg Jul 18, 2024
@DHowett DHowett marked this pull request as ready for review July 18, 2024 23:10
@DHowett DHowett merged commit 3c5800f into main Jul 19, 2024
18 of 20 checks passed
@DHowett DHowett deleted the dev/duhowett/vcpkg-manifest branch July 19, 2024 18:29
@zadjii-msft
Copy link
Member

oh no we forgot to do the .editorconfig thing

DHowett added a commit that referenced this pull request Jul 22, 2024
`nuget restore` actually runs through MSBuild! However, #15855 added a
dependency from our project on a system-installed _or locally detected_
`vcpkg.targets` (or `.props`).

Our build runs `nuget restore` before finding or installing vcpkg, so
the rules in our project file would try to import vcpkg before it had
been found (or installed).

On build agents with vcpkg installed via the VS workload, this was fine:
we would import the one that came with VS and go on our merry way. On
build agents where it needs to be installed locally, it could not be
imported.

The fix in this PR is to install/bootstrap vcpkg before running nuget.

I tried to isolate the vcpkg rules to only run _in the absence of
nuget_, but that didn't work.
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.

3 participants