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

Adding support for customized Project configurations? #1638

Closed
paercebal opened this Issue Aug 12, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@paercebal
Contributor

paercebal commented Aug 12, 2017

For now, VCPKG only supports 'Debug' and 'Release' configurations, which means other configurations do not profit from the automatic include, linking, and binaries copying to the output directory at compilation time.

I propose to add a partial support based on configuration name prefix, instead of strict configuration name.

Pre-requisite

The pre-requisite is the following: It is possible to offer different but compatible configurations binaries.

For example, if I create a using a custom configuration called "ReleaseDev" based on "Release", but disabling optimizations, my DLL "ReleaseDev" should work flawlessly with a "Release" DLL compiled with VCPKG.

AFAIK, the main breaking difference between Debug and Release is the use of the debug or release runtime DLL. Everything else should be compatible (please correct me on the flags I'm wrong).

How it currently works

Currently, the "magic" is done in vcpkg.targets: https://github.com/Microsoft/vcpkg/blob/master/scripts/buildsystems/msbuild/vcpkg.targets

For example:

      <AdditionalDependencies Condition="'$(VcpkgConfiguration)' == 'Debug' and '$(VcpkgAutoLink)' != 'false'">%(AdditionalDependencies);$(VcpkgRoot)debug\lib\*.lib</AdditionalDependencies>

The change I propose

Instead of supporting only 'Release' or 'Debug', I propose to support all configurations starting with 'Release' or 'Debug'.

This would result in something like the following change in vcpkg.targets file:

      <AdditionalDependencies Condition="$(VcpkgConfiguration.StartsWith('Debug')) and '$(VcpkgAutoLink)' != 'false'">%(AdditionalDependencies);$(VcpkgRoot)debug\lib\*.lib</AdditionalDependencies>

... the difference being $(VcpkgConfiguration.StartsWith('Debug')) replacing '$(VcpkgConfiguration)' == 'Debug'.

Question

I've successfully tested the proposed modification above on my machine.

And, IMHO, adding to the specification that all user-defined configuration starting with "Release" should be compatible with the default "Release" (same for "Debug") is a good compromise.

So, is this something worth exploring? Either with the solution above, or a variation?

Edit: Just in case the solution above is good enough, I proposed the change in the request below.

Thanks,

@martin-s

This comment has been minimized.

Show comment
Hide comment
@martin-s

martin-s Aug 15, 2017

Contributor

What about other schemes of configurations? For example, RelWithDebInfo and others cmake provides...

Contributor

martin-s commented Aug 15, 2017

What about other schemes of configurations? For example, RelWithDebInfo and others cmake provides...

@ras0219-msft

This comment has been minimized.

Show comment
Hide comment
@ras0219-msft

ras0219-msft Aug 15, 2017

Contributor

@martin-s The VcpkgConfiguration variable is initialized with your solution configuration by default[1], but you can override it inside your MSBuild.

So, you could add:

<PropertyGroup>
  <VcpkgConfiguration Condition="'$(Configuration)' == 'RelWithDebInfo'">Release</VcpkgConfiguration>
</PropertyGroup>

to your project file's globals section and we'll see and match against "Release".

[1]

<VcpkgConfiguration Condition="'$(VcpkgConfiguration)' == ''">$(Configuration)</VcpkgConfiguration>

Contributor

ras0219-msft commented Aug 15, 2017

@martin-s The VcpkgConfiguration variable is initialized with your solution configuration by default[1], but you can override it inside your MSBuild.

So, you could add:

<PropertyGroup>
  <VcpkgConfiguration Condition="'$(Configuration)' == 'RelWithDebInfo'">Release</VcpkgConfiguration>
</PropertyGroup>

to your project file's globals section and we'll see and match against "Release".

[1]

<VcpkgConfiguration Condition="'$(VcpkgConfiguration)' == ''">$(Configuration)</VcpkgConfiguration>

@paercebal

This comment has been minimized.

Show comment
Hide comment
@paercebal

paercebal Aug 15, 2017

Contributor

@ras0219-msft Is your solution what you recommend for all the cases (including the ones I describe)?

(i.e. I didn't realize this was a possible solution, even in my case. I find your solution actually better than the "hack" in my pull requests, or the hacks I tried at home - I actually went through a few hacks to produce similar results, after failing to find a way to do it cleanly).

If yes, is there a documentation on the subject?

If there isn't a documentation, I suggest we should update the FAQ:

https://github.com/Microsoft/vcpkg/blob/master/docs/about/faq.md

... with something like:

I have non-standard configurations that are compatible with standard configuration. How can I profit from VCPKG integration?

In your project file, you can add the following:

<PropertyGroup>
  <VcpkgConfiguration Condition="'$(Configuration)' == 'YOUR_CONFIGURATON_NAME_HERE'">Release</VcpkgConfiguration>
</PropertyGroup>

... to have VCPKG consider your non-standard configuration as a "Release" one (the same for "Debug")."

If everyone agrees, I can cancel this issue and the associated pull request, and update the FAQ documentation instead.

Contributor

paercebal commented Aug 15, 2017

@ras0219-msft Is your solution what you recommend for all the cases (including the ones I describe)?

(i.e. I didn't realize this was a possible solution, even in my case. I find your solution actually better than the "hack" in my pull requests, or the hacks I tried at home - I actually went through a few hacks to produce similar results, after failing to find a way to do it cleanly).

If yes, is there a documentation on the subject?

If there isn't a documentation, I suggest we should update the FAQ:

https://github.com/Microsoft/vcpkg/blob/master/docs/about/faq.md

... with something like:

I have non-standard configurations that are compatible with standard configuration. How can I profit from VCPKG integration?

In your project file, you can add the following:

<PropertyGroup>
  <VcpkgConfiguration Condition="'$(Configuration)' == 'YOUR_CONFIGURATON_NAME_HERE'">Release</VcpkgConfiguration>
</PropertyGroup>

... to have VCPKG consider your non-standard configuration as a "Release" one (the same for "Debug")."

If everyone agrees, I can cancel this issue and the associated pull request, and update the FAQ documentation instead.

@ras0219-msft

This comment has been minimized.

Show comment
Hide comment
@ras0219-msft

ras0219-msft Aug 24, 2017

Contributor

While the solution I've posted would work for your cases, I think that your proposed change will make for a better "by-default" experience. In parallel though, we should improve the FAQ and I'd be delighted if you added that section to your PR.

Contributor

ras0219-msft commented Aug 24, 2017

While the solution I've posted would work for your cases, I think that your proposed change will make for a better "by-default" experience. In parallel though, we should improve the FAQ and I'd be delighted if you added that section to your PR.

@paercebal

This comment has been minimized.

Show comment
Hide comment
@paercebal

paercebal Aug 29, 2017

Contributor

Ok.

I have two questions, and one proposal:

Questions

Do we make this case-insensitive?

Do we make the test case insensitive as suggested by @pravic ?

$(VcpkgConfiguration.StartsWith('Debug', $true))
$(VcpkgConfiguration.StartsWith('Release', $true))

What about other CMake "configurations"

For the sake of a better "by-default" experience, should the pull request also support CMake's "configurations" (for lack of a better term)? That is:

  • MinSizeRel
  • RelWithDebInfo (this is the only CMake "Release"-compatible configuration with PDBs)

(I've looked at VCXPROJ files generated by CMake, and they seem compatible with "Release")

Proposal

I propose two pull requests:

  1. one pull request to update the documentation about the manual support for other configurations (in the FAQ: https://github.com/Microsoft/vcpkg/blob/master/docs/about/faq.md). Done at #1727
  2. followed by another to extend the automatic support of other, compatible, configurations, as well as the relevant documentation update
Contributor

paercebal commented Aug 29, 2017

Ok.

I have two questions, and one proposal:

Questions

Do we make this case-insensitive?

Do we make the test case insensitive as suggested by @pravic ?

$(VcpkgConfiguration.StartsWith('Debug', $true))
$(VcpkgConfiguration.StartsWith('Release', $true))

What about other CMake "configurations"

For the sake of a better "by-default" experience, should the pull request also support CMake's "configurations" (for lack of a better term)? That is:

  • MinSizeRel
  • RelWithDebInfo (this is the only CMake "Release"-compatible configuration with PDBs)

(I've looked at VCXPROJ files generated by CMake, and they seem compatible with "Release")

Proposal

I propose two pull requests:

  1. one pull request to update the documentation about the manual support for other configurations (in the FAQ: https://github.com/Microsoft/vcpkg/blob/master/docs/about/faq.md). Done at #1727
  2. followed by another to extend the automatic support of other, compatible, configurations, as well as the relevant documentation update
@paercebal

This comment has been minimized.

Show comment
Hide comment
@paercebal

paercebal Aug 31, 2017

Contributor

The basic support (for strict "Release"/"Debug" prefix), as well as the relevant documentation was done:

Do I close this issue now? Or do I conclude by providing:

  • support of case insensitive prefixes (e.g. "releaseGizmo" will also work)
  • and/or support for default CMake configs (i.e. "MinSizeRel" and "RelWithDebInfo")

... as well as the relevant documentation update?

Contributor

paercebal commented Aug 31, 2017

The basic support (for strict "Release"/"Debug" prefix), as well as the relevant documentation was done:

Do I close this issue now? Or do I conclude by providing:

  • support of case insensitive prefixes (e.g. "releaseGizmo" will also work)
  • and/or support for default CMake configs (i.e. "MinSizeRel" and "RelWithDebInfo")

... as well as the relevant documentation update?

@bjornpiltz

This comment has been minimized.

Show comment
Hide comment
@bjornpiltz

bjornpiltz Nov 15, 2017

Contributor

👍 for adding support for default CMake configs (i.e. "MinSizeRel" and "RelWithDebInfo").

Contributor

bjornpiltz commented Nov 15, 2017

👍 for adding support for default CMake configs (i.e. "MinSizeRel" and "RelWithDebInfo").

@ras0219-msft

This comment has been minimized.

Show comment
Hide comment
@ras0219-msft

ras0219-msft Nov 16, 2017

Contributor

Yep, both of those CMake configs were added to autodetection.

I'd be happy to accept documentation updates or case-insensitivity changes, but I think this is good enough to call this issue closed.

Thanks @paercebal for working through this!

Contributor

ras0219-msft commented Nov 16, 2017

Yep, both of those CMake configs were added to autodetection.

I'd be happy to accept documentation updates or case-insensitivity changes, but I think this is good enough to call this issue closed.

Thanks @paercebal for working through this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment