Skip to content

Added provision to pass misc flags to cmake#89

Open
Nidish96 wants to merge 11 commits into
atilaneves:masterfrom
Nidish96:nidish
Open

Added provision to pass misc flags to cmake#89
Nidish96 wants to merge 11 commits into
atilaneves:masterfrom
Nidish96:nidish

Conversation

@Nidish96
Copy link
Copy Markdown

@Nidish96 Nidish96 commented Feb 12, 2017

In invoking cmake from emacs, previously there was no provision to pass flags to cmake. For example, it was not possible to specify the build type or install prefix, etc.
The current commit replaces the (start-process "cmake" ...) call with (apply #'start-process "cmake" ...) and defines a list, cmake-ide-cmake-command-flags which is initialized to nil and may be set in .dir-locals.el.

example (in .dir-locals.el):
((nil . ((cmake-ide-project-dir . "path/to/project") (cmake-ide-build-dir . "path/to/build") (cmake-ide-make-command . "make install") (cmake-ide-cmake-command-flags . ("-DCMAKE_INSTALL_PREFIX=/path/to/install" "-DCMAKE_BUILD_TYPE=Debug")))))

PS: Forgive me for the awful commits - am just learning to use magit on emacs :P

@myrgy
Copy link
Copy Markdown

myrgy commented Jun 30, 2017

useful patch (y)

@atilaneves
Copy link
Copy Markdown
Owner

@myrgy I agree this is useful but the review comments were not addressed.

@myrgy
Copy link
Copy Markdown

myrgy commented Jun 30, 2017

atilaneves , could you please describe the issue with that PR.
Seems that his author fogot about it. I'll try to complete that stuff.

P.S.
Actually I did something similar locally to be able to define project specific things per-project.

@atilaneves
Copy link
Copy Markdown
Owner

@myrgy My comments on the original PR are still visible.

@Nidish96
Copy link
Copy Markdown
Author

Nidish96 commented Jul 1, 2017

@atilaneves I can't really see the review comments

Comment thread cmake-ide.el
:group 'cmake-ide
:safe #'stringp)

(defcustom cmake-ide-cmake-command-flags
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just test comment

@myrgy
Copy link
Copy Markdown

myrgy commented Jul 2, 2017

@atilaneves , @Nidish96 I'm very sorry, but I can't see review comments as well.
I did one test comment, it's visible.
@atilaneves , is it possible that you forgot to publish review comments?

@atilaneves
Copy link
Copy Markdown
Owner

Ugh, I seem to have forgotten to actually commit the review, sorry about that.

Comment thread cmake-ide.el
nil
"List of misc flags passed to the cmake invocation."
:group 'cmake-ide
:safe #'stringp)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a list of strings, no?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I have done something similar, and a string seems fine, you can set it to a bunch of option (like -DCMAKE_BUILD_TYPE=Debug -DOtherUsefullDef"
Then, I set it to "-DCMAKE_BUILD_TYPE=Debug" by default and use it to define the build directory.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad - it should be listp

Comment thread cmake-ide.el
(cmake-ide--message "Starting rdm server")
(with-current-buffer buf (start-process "rdm" (current-buffer)
cmake-ide-rdm-executable
"-j 2" "-i 40" "-a 10"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed the number of jobs to 2 - you can use the previous configuration itself

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed the number of jobs to 2 - you can use the previous configuration itself

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but why? This should be at least configureable.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think if we add this settings as cmake-ide-rdm-options?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myrgy That makes sense.

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.

4 participants