|
Notes |
|
|
(0029125)
|
|
Brad King
|
|
2012-04-11 08:09
|
|
Thanks for the patch. Here are a few comments. Please attach a new patch addressing them.
- What is the purpose of the commented-out custom command at the bottom of the patch?
- Please add to the command's documentation to explain the new interface. Include limitations like behavior on VS 6 and VS >= 10. It is not clear from the patch whether it addresses those versions.
- To parse keyword arguments we use an enumeration value to track the current state. See http://cmake.org/gitweb?p=cmake.git;a=blob;f=Source/cmInstallCommand.cxx;hb=v2.8.7#l846 [^] for example.
- I prefer the property prefix to be "VS_" instead of "MSVS_" because we already have several other properties with the former prefix. |
|
|
|
(0029146)
|
|
Leonid Yurchenko
|
|
2012-04-13 04:58
|
|
Thanks for the feedback.
Added new patch that addresses this requests: include_external_msproject.patch
Tested compatibility with VS 10 and VS 11, and appeared that support of VS 10+ was broken. This new patch includes fix for this. |
|
|
|
(0029167)
|
|
Leonid Yurchenko
|
|
2012-04-16 07:04
|
|
|
Anything else we can improve in the patch to help it be included in the future releases? |
|
|
|
(0029176)
|
|
Brad King
|
|
2012-04-16 10:21
|
|
|
Please test patch "0001-include_external_msproject-Add-TYPE-GUID-PLATFORM-op.patch" which I adapted from your patch to adjust some documentation phrasing and fix keyword argument handling. |
|
|
|
(0029179)
|
|
Leonid Yurchenko
|
|
2012-04-16 11:08
|
|
|
It works the same way as previous did. So nothing was broken, everything is fine. Thanks for the documentation update. |
|
|
|
(0029180)
|
|
Brad King
|
|
2012-04-16 11:21
|
|
Great, thanks for testing.
In order to ensure this feature works and keeps working please extend the test case in Tests/VSExternalInclude to cover the optional arguments. |
|
|
|
(0029200)
|
|
Leonid Yurchenko
|
|
2012-04-17 06:45
|
|
Thanks. Is it possible for you to elaborate a bit, how you expect test case to check there optional arguments?
We are new to cmake unit tests, so it's unclear how to test if custom guid was set correctly (It may require VS plugin to be installed - it's the main purpose of this patch, to allow work with VS plugins, like wix projects).
Easiest solution that comes to mind is to check the solution manually with file(STRINGS REGEX) to whether guid was set correctly. Does it sounds ok? Or you had something different in mind? |
|
|
|
(0029201)
|
|
Brad King
|
|
2012-04-17 08:07
|
|
Re 0013120:0029200: We do need the VSExternalInclude test project to load and build to continue to cover the current cases it checks, so if a custom GUID can't load without a VS plugin then we'll need to put the test somewhere else.
The file(STRINGS) approach sounds reasonable so it can verify that the generator at least puts something in there. This would be the first test to do such direct testing on the generated solution so I'm not sure off the top of my head where it would best fit in the test suite. Instead please just provide a sample CMakeLists.txt file that covers a few cases using the optional arguments and a .cmake script that uses file(STRINGS) to validate the .sln files. I'll find a place to add them to the automated tests. |
|
|
|
(0029211)
|
|
Leonid Yurchenko
|
|
2012-04-18 09:58
|
|
Added include_external_msproject-test.zip, that contains:
- check_utils.cmake - helper functions to check vs solutions for certain values
- validate.cmake - validation script. It can be started with the command mode (cmake -P):
cmake -DTARGET_FILE=test.sln -DPROJECT_NAME=external -DPROJECT_GUID=111 -P validate.cmake
- simple_test folder - folder that contains test projects and cmake lists that uses this external projects and generates solution that should be tested.
Looking forward for the feedback, if we can change or improve something there. |
|
|
|
(0029213)
|
|
Brad King
|
|
2012-04-18 11:30
|
|
Thanks.
In validate.cmake shouldn't the test for project type call find_project_with_type instead of find_project_with_guid?
Also the TYPE/PLATFORM test without GUID cannot be checked with the tools provided. We need to find the project with the given type, extract its GUID, and then check the platform for that GUID later. Please add that capability to check_utils. Otherwise we cannot verify that setting a TYPE/PLATFORM works with an auto-generated GUID. |
|
|
|
(0029230)
|
|
Leonid Yurchenko
|
|
2012-04-19 07:53
|
|
|
Thanks for the feedback. Added include_external_msproject-test_v2.zip. Updated files are only: check_utils.cmake and validate.cmake |
|
|
|
(0029241)
|
|
Brad King
|
|
2012-04-19 09:37
|
|
|
|
|
(0032018)
|
|
Robert Maynard
|
|
2013-01-09 10:55
|
|
|
Closing resolved issues that have not been updated in more than 4 months. |
|
custom_guid_in_external_prj.patch (13,730) 2012-04-11 06:25
include_external_msproject-test.zip (3,259) 2012-04-18 09:55