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

Make vcpkg.targets more defensive w.r.t. the naming of build configurations #4454

Closed
FrankHeimes opened this issue Oct 12, 2018 · 4 comments
Closed
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed

Comments

@FrankHeimes
Copy link
Contributor

I'm using Visual Studio 2017 with the vcpkg integration to build my projects.
The vcpkg/scripts/buildsystems/msbuild/vcpkg.targets file is of version #3504, 8. Aug. 2018.

Besides the well known build configurations Debug and Release, I also use other names that prefix or suffix additional words, like Test Debug or Debug CLR. In my project settings, I examine the configuraion name to automatically adjust certain project settings.

The problem with vcpkg is that the condition $(VcpkgConfiguration.StartsWith('Debug')) does match some cases, but not all. Also, creating the property $(VcpkgNormalizedConfiguration) as string and examining it later just to add a sub directory is more verbose than necessary.

I'd like to propose the following:

  • Create a property $(VcpkgConfigSubdir) that is 'debug\' if the configuration contains 'Debug' (case-insensitive) and is empty in all other cases.
  • Remove lot's of redundant code by inserting $(VcpkgConfigSubdir) into the strings.
  • Remove the error message "Vcpkg is unable to link because we cannot decide between Release and Debug libraries." because $(VcpkgConfigSubdir) will always be defined and the test cannot fail anymore.

I have prepared a modified version of vcpkg.targets that works very well that I'd like to submit.

FrankHeimes added a commit to FrankHeimes/vcpkg that referenced this issue Oct 12, 2018
@Neumann-A
Copy link
Contributor

As I was doing work on #4361 I also thought of changing this behavior but decided against it due to names like (Release_with_debug_symbols). Your current changes will link the debug libraries in this case which is wrong. The other question is if linking the release libraries in all other non debug named configurations is the right thing to do? (e.g. in cases like: Deb_Config) Having a hard error in these cases could be better than silently linking the release libraries.

In cases where the user defines configurations he should set $(VcpkgNormalizedConfiguration) to the appropriate vcpkg configuration via a project *.props file or via the Visual Studio GUI (see #4361)

FrankHeimes added a commit to FrankHeimes/vcpkg that referenced this issue Oct 15, 2018
@FrankHeimes
Copy link
Contributor Author

... but decided against it due to names like (Release_with_debug_symbols).
... non debug named configurations ... (e.g. in cases like: Deb_Config)
In cases where the user defines configurations he should set $(VcpkgNormalizedConfiguration) ...

Good point. Having thought about this for a while, I think $(VcpkgNormalizedConfiguration) does have a right to exist. But it should be used consistently, i.e. it should replace all occurences of VcpkgConfiguration.StartsWith('...')

I'd still suggest using $(VcpkgConfigSubdir) as proposed to reduce the redundant code.
I updated my pull request appropriately.

FrankHeimes added a commit to FrankHeimes/vcpkg that referenced this issue Oct 30, 2018
These tool agnostic properties allow to configure ClCompile and ResourceCompile without repeating the code.
This change includes my changes from microsoft#4454.
@zufuliu
Copy link

zufuliu commented Dec 28, 2018

I think at least StartsWith() can be replaced with Contains(). e.g:

VcpkgConfiguration.Contains('Debug')
VcpkgConfiguration.Contains('Release')

or

VcpkgConfiguration.ToLowerInvariant().Contains('debug')
VcpkgConfiguration.ToLowerInvariant().Contains('release')

@PhoebeHui PhoebeHui added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Jan 25, 2019
@JackBoosY
Copy link
Contributor

This issue hasn’t been updated in a year; if it is still an issue, please reopen this issue.

strega-nil added a commit that referenced this issue Jul 5, 2020
Use IncludePath and LibraryPath propertiesThese tool agnostic properties allow to configure ClCompile and ResourceCompile without repeating the code.
This change includes my changes from #4454.

Co-authored-by: Nicole Mazzuca <mazzucan@outlook.com>
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 <mazzucan@outlook.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Development

No branches or pull requests

5 participants