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

Quote string(REPLACE) cmake command fourth argument #801

Merged
merged 1 commit into from
Dec 26, 2016

Conversation

Manu343726
Copy link
Contributor

This commit fixes potential errors when CONAN_VERSION variable
in conan_split_version() function is not set so the command
is interpreted as with 3 arguments only. This could also help if
the variable has spaces so the command is interpreted with 5 arguments.
As a rule, always quote cmake command arguments

See https://travis-ci.org/foonathan/type_safe/jobs/186587154#L756 and https://travis-ci.org/ChaiScript/ChaiScript/jobs/186450865#L558 for examples of this error.

This commit fixes potential errors when CONAN_VERSION variable
in conan_split_version() function is not set so the command
is interpreted as with 3 arguments only. This could also help if
the variable has spaces so the command is interpreted with 5 arguments.
As a rule, always quote cmake command arguments
@CLAassistant
Copy link

CLAassistant commented Dec 25, 2016

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Dec 25, 2016

Current coverage is 94.51% (diff: 100%)

Merging #801 into develop will not change coverage

@@            develop       #801   diff @@
==========================================
  Files           228        228          
  Lines         16679      16679          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          15764      15764          
  Misses          915        915          
  Partials          0          0          

Powered by Codecov. Last update 1a5eaec...6ff9e0f

@lasote
Copy link
Contributor

lasote commented Dec 26, 2016

So why the variable is empty?

@memsharded
Copy link
Member

It could be empty, because conan CMake macros are assuming CMAKE_CXX_COMPILER_VERSION has a value. We are just checking it for the compiler version, assuming that C++ compiler will be always installed, but it could happen that it is not. So IMO it can be safely merged.

@memsharded
Copy link
Member

But I have a question, from the output of https://travis-ci.org/ChaiScript/ChaiScript/jobs/186450865#L558, I don't understand the error, CMAKE_CXX_COMPILER_VERSION, should have a value for that case, right? Just want to understand what is happening, maybe there is some underlying issue we could better address.

@lasote
Copy link
Contributor

lasote commented Dec 26, 2016

@memsharded that's why I asked, I don't fully understand why the variable is empty in that case.

@Manu343726
Copy link
Contributor Author

Manu343726 commented Dec 26, 2016

So why the variable is empty?

That's another question. What I'm really trying to fix is the usage of a cmake antipattern in your generated code. But you're right, maybe there's a deeper issue with that empty variable.

@memsharded memsharded merged commit 0c69373 into conan-io:develop Dec 26, 2016
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

Successfully merging this pull request may close these issues.

5 participants