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

Prevent projectile-project-root from masking subprojects #21

Merged
merged 2 commits into from Mar 3, 2018

Conversation

@jaelsasser
Copy link
Contributor

commented Mar 3, 2018

With apologies to @MaskRay for having absolutely no luck with Projectile… I love the integrations, but they've given me nothing but grief on my work's monorepo.


Only falling back to projectile-project-root if cquery--get-root
cannot find a valid compile_commands.json or .cquery by traversing
upwards from the current directory.

Allows cquery to work out-of-the-box for the following layout:

./project/
  - .git/
  - src/
    - compile_commands.json
    - Makefile
@MaskRay

This comment has been minimized.

Copy link
Member

commented Mar 3, 2018

You can use cquery-project-roots, rather than change the logic.

Locating compile_commands.json is bad for projects with generated files in the build directory.

@MaskRay

This comment has been minimized.

Copy link
Member

commented Mar 3, 2018

If you have other concerns, chime in in emacs-lsp/lsp-mode#293

@jaelsasser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2018

Looking through the discussion now.

My main annoyance with this is that it breaks pretty much every project I use with lsp-mode with. I burned about half an hour regenerating compile_commands.json files and bisecting cquery itself to try and find the issue, only to realize that the project root detection had changed drastically overnight.

Also, the existing logic is a violation of the compile_commands.json spec:

Clang tools are pointed to the top of the build directory to detect the file and use the compilation database to parse C++ code in the source tree.

@MaskRay

This comment has been minimized.

Copy link
Member

commented Mar 3, 2018

Having a symlink in the project root compile_commands.json pointing to build/compile_commands.json is easy for clients. You PR will make generated files hard

@MaskRay

This comment has been minimized.

Copy link
Member

commented Mar 3, 2018

.cquery can be hierarchical and locate-dominating-file will incorrectly take the subdirectories as :rootUri of files in subdirectories. locate-dominating-file is a convenience tool for non-trivial projects but it gets in the way for larger projects. Your PR is not moving in the right direction.

@jaelsasser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2018

I can't symlink in my project root, I have about eight or nine different subprojects in my work monorepo. This change broke all of them.

@MaskRay how about I revise the patch to centre around a defcustom list option to determine the order of the built-in root file detectors? I'm reading through the documentation for the allowed :type params and I should be able to set something up that makes it easy for users to:

  • provide a string to be passed to locate-dominating-file
  • provide a directory to be matched using cquery-project-roots
  • provide a function to be executed (with cquery-get-root-projectile provided by default)
  • provide arbitrary functions to do complicated matching
  • disable specific matchers that they don't want (i.e. remove ".cquery" from the default list definition to never match on a .cquery definition)
  • re-order definitions to allow certain matchers to take precedence over others
@MaskRay

This comment has been minimized.

Copy link
Member

commented Mar 3, 2018

A customizable project root detection list will be helpful, but IMO it re-invents logic of projectile

(defcustom projectile-project-root-files-functions
  '(projectile-root-local
    projectile-root-bottom-up
    projectile-root-top-down
    projectile-root-top-down-recurring)
  "A list of functions for finding project roots."
  :group 'projectile
  :type '(repeat function))

Your scheme will be definitely better than the current makeshift but it would also complicate things.

I raised emacs-lsp/lsp-mode#293 because I hope other language client/server users discuss this as it is a general problem.

When the general solution surfaces, the cquery project detection logic will become unnecessary.

cquery-project-root-function cquery-project-roots shall be much simpler approaches for your case.

@jaelsasser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2018

My issue is that I never projectile to have anything whatsoever to do with setting up an LSP buffer. My goal is to get a way to disable it (or at least lower its priority preference) without needing a defadvice hack.

On master right now, I need to explicitly blacklist / whitelist to work around the fact that lsp-cquery thinks that it can start in every C/C++ source directory that happens to be checked into a git repo. I got the following on my local checkout of WireGuard, which doesn't have a compile_commands.json file:

LIBCLANG TOOLING ERROR: json-compilation-database: Error while opening JSON database: No such file or directory

2018-03-03 12:04:50.773 (   0.001s) [querydb      ]        project.cc:310   WARN| cquery has no clang arguments. Considering adding either a compile_commands.json or .cquery file. See the cquery README for more information.
introduce defcustom to control ordering within cquery--get-root
Obsoletes `cquery-project-root-function` in favour of a new defcustom,
`cquery-project-root-matchers`, that allows a user to tweak how cquery.el
discovers its project root.

In the default configuration, `projectile-project-root` wil accurately
find the outermost `compile_commands.json` in the example project below:

projectA/
- .git/
- Makefile
- compile_commands.json -> src/compile_commands.json
- src/
  - compile_commands.json

Removing `projectile-project-root` from the head of the list allows
users to work with  project roots within subdirectories of a git repo:

projectB/
- .git/
- src/
  - Makefile
  - compile_commands.json

@jaelsasser jaelsasser force-pushed the jaelsasser:master branch from fbae80c to 6b83282 Mar 3, 2018

@MaskRay

This comment has been minimized.

Copy link
Member

commented Mar 3, 2018

If you call lsp-cquery-enable in c-mode-common-hook, you may customize lsp-project-blacklist

  (use-package cquery
    :commands lsp-cquery-enable
    :init (add-hook 'c-mode-common-hook #'cquery//enable)
;; .........
    ))
@jaelsasser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2018

@MaskRay how's the revised patch look? I think this strikes a compromise between our different setups -- it allows setups with layered projects where the only compile_commands.json that matters is the topmost one, while allowing me to change the defcustom to ignore projectile for my weird work monorepo.

@jaelsasser jaelsasser force-pushed the jaelsasser:master branch from 0d8b429 to bb86566 Mar 3, 2018

@@ -99,6 +94,23 @@ does not belong to the current workspace.
:type '(repeat directory)
:group 'cquery)

(defcustom cquery-project-root-matchers
'(projectile-project-root "compile_commands.json" ".cquery")

This comment has been minimized.

Copy link
@MaskRay

MaskRay Mar 3, 2018

Member

".cquery" overrides "compile_commands.json" on the server side.

This comment has been minimized.

Copy link
@jaelsasser

jaelsasser Mar 3, 2018

Author Contributor

Ah, thanks, I'll swap them around. I was preserving the order of the old cquery--get-root, but I'll change it so that it matches the executable behaviour.

This comment has been minimized.

Copy link
@MaskRay

MaskRay Mar 3, 2018

Member

Thinking it over, "compile_commands.json" before ".cquery" is reasonable.

For people have hierarchical ".cquery" setting, they can put a compile_commands.json in the project root because otherwise .cquery in subdirectories will mark the :rootUri

A bit weird though

@MaskRay MaskRay merged commit 3ed96eb into cquery-project:master Mar 3, 2018

@jaelsasser

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.