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

Empty Vcpkg project property sheet, missing '-static' suffix and some clean-up #13753

Closed
FrankHeimes opened this issue Sep 26, 2020 · 2 comments
Assignees
Labels
category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`)

Comments

@FrankHeimes
Copy link
Contributor

FrankHeimes commented Sep 26, 2020

I applied two bug fixes and some improvements to the files in vcpkg/scripts/buildsystems/msbuild and submitted a PR with my changes and reference to this issue.

[Edit] Updated this description according to my additional findings and fixes.

vcpkg-general.xml

Bug Fixes

Problem: The Vcpkg project sheet does not display any values.
Cause: Empty XML groups have been removed from the project file. Visual Studio however expects that a project file contains at least an empty <PropertyGroup /> below the last import of properties (e. g.: vcpkg.props). Otherwise, all entries of the vcpkg project sheet will remain empty, but only if the project sheet vcpkg-general.xml also specifies Label="Vcpkg".
Proof: Remove lines 72-87 from scripts\testing\integrate-install\VcpkgTriplet.vcxproj and open it in Visual Studio.

We have moved almost every project property definition to shared .props and .targets files. So most of our project files contain only <ItemDefinitionGroup>s, <ItemGroup>s, and <import>s below the import statement for Microsoft.Cpp.props.

IMHO, it's a design flaw to exhibit undefined behavior if an empty and thus obsolete XML object is removed from a file.

Workaround Options:

  1. Remove Label="Vcpkg" from the property groups and Vcpkg project sheet, as I did initially. This is a breaking change for projects that have specified a property using this label (like VcpkgTriplet.vcxproj).
  2. Require all projects that import Vcpkg to add an empty <PropertyGroup /> to their project files if they encounter this problem.
  3. Advise users to remove Label="Vcpkg" from their copy of vcpkg-general.xml if they encounter this problem.

Improvements

  • Combined some rule properties into single lines to improve overview of file.
  • Moved rule properties "hidden" at the end of large description strings to other locations to improve readability without so much horizontal scrolling.
  • Added newlines between property items to increase readability.
  • Added category Conditional that contains only those properties that have HasConfigurationCondition="true". It doesn't make sense to me that some properties, like $(VcpkgEnabled), may have different values depending on the Debug/Release configuration.
  • Reordered the properties according to their categories, so that they have the same order as displayed by Visual Studio.
  • Removed the Switch and DisplayName properties from the VcpkgConfiguration EnumProperty because that led to a redundant and incorrect display "Debug (Debug)", because Debug and Release are - strictly speaking - not switches (like '/O3').
  • Augmented the description of VcpkgTriplet on how it depends on the value of VcpkgUseStatic.
  • Removed "Only change this if you know what you are doing!" from the description of VcpkgCurrentInstalledDir, because it is silly. You could write this remark to basically every setting.

vcpkg.props

Improvements

  • Renamed property VcpkgHasProps to VcpkgPropsImported, because that is a better fit.
  • Compacted the definitions of properties VcpkgOSTarget and VcpkgPlatformTarget by swapping the order in which their two conditions are tested.
  • Moved property definitions from vcpkg.targets here that do not depend on other properties.
  • Updated all properties to set default values only optionally without overwriting values (e. g. VcpkgManifestRoot).
  • Changed definition of VcpkgRoot to rely on the location of vcpkg.props instead of the tag file .vcpkg-root, because that suits our projects better.

vcpkg.targets

Bug Fixes

  • Property VcpkgTriplet is updated if $(VcpkgUseStatic) is true but property VcpkgCurrentInstalledDir is not updated. Introduced helper property VcpkgLinkage instead.

Improvements

  • Augmented TreatAsLocalProperty list with properties used only in this file.
  • Removed redundant property definitions already present in vcpkg.props (see above).
  • Imported vcpkg.props depending on the value of VcpkgPropsImported.
  • Imported VcpkgPageSchema only if it exists. So it can be disabled by specifying an invalid string.
  • Added comment to overwrite VcpkgPageSchema to deliberately disable the project property sheet for Vcpkg.
  • Used [System.IO.Path]::Combine() to combine paths because it is robust against missing trailing paths. This is relevant if properties are specified on the command line and MSBuild refuses to alter them.
  • Reordered the Target properties to improve readability.
FrankHeimes added a commit to FrankHeimes/vcpkg that referenced this issue Sep 26, 2020
@PhoebeHui PhoebeHui self-assigned this Sep 27, 2020
@PhoebeHui PhoebeHui added the category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) label Sep 27, 2020
@FrankHeimes
Copy link
Contributor Author

Update: Fixed warning and error that caused vcpkg end-to-end tests to fail

  • Added VcpkgHasProps back as VcpkgPropsImported to avoid warning MSB4011. I think that name is a better match.
  • Reverted the change to VcpkgRoot that may have caused a mal-formed include path.

strega-nil added a commit that referenced this issue Nov 19, 2020
…13755)

* Use IncludePath and LibraryPath properties

These tool agnostic properties allow to configure ClCompile and ResourceCompile without repeating the code.
This change includes my changes from #4454.

* Applied changes as described in #13753

* Fixed warning and error in vcpkg end-to-end tests

* Fixed incorrect warning "we found a manifest file in \."

* Fixed still failing integration test. See discussion in #13753.

* Code Review Correction

Removed stray double quote reported by @strega-nil

* change display name

Co-authored-by: Nicole Mazzuca <[email protected]>
Co-authored-by: Billy Robert O'Neal III <[email protected]>
@JackBoosY
Copy link
Contributor

JackBoosY commented Nov 20, 2020

Fixed. Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`)
Projects
None yet
Development

No branches or pull requests

3 participants