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

Add support for CMake-presets. #1656

Merged
merged 1 commit into from
May 25, 2021

Conversation

jehelset
Copy link
Contributor

@jehelset jehelset commented Mar 7, 2021

Support configuring, building and testing CMake projects using cmake-presets.

Introduces projectile--cmake-command for CMake projects which computes a command based on the command-type. If CMake preset support is enabled through the projectile-enable-cmake-presets custom, and if presets are supported for the command-type by CMake, CMake preset-files are parsed and a completing-read is done on the names of the command presets along with an option for manual configuration. If not supported, or there are no presets specified for the given command-type, it reverts to manual configuration as before. Configure, build and test commands for CMake projects are all implemented in terms of this command.

To support presets the compilation-dir for CMake-projects was removed, and the manual commands were adjusted to support being run from the project-root.

Needs json-parse-buffer to be available, else falls back to manual configuration.

Tested manually with CMake 3.19 (presets supported), and 3.18 (not supported) only, and emacs 27.1 (json-parse-buffer available) and emacs 26.3 (json-parse-buffer unavailable). Not sure how to to project-type specific testing. Initially implemented at jehelset/projectile-cmake.


Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

Thanks!

@jehelset
Copy link
Contributor Author

jehelset commented Mar 7, 2021

Not sure if this too much CMake for projectile. I guess this needs to check if json is available in a better way. Not quite sure what is the best method to do it.

@jehelset jehelset force-pushed the cmake-configure-presets branch 2 times, most recently from 277e1b7 to e5b77df Compare March 13, 2021 12:47
@jehelset jehelset marked this pull request as ready for review March 13, 2021 12:49
@jehelset
Copy link
Contributor Author

Ended up just relying on json-parse-buffer being available rather than trying to support older versions of emacs.

@bbatsov
Copy link
Owner

bbatsov commented Mar 15, 2021

I'm okay with the proposed addition, but this has to be documented well if we expect people to make good use of it. I'm also wondering if there might be a case when someone would prefer the old behaviour (e.g. should we have a defcustom for it).

projectile.el Outdated Show resolved Hide resolved
projectile.el Outdated Show resolved Hide resolved
@jehelset
Copy link
Contributor Author

I'm okay with the proposed addition, but this has to be documented well if we expect people to make good use of it. I'm also wondering if there might be a case when someone would prefer the old behaviour (e.g. should we have a defcustom for it).

Yes. Where do you want me to add the documentation? Somewhere under projects.adoc? Do you mean a defcustom for specifying the configuration-cmd (ie a projectile-cmake-project-configure-function), or one to toggle preset integration on and off?

@jehelset
Copy link
Contributor Author

I tried adding a defcustom to opt-in to preset-configuration, as well as improving the docstrings and naming. It seems that the snapshot CI is failing due to some other long docstring.

@jehelset jehelset requested a review from bbatsov March 21, 2021 09:19
@jehelset
Copy link
Contributor Author

CMake just got support for build and test presets in 3.20 (https://cmake.org/cmake/help/latest/release/3.20.html#presets), do you mind if I add that also here?

@bbatsov
Copy link
Owner

bbatsov commented Mar 29, 2021 via email

@jehelset
Copy link
Contributor Author

I will squash / tidy the commits afterwards. It seemed to mess with the change-requests. I tried adding preset-support for compile and test commands.

projectile.el Outdated
;;;; Constant signifying opting out of CMake preset commands.
(defconst projectile--cmake-no-preset "*no preset*")

(defun projectile--cmake-version()
Copy link
Owner

Choose a reason for hiding this comment

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

It's idiomatic to have a space between the function name and its param list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

projectile.el Outdated
'((:configure-command . (3 19))
(:compile-command . (3 20))
(:test-command . (3 20))))
(defun projectile--cmake-command-presets-supported(command-type)
Copy link
Owner

Choose a reason for hiding this comment

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

I normally add blank lines around function defs.

Copy link
Owner

Choose a reason for hiding this comment

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

Btw, isn't it easier to just set 3.20 as the minimum version for everything instead of having it so granular?

Copy link
Contributor Author

@jehelset jehelset May 24, 2021

Choose a reason for hiding this comment

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

Fixed the blank lines.

I think the combination of how nice presets are, and CMake often being installed via the system package manager on non-rolling distros justifies the level of granularity. The configuration-preset is by far the most valuable. See f.ex: https://packages.debian.org/sid/cmake (unstable is still on 3.18).

I can remove it if it's too much, but I would be hard pressed to make a choice between setting minimum version to 3.19 and dropping support for compile and test-commands or setting the minimum version to 3.20!

@bbatsov
Copy link
Owner

bbatsov commented May 24, 2021

Where do you want me to add the documentation? Somewhere under projects.adoc?

Either there or under "Configuration".

Do you mean a defcustom for specifying the configuration-cmd (ie a projectile-cmake-project-configure-function), or one to toggle preset integration on and off?

I was thinking of something that toggles the new functionality globally. (mostly so people can easily opt out of it, if they run into any issues)

@jehelset
Copy link
Contributor Author

Where do you want me to add the documentation? Somewhere under projects.adoc?

Either there or under "Configuration".

Do you mean a defcustom for specifying the configuration-cmd (ie a projectile-cmake-project-configure-function), or one to toggle preset integration on and off?

I was thinking of something that toggles the new functionality globally. (mostly so people can easily opt out of it, if they run into any issues)

I previously added the projectile-enable-cmake-presets defcustom, and set it to nil by default, so that users can opt-in instead for now.

CHANGELOG.md Outdated
* Add `projectile-update-project-type-function` for updating the properties of existing project types
* [#1656](https://github.com/bbatsov/projectile/pull/1656) Add support for CMake configure, build and test presets. Enabled by setting `projectile-cmake-presets` to non-nil, disabled by default.
Copy link
Owner

Choose a reason for hiding this comment

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

You forgot : here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

CHANGELOG.md Outdated

### Changes

* Add `project` param to `projectile-generate-process-name`.
* [#1608](https://github.com/bbatsov/projectile/pull/1608): Use rebar3 build system by default for Erlang projects.
* Rename `projectile-project-root-files-functions` to `projectile-project-root-functions`.
* [#1647](https://github.com/bbatsov/projectile/issues/1647): Use "-B" in the mvn commands to avoid ANSI coloring clutter in the compile buffer
* [#1656](https://github.com/bbatsov/projectile/pull/1656) CMake compilation-dir removed to accomodate preset support, commands adjusted to run from project-root, with "build" still being the default build-directory. The non-preset test-command now uses "cmake" with "--target test" instead of "ctest".
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@bbatsov
Copy link
Owner

bbatsov commented May 24, 2021

It seems you'll also have to rebase on top of master.

@jehelset
Copy link
Contributor Author

Squashed and rebased.

@bbatsov
Copy link
Owner

bbatsov commented May 24, 2021

image

Are you sure you rebased?

@jehelset
Copy link
Contributor Author

Hm, I thought so.

@jehelset
Copy link
Contributor Author

I fetched from wrong remote... Sorry, sec.

@jehelset
Copy link
Contributor Author

There we go. :P

Introduce a `projectile--cmake-command`, for CMake projects
parametrized on command-type, and route configure, build and test
commands for cmake projects through this.

This command ensures that presets for the given command-type is
supported by the installed version of CMake, that `json-parse-buffer`
is available, that the user has opted in to preset support through
the `projectile-enable-cmake-presets` custom, and that there is at
least one preset available for the given command type.

If all these conditions hold it parses the preset files and prompts
the user to select a preset, or to opt-out of preset configuration,
and creates a manual or preset command based on the users selection.
Else it will revert to manual configuration.
@bbatsov bbatsov merged commit e627bd7 into bbatsov:master May 25, 2021
@bbatsov
Copy link
Owner

bbatsov commented May 25, 2021

Thanks!

@jehelset jehelset deleted the cmake-configure-presets branch May 25, 2021 06:42
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.

2 participants