You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Chug Rolke <cr...@redhat.com> on 2015/06/29 23:07:49 UTC
Review Request 36020: Create executable binaries in same folder as
DLL files
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36020/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher and Steve Huston.
Bugs: proton-926
https://issues.apache.org/jira/browse/proton-926
Repository: qpid-proton-git
Description
-------
Same as https://issues.apache.org/jira/browse/QPID-6611 for qpid this patch creates .exe files in common place. It is windows only with no change for linux.
The patch moves intermediate files around in the build tree only. It does not change the layout of the install directory.
Diffs
-----
examples/c/messenger/CMakeLists.txt 1b32d0c
proton-c/CMakeLists.txt a1cf0e4
proton-c/src/tests/CMakeLists.txt ac749e1
tests/tools/apps/c/CMakeLists.txt 9507c1f
Diff: https://reviews.apache.org/r/36020/diff/
Testing
-------
Passes all tests on windows and linux.
Thanks,
Chug Rolke
Re: Review Request 36020: Create executable binaries in same folder
as DLL files
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36020/#review89809
-----------------------------------------------------------
I'm not convinced there is a good reason for this change.
If you want to use the executables in the build tree you need to run the config.sh or config.bat scripts there to set this up in any case - not doing this may cause some confusing issues down the line.
On Windows this script will include the DLLs in PATH so I don't really see what this change brings. It also has the downside (IMO) of making the layout of the Windows build tree different from the Unix build tree (of course this is the main purpose of the change in some ways). I'm not convinced there are sufficient benefits to warrant the inconsistent tree layout.
Be that as it may: I think the overall strategy here is flawed because of the way that CMake strings and lists work. It may work on the versions of CMake that you have tested it with, but it won't work with other versions (I'm guessing older versions).
when you so set(BLAH FOO "/path/to/something") then BLAH gets set to "FOO;/path/to/something" so when you later do set_target_property(file ${BLAH}) you actually get set_target_property(file "FOO;/path/to/something") whereas the syntax you are looking for is set_target_property(file FOO /path/to/something).
It seems that the later versions of CMake may account for this somehow, earlier ones do not.
- Andrew Stitcher
On June 29, 2015, 9:07 p.m., Chug Rolke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36020/
> -----------------------------------------------------------
>
> (Updated June 29, 2015, 9:07 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Steve Huston.
>
>
> Bugs: proton-926
> https://issues.apache.org/jira/browse/proton-926
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Same as https://issues.apache.org/jira/browse/QPID-6611 for qpid this patch creates .exe files in common place. It is windows only with no change for linux.
>
> The patch moves intermediate files around in the build tree only. It does not change the layout of the install directory.
>
>
> Diffs
> -----
>
> examples/c/messenger/CMakeLists.txt 1b32d0c
> proton-c/CMakeLists.txt a1cf0e4
> proton-c/src/tests/CMakeLists.txt ac749e1
> tests/tools/apps/c/CMakeLists.txt 9507c1f
>
> Diff: https://reviews.apache.org/r/36020/diff/
>
>
> Testing
> -------
>
> Passes all tests on windows and linux.
>
>
> Thanks,
>
> Chug Rolke
>
>
Re: Review Request 36020: Create executable binaries in same folder
as DLL files
Posted by Steve Huston <sh...@riverace.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36020/#review89905
-----------------------------------------------------------
Similar to the Qpid C++ change that's analogous to this, I prefer to use PATH and installers to take care of this problem. OS-specific things I'd rather keep out of the cmake files. They're tangled up enough as it is.
- Steve Huston
On June 29, 2015, 9:07 p.m., Chug Rolke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36020/
> -----------------------------------------------------------
>
> (Updated June 29, 2015, 9:07 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Steve Huston.
>
>
> Bugs: proton-926
> https://issues.apache.org/jira/browse/proton-926
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> Same as https://issues.apache.org/jira/browse/QPID-6611 for qpid this patch creates .exe files in common place. It is windows only with no change for linux.
>
> The patch moves intermediate files around in the build tree only. It does not change the layout of the install directory.
>
>
> Diffs
> -----
>
> examples/c/messenger/CMakeLists.txt 1b32d0c
> proton-c/CMakeLists.txt a1cf0e4
> proton-c/src/tests/CMakeLists.txt ac749e1
> tests/tools/apps/c/CMakeLists.txt 9507c1f
>
> Diff: https://reviews.apache.org/r/36020/diff/
>
>
> Testing
> -------
>
> Passes all tests on windows and linux.
>
>
> Thanks,
>
> Chug Rolke
>
>