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

Should conan_output_dirs_setup in conanbuildinfo.cmake be optional? #1448

Closed
amues opened this issue Jun 28, 2017 · 8 comments
Closed

Should conan_output_dirs_setup in conanbuildinfo.cmake be optional? #1448

amues opened this issue Jun 28, 2017 · 8 comments
Milestone

Comments

@amues
Copy link

amues commented Jun 28, 2017

Running conan 0.24.0 installed with pip.

When consuming libraries, the conan install <path> step generates a conanbuildinfo.cmake file with the macro conan_basic_setup, which in turn calls conan_output_dirs_setup. This macro sets CMake variables that cause binary and library files to be moved to <build_dir>/bin and <build_dir>/lib, respectively.

This can be easily changed by commenting out that line (e.g. running grep in an automated setup), but if the user wants to keep those files in their original location in the build directory, would it make sense for conan_output_dirs_setup to be inside a conditional (could test a CMake variable or an argument to conan_basic_setup, as is done with TARGETS)?

@memsharded
Copy link
Member

Actually, the conan_basic_setup() is just a list of macros so if you want a different behavior, you can just call the macros you want. That would be a bit easier than just adding many different arguments to enable or disable different parts fo the setup (like compiler-check, adding paths to cmake-find paths, etc), which would also be suitable to be enabled/disabled by the user. So when I have to customize the behavior, I just copy the contents of conan_basic_setup() and create a custom version of it.

Another easy solution is to redefine/restore the paths just after the call to conan_basic_setup().

In any case, I am not opposed to adding that argument to disable it, anyone else feels that it deserves an argument? Thanks!

@amues
Copy link
Author

amues commented Jun 28, 2017

My first thought was to simply copy the contents of conan_basic_setup() to my CMakeLists.txt file and remove the unwanted parts, but my concern in this case is the duplication; if something is added or changed in conan_basic_setup(), then my copy of the contents may diverge from it if I don't manually update it.

I agree that adding arguments to enable or disable each and every part of the setup can result in a large number of arguments being available, which can make understanding the generated file more complex. So for now I'll just redefine the paths after the conan_basic_setup() call, but let's hear if anyone else has an opinion about this 😀

@madpipeline
Copy link

Hi,

I'm also for making this optional. Disabled by default IMO. I don't know in what scenarios this would be desired, so my actual current opinion is to simply remove it.

For now I've also redefined the paths after the conan_basic_setup() call. But I believe that this is a workaround that shouldn't be necessary.

@memsharded
Copy link
Member

Agree, it should be optional. Disabled by default is not possible, it would break many, many builds, so we need to make it opt-in. It is a very useful default for Windows build in which Visual Studio will put binaries under Debug/Release subfolders, and then the package() method might need to be customized to get them, adding extra complexity to it.

But, yes, lets make it opt-in.

@memsharded memsharded added this to the 0.26 milestone Aug 7, 2017
@amues
Copy link
Author

amues commented Aug 25, 2017

The conditional inside the conan_basic_setup macro is if(NOT "${ARGV0}" STREQUAL "TARGETS"), but according to CMake 3.8 documentation for the macro command (https://cmake.org/cmake/help/v3.8/command/macro.html), referencing ARGV arguments beyond ${ARGC} has undefined behaviour.

This does not seem to cause problems, as in all cases I tested referencing arguments beyond the last, I got empty strings, but maybe it would be a good idea not to rely on this. The issue is the only solution I can think to it requires adding an extra check:

if(${ARGC} GREATER 0 AND "${ARGV0}" STREQUAL "TARGETS")
    ...

Any ideas on this?

@memsharded
Copy link
Member

Thanks @amues for the reference, yes, you are right, ${ARGC} GREATER 0 is the way to go. Checks for args will have to use this syntax.

@memsharded
Copy link
Member

Finally, I am using cmake_parse_arguments(), check #1665

@memsharded
Copy link
Member

Done, you have NO_OUTPUT_DIRS as argument to conan_basic_setup() released in 0.26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants