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

wip: fix #396, #395 and #398 #409

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

wip: fix #396, #395 and #398 #409

wants to merge 9 commits into from

Conversation

wgrr
Copy link
Contributor

@wgrr wgrr commented Aug 1, 2021

The intention of this draft is mostly get feedback on how configurations should look like for plugins.
I feel doing all at the same place would be better than spread them across plugins. I made up a quick-lint-js search function from node_modules, however I'm unsure if it's the desire, currently the user must choose one or implement their search function, or by default use emacs built-in 'exec-path search function, i think we might want by default try npm, then exec-path? Lint requests timing is a missing unified configuration too.

Also, currently to find node_modules folder, we search backwards from buffers default-directory until we found it or not, Emacs 27 has a yet unstable API for a builtin library for projects named project, after version 0.3.0 it has a function project-root we could use or try before instead, but it's only available from elpa or emacs >= 28, which still in development.

@strager What you feel? is this on the right track?

Copy link
Collaborator

@strager strager left a comment

Choose a reason for hiding this comment

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

See must fix comment. Otherwise, this looks good.

I made up a quick-lint-js search function from node_modules, however I'm unsure if it's the desire, currently the user must choose one or implement their search function, or by default use emacs built-in 'exec-path search function, i think we might want by default try npm, then exec-path?

We must not search node_modules by default until code signing (#133) and signature checking is implemented. Otherwise, the plugin introduces an arbitrary code execution security risk.

Must fix: I don't want to even make node_modules searching an option, because people will enable it without understanding the security risks. If we do want the feature, we should discourage its use with a scary message.

Comment on lines +49 to +76
"quick-lint-js exited with a deadly signal.\n\
Please consider filing a bug at\
https://github.com/quick-lint/quick-lint-js/issues/new\n\
qjls-stderr-buf:\n\
%s\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

(when (zerop (buffer-size))
(goto-char (point-min))
(insert-char ?\())
;; the above left a '(' danlging or crashed and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo:

Suggested change
;; the above left a '(' danlging or crashed and
;; the above left a '(' dangling or crashed and

Comment on lines -71 to -74
(eval (let ((file (buffer-file-name)))
(if file
`("--path-for-config-search" ,file)
())))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you intentionally rewrite this code in this commit? This change seems unrelated to unifying settings. (The change looks fine, though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you intentionally rewrite this code in this commit?

No, I think I merged the wrong piece on a conflict, i'll revert

@strager
Copy link
Collaborator

strager commented Aug 4, 2021

@strager What you feel? is this on the right track?

Unifying the plugin settings is a good idea!

@wgrr
Copy link
Contributor Author

wgrr commented Aug 4, 2021

I don't want to even make node_modules searching an option, because people will enable it without understanding the security risks. If we do want the feature, we should discourage its use with a scary message.

Oh, i made this becasue if im not mistaken, i recall you mentioned on a past code review a "what if quick-lint-js was installed with npm problem", i infered that meant "look for quick-lint-js in node_modules", if that's not it, there's any secure way to do it? or should we gave up on that?

@strager
Copy link
Collaborator

strager commented Aug 5, 2021

i recall you mentioned on a past code review a "what if quick-lint-js was installed with npm problem",

Yup.

i infered that meant "look for quick-lint-js in node_modules",

I did want that feature. I started implementing it for Vim, then realized the security problem with it. So I changed my opinion for now. I do eventually want the feature though.

if that's not it, there's any secure way to do it?

Not yet. We need a way to ask "is this a trusted executable?". I haven't built that infrastructure yet. I filed #411 for making this feature.

or should we gave up on that?

Let's give up on node_modules for now. We can save your patch for later.

This is causing diagnostics to not be cleared sometimes (see quick-lint#398), it
was first introduced for developement, because restarting Flymake
without changes in buffers would not delete old diagnostics, I think
it's fine if we not specify this option.

However I suspect this is a bug in Flymake.

Signed-off-by: wagner riffel <[email protected]>
(cherry picked from commit 6a223f8)
Currently there is at least two customizeble variables per plugin,
using more than one plugin at time is annoying to configure, this
change generalize and centralize configuration in a single place.

This commit also adds configuration for searching quick-lint-js
executable, along with helper to find it under npm folder structure.

Signed-off-by: wagner riffel <[email protected]>
Unlike M-x lsp, M-x lsp-quicklintjs doesn't prompt for a root folder.
It does this by always before calling (lsp) saving on its own state
file the buffer default directory as a project root.

Signed-off-by: wagner riffel <[email protected]>
@wgrr
Copy link
Contributor Author

wgrr commented Aug 11, 2021

@strager Could you review these CMake files? i havent written one in ages. Also, I'm still unsure about their need, i've written them because i'm unsure if emacs generated files are forward and backward compatible, so i left that to the host building the package decide (that's what happens in elpa/melpa, for example). But i have a feeling we could generate and track those files beforehand.

@wgrr
Copy link
Contributor Author

wgrr commented Aug 11, 2021

Also, explaining how #395 and #396 are fixed

7646781 fixes #396 by relying on Emacs builtin package library which automatically loads *-autoloads.el files, and those files are generated by quicklintjs-pkg.el. (the suggestion in the issue doesn't fix the issue because it would just move the problem elsewhere, e.g: it would need the user to add a (add-to-list ...) instead of require). Furthermore, this change will make the local installed symetric with the remotes i will add later (melpa and dpkg). Another path i could've taken is making a site-start.el which is a magic file automatically loaded by emacs, but using the builtin package library looks like the standard.

316e270 fixes #395 by adding a wrapper command which marks the default-directory as the root folder, unfortunately i think we can't transparently fix this, my first attempt was to use lsp-mode provided callback lsp-before-initialize-hook, but this is called after the prompt. I oppose to set the workaround variable lsp-auto-guess-root because it overwrites a global user setting and may leave lsp-mode in a broken state if projectile or project are previously misconfigured.

remove require on cl-seq not present in emacs24, cl-remove-if-not is
loaded by default.

Signed-off-by: wagner riffel <[email protected]>
Copy link
Collaborator

@strager strager left a comment

Choose a reason for hiding this comment

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

@strager Could you review these CMake files?

They look good to me.

install(
DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/lsp-quicklintjs-0.0.1
DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/emacs/site-lisp/elpa)
endif ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need five separate packages? Could we make just one package? Seems simpler to make one package IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we make just one package?

Yes, to use what emacs calls "multi-file" packages, it would require tar available while building the packages tho, i didn't use it here because i imagine that's unlikely to tar be available on a Windows machine, what you think? i'm fine with both ways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it would require tar available while building the packages tho, i didn't use it here because i imagine that's unlikely to tar be available on a Windows machine, what you think?

CMake provides a tool to create tar archives: https://cmake.org/cmake/help/v3.21/manual/cmake.1.html#run-a-command-line-tool

$ cmake -E tar czf src.tar.gz src
$ file src.tar.gz
src.tar.gz: gzip compressed data, last modified: Fri Aug 13 18:15:35 2021, from Unix, original size 1392640

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@strager i made the change to use a tar archive as a single package, besides that it feel clunky, it's discouraged by package archives, do you think the last "problem" in this list applies here? i do think. https://github.com/melpa/melpa/blob/5d49574/CONTRIBUTING.org#fixing-typical-problems

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's discouraged by package archives

Having multiple .el files in a package is discouraged? O_o I'm unsure if that's what the documentation actually says.

do you think the last "problem" in this list applies here? i do think. https://github.com/melpa/melpa/blob/5d49574/CONTRIBUTING.org#fixing-typical-problems

It says:

MELPA only looks at the main .el file (or -pkg.el file, if provided, though we discourage that).

What does it mean to "look at" an .el file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does it mean to "look at" an .el file?

In short, an .el file can be package on its own, its format something along the lines:

;;; <name>.el ---
;;; <headers>

<code>

;;; <name>.el ends here

so when it says "look at" an .el file, is an .el file which is in this package format, when a package has multiple .el files, this format is not used, instead there should be a tar archive that contains all .el files along with a magic <name>-pkg.el, which must call (define-package ...args).

Copy link
Collaborator

Choose a reason for hiding this comment

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

[a tar archive is] discouraged by package archives, do you think the last "problem" in this list applies here? i do think. https://github.com/melpa/melpa/blob/5d49574/CONTRIBUTING.org#fixing-typical-problems

I think we should do one of two things:

  • merge all of our .el files into one .el file
  • create a tar, even though MELPA docs say "we discourage that"

I'm happy with the tar if it's already implemented and if it makes developing the plugin easier for us.

${QUICK_LINT_JS_EMACS}
${CMAKE_CURRENT_LIST_DIR}/quicklintjs-pkg.el
${FILE}
VERBATIM)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: This command is pretty noisy during building. Can we silence the output unless there is an error or warning?

[23/226] Generating quicklintjs-0.0.1
Generating autoloads for quicklintjs.el...
Generating autoloads for quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs-autoloads.el
Checking /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1...
Compiling /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs-autoloads.el...
Compiling /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs-pkg.el...
Compiling /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs.el...
Done (Total of 1 file compiled, 2 skipped)
[33/226] Generating flycheck-quicklintjs-0.0.1
Generating autoloads for flycheck-quicklintjs.el...
Generating autoloads for flycheck-quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/flycheck-quicklintjs-0.0.1/flycheck-quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/flycheck-quicklintjs-0.0.1/flycheck-quicklintjs-autoloads.el
Unable to activate package ‘flycheck-quicklintjs’.
Required package ‘quicklintjs-0.0.1’ is unavailable
[34/226] Generating eglot-quicklintjs-0.0.1
Generating autoloads for eglot-quicklintjs.el...
Generating autoloads for eglot-quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/eglot-quicklintjs-0.0.1/eglot-quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/eglot-quicklintjs-0.0.1/eglot-quicklintjs-autoloads.el
Unable to activate package ‘eglot-quicklintjs’.
Required package ‘quicklintjs-0.0.1’ is unavailable
[35/226] Generating flymake-quicklintjs-0.0.1
Generating autoloads for flymake-quicklintjs.el...
Generating autoloads for flymake-quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/flymake-quicklintjs-0.0.1/flymake-quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/flymake-quicklintjs-0.0.1/flymake-quicklintjs-autoloads.el
Unable to activate package ‘flymake-quicklintjs’.
Required package ‘quicklintjs-0.0.1’ is unavailable
[36/226] Generating lsp-quicklintjs-0.0.1
Generating autoloads for lsp-quicklintjs.el...
Generating autoloads for lsp-quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/lsp-quicklintjs-0.0.1/lsp-quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/lsp-quicklintjs-0.0.1/lsp-quicklintjs-autoloads.el
Unable to activate package ‘lsp-quicklintjs’.
Required package ‘quicklintjs-0.0.1’ is unavailable

Comment on lines 4 to 8
if (QUICK_LINT_JS_EMACS)
set(QUICK_LINT_JS_EMACS "${QUICK_LINT_JS_EMACS}")
else ()
find_program(QUICK_LINT_JS_EMACS "emacs")
endif ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

find_program already checks if the program was found. No need to check yourself:

Suggested change
if (QUICK_LINT_JS_EMACS)
set(QUICK_LINT_JS_EMACS "${QUICK_LINT_JS_EMACS}")
else ()
find_program(QUICK_LINT_JS_EMACS "emacs")
endif ()
find_program(QUICK_LINT_JS_EMACS "emacs")

endif ()

if (NOT QUICK_LINT_JS_EMACS)
message(WARNING "Emacs not found. Skipping... ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think this message is noisy. I'd omit it.

Suggested change
message(WARNING "Emacs not found. Skipping... ")

endif ()

set(QUICK_LINT_JS_EMACS_FOUND TRUE)
message(STATUS "Found Emacs: (${QUICK_LINT_JS_EMACS}) suitable version ${EMACS_VERSION} minimum required is 24.5")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Match format of similar messages in CMake:

Suggested change
message(STATUS "Found Emacs: (${QUICK_LINT_JS_EMACS}) suitable version ${EMACS_VERSION} minimum required is 24.5")
message(STATUS "Found Emacs: ${QUICK_LINT_JS_EMACS} (found suitable version \"${EMACS_VERSION}\", minimum required is \"24.5\"")

set(QUICK_LINT_JS_EMACS_FOUND TRUE)
message(STATUS "Found Emacs: (${QUICK_LINT_JS_EMACS}) suitable version ${EMACS_VERSION} minimum required is 24.5")

macro(emacs_pkg_target NAME FILE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I'd put this macro in plugin/emacs/CMakeLists.txt instead of here. This macro is tightly coupled to quicklintjs-pkg.el, so I think the two belong next to each other.

${QUICK_LINT_JS_EMACS}
-Q -batch
-l package --eval "(package-initialize)"
--eval "(add-to-list 'package-directory-list \"${CMAKE_CACHEFILE_DIR}\")"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add CMAKE_CACHEFILE_DIR? What files are in the build dir that we need to expose to Emacs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's the output directory for the package, otherwise Emacs would make it somewhere on its defaults, (probably ~/.emacs.d/elpa), i used the CMAKE_CACHEFILE_DIR because i assume cmake calls ${QUICK_LINT_JS_EMACS} with current directory being the same from it was invoked from, so the root of build directory would have been filled with emacs package folders instead of ${BUILD_DIR}/plugins/emacs. I cloud've made this from emacs by appending plugins/emacs to cwd, or you have something else in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's the output directory for the package

Oh. It looked like an input directory to me.

${CMAKE_CURRENT_BINARY_DIR} might be better.

I cloud've made this from emacs by appending plugins/emacs to cwd, or you have something else in mind?

I think specifying cwd explicitly is what we should do:

  add_custom_command(
    OUTPUT ${_OUTPUT}
    COMMAND
      ${QUICK_LINT_JS_EMACS}
      -Q -batch
      -l package --eval "(package-initialize)"
      --eval "(add-to-list 'package-directory-list .)"
      ...
    WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
    ...
  )

FILES flymake-quicklintjs.el
DESTINATION "${CMAKE_INSTALL_DATAROOTDIR}/emacs/site-lisp"
)
find_package(QuickLintJsEmacs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Spelling:

Suggested change
find_package(QuickLintJsEmacs)
find_package(QuickLintJSEmacs)

@strager
Copy link
Collaborator

strager commented Aug 13, 2021

Also, I'm still unsure about their need, i've written them because i'm unsure if emacs generated files are forward and backward compatible, so i left that to the host building the package decide (that's what happens in elpa/melpa, for example). But i have a feeling we could generate and track those files beforehand.

I think a build-time step (as you wrote) makes sense.

@strager
Copy link
Collaborator

strager commented Aug 13, 2021

316e270 fixes #395 by adding a wrapper command which marks the default-directory as the root folder, unfortunately i think we can't transparently fix this, my first attempt was to use lsp-mode provided callback lsp-before-initialize-hook, but this is called after the prompt.

Aww.

I don't know if this is a good workaround. Would an lsp-mode user know to use this separate function? (I.e. would they blindly copy-paste from our documentation?)

Do other lsp-mode plugins have the same problem? If not, how do they solve it?

@wgrr
Copy link
Contributor Author

wgrr commented Aug 13, 2021

Do other lsp-mode plugins have the same problem? If not, how do they solve it?

I just tried their builtin plugins for lsp-eslint, lsp-jsts and lsp-go, all of them prompt for a root direcotry.

The relavant piece which decides to prompt or not is here:
https://github.com/emacs-lsp/lsp-mode/blob/5b2daf6/lsp-mode.el#L8101-L8106

The only path I see is to make lsp-find-session-folder return non-nil by inserting the folder on its state file beforehand, I think instead we should just suggest to set lsp-auto-guess-root

To avoid confusion, *-pkg.el files has a special meaning for Emacs packages.

Signed-off-by: wagner riffel <[email protected]>
@strager
Copy link
Collaborator

strager commented Sep 28, 2021

@wgrr Are you waiting for feedback from me? The PR is in a draft state still.

@strager
Copy link
Collaborator

strager commented Nov 3, 2021

ping @wgrr

@wgrr
Copy link
Contributor Author

wgrr commented Nov 3, 2021

ping @wgrr

sorry, I've missed your previous comment in notifications, no, I went to a vacation and now I've been procrastinating on finishing this, I'll pick it up this weekend, sorry about the lack of input here.

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.

Emacs Flymake: diagnostics not cleared sometimes Emacs: automatically require plugins?
2 participants