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

fixes 1230: Error:string sub-command REGEX, mode REPLACE needs at least 6 arguments total to command #1231

Merged
merged 1 commit into from
Jul 27, 2016

Conversation

clumsy
Copy link
Contributor

@clumsy clumsy commented Jul 22, 2016

quoting the las argument of string REGEXP REPLACE function

…st 6 arguments total to command

quoting the las argument of string REGEXP REPLACE function
@ghost ghost added the CLA Signed label Jul 22, 2016
@adamretter
Copy link
Collaborator

adamretter commented Jul 24, 2016

@yuslepukhin Could you give me an opinion on the changes to CMakeLists.txt for Windows please?

@ghost ghost added the CLA Signed label Jul 24, 2016
@yuslepukhin
Copy link
Contributor

@adamretter @clumsy I think this change is OK and the quotes are necessary.
On the other hand this code is a copy of what runs in the main CMake file. I believe that the parent CMake vars are accessible in the included CMake projects. Thus if these changes are made in the parent CMake we might be able to keep one copy of the code and run it once. Disclaimer: I did not test this change.

@clumsy
Copy link
Contributor Author

clumsy commented Jul 26, 2016

@yuslepukhin What parent CMake file did you mean?
As far as I've read the quotes are indeed harmless, but I would not recommend making more changes that is necessary.
As for the provided patch - this is all that has allowed me to make a CMake build via Lion on my OS X.

@yuslepukhin
Copy link
Contributor

@clumsy This CMake is included into CMakeLists.txt off the root on line 309 when you invoke CMake with -DJNI=1 and so it gets built along with everything else.

However, it is up to you. Neither Travis nor AppVeor are invoked for this PR somehow. I would love someone to run those. Other than that I could merge it.

@ghost ghost added the CLA Signed label Jul 26, 2016
@yuslepukhin
Copy link
Contributor

@clumsy I am sorry, this is the parent CMake, I am reading this wrong. So if someone would test this on Windows, I could merge it.

@ghost ghost added the CLA Signed label Jul 26, 2016
@clumsy
Copy link
Contributor Author

clumsy commented Jul 26, 2016

@yuslepukhin Ok, I thought I started seeing things :)
I agree that it would be perfect is someone can run this too.
I tried the build on Windows (Visual Studio 2015) - it was fine.
If someone else tries it on Windows - it should be good enough.

BTW, are there any plans of making the CMake build cross-platform?

@ghost ghost added the CLA Signed label Jul 26, 2016
@yuslepukhin
Copy link
Contributor

@clumsy There is an effort that I supposed to contribute to but I have been swamped lately with work. #1135

I will try to verify the change when I get a minute today

@ghost ghost added the CLA Signed label Jul 26, 2016
@clumsy
Copy link
Contributor Author

clumsy commented Jul 27, 2016

Thanks, I can participate in cross-platform build initiative from the OS X side.
Do you keep the current state in a public branch or that PR is all we have as of now?

@adamretter
Copy link
Collaborator

Closes #1230

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

Successfully merging this pull request may close these issues.

4 participants