search

ethereum / emacs-solidity

The official solidity-mode for EMACS
GNU General Public License v3.0
205 stars 66 forks source link

fixes flycheck integration #28

Closed favadi closed 6 years ago

favadi commented 6 years ago
LefterisJP commented 6 years ago

Hello @favadi! Thank you for taking the time to open a PR.

By default value of solidity-solc-path is "solc", therefore (file-executable-p solidity-solium-path) always return nil unless solidity-solium-path is set to absolute path. This is fixed by replace file-executable-p with flycheck-executable-find.

I guess you mean (file-executable-p solidity-solc-path). Yes that is true, you need to set the absolute path. But I tried what you suggest in my system and ran (funcall flycheck-executable-find "solc") but it returns nil, even though solc is in the $PATH.

The flycheck checkers configurations codes are not loaded when (require solidity-mode), moved them to a function to explicitly call in config.

I prefer lazy configuration. I don't want to have to call a specific flycheck-setup function in order to integrate with flycheck. Plus if we did that this would break everyone's existing configuration.

favadi commented 6 years ago

But I tried what you suggest in my system and ran (funcall flycheck-executable-find "solc") but it returns nil, even though solc is in the $PATH

It is pretty weird flycheck-executable-find is defined in https://github.com/flycheck/flycheck/blob/master/flycheck.el and supposed to work. I tested in both OS X and Ubuntu.

screen shot 2018-04-24 at 3 37 14 pm

I prefer lazy configuration. I don't want to have to call a specific flycheck-setup function in order to integrate with flycheck. Plus if we did that this would break everyone's existing configuration.

That's true, I should revert it.

favadi commented 6 years ago

  1. For the flycheck-executable-find I couldn't figured out why it didn't work for you. The only explanation left is you are using Emacs on OS X and the PATH inside Emacs is different with the one in your shell (https://emacs.stackexchange.com/questions/10722/emacs-and-command-line-path-disagreements-on-osx).

  2. If I remove the defun, somehow the checkers are not registered anymore. Look like they didn't even get executed when require. I even tried just put a (message "debug-abc") to solidity-flycheck.el file but didn't see anything in Messages buffer. Do you have any idea why it behaves this way?

LefterisJP commented 6 years ago

For the flycheck-executable-find I couldn't figured out why it didn't work for you.

Yes that's it (but I use Archlinux, not OSX). It can find python but not solc. I compile solc myself and add it to ~/.local/bin/ but the emacs path does not know that as emacs is started before my shell.

I run emacs as a system service which is automatically started when the system boots, so getting the user specific path setting and other environment variables does not work. That's why I prefer the explicit way of setting the path and only if the path is not set would I allow a fallback to the flycheck-executable-find.

If I remove the defun, somehow the checkers are not registered anymore.

Hmm .. some kind of caching issue? Make sure to delete all *.elc compiled files.

favadi commented 6 years ago

That's why I prefer the explicit way of setting the path and only if the path is not set would I allow a fallback to the flycheck-executable-find.

flycheck-executable-find works with absolute path too, no need to set a fall back.

Hmm .. some kind of caching issue? Make sure to delete all *.elc compiled files.

I deleted all .elc already, so it's something else. Could you help testing by put a (message "something") to solidity-flycheck.el, restart Emacs to see if something prints out on Messages buffer?

LefterisJP commented 6 years ago

flycheck-executable-find works with absolute path too, no need to set a fall back.

You are correct! So disregard that comments.

Could you help testing by put a (message "something") to solidity-flycheck.el, restart Emacs to see if something prints out on Messages buffer?

It does. But most importantly looking at flyckeck-checkers both solc and solium are in there, every time I restart emacs.

favadi commented 6 years ago

@LefterisJP I updated the PR with just the change to use flycheck-executable-find.

LefterisJP commented 6 years ago

@favadi Thank you very much @favadi. Can you also add an entry to the changelog before I merge?

favadi commented 6 years ago

@LefterisJP I did that in https://github.com/ethereum/emacs-solidity/pull/28/commits/c62cc12dd3ffcca43b1d5de423bad6b1bd1f3d59.

LefterisJP commented 6 years ago

Thankl you @favadi for the time you spent on this and for your contribution!