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 remote support #1922

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Add remote support #1922

wants to merge 18 commits into from

Conversation

yfyyfy
Copy link

@yfyyfy yfyyfy commented Nov 22, 2021

Add support for remote hosts via TRAMP.

In response to #1816,
based on #1842

Enable the following checkers to work on remote files:

  • python-pycompile
  • python-pylint
  • python-flake8
  • javascript-eslint
  • scss-stylelint
  • css-stylelint

Probably further additional changes may be necessary for some (or majority) of the others checkers.

Remote support works only with Emacs >=27.1.

One function is added: file-local-name-pass-nil.
It is not prefixed with "flycheck-" as with the other functions added in #1842.

Travis CI may pass with the following modification:

  • Update ca-certificates
    Access to repository (and thus cask installation) fail.
    yfyyfy@09271d2
  • Omit bazel-mode
    Installation of bazel-mode by cask fails.
    yfyyfy@4dc94e2
  • Disable scheme-chicken test
    It prompts for "Scheme implementation" and fails in Travis CI.
    yfyyfy@9faddc0

(The above references are the last three commits of https://github.com/yfyyfy/flycheck/tree/add-remote-support-tweak-test)

@CLAassistant
Copy link

CLAassistant commented Nov 22, 2021

CLA assistant check
All committers have signed the CLA.

@yfyyfy
Copy link
Author

yfyyfy commented Nov 22, 2021

I am a newbie to this project.
Should I post this on #1842, rather than creating a new issue?
I'm sorry if I go against a rule.

@yfyyfy
Copy link
Author

yfyyfy commented Nov 22, 2021

In my local test, these modification were necessary even for master branch.

Travis CI may pass with the following modification:

Copy link

@FelipeLema FelipeLema left a comment

Choose a reason for hiding this comment

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

this looks like it will run correctly, but the user is likely to have Emacs freeze for several seconds during their use with connections with long delays ("remote hosts that are far away")

flycheck.el Outdated
This function is a wrapper for `file-local-name' function.
if `file-local-name' is available, this function invokes it only if
FILE is not nil.
If FILEL is nil or `file-local-name' is not available, returns FILE."

Choose a reason for hiding this comment

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

typo: FILEL

@gsingh93
Copy link
Contributor

gsingh93 commented Oct 7, 2022

FWIW, I cloned this locally and rebased on master (there were no conflicts), and this seems to be working well.

@yfyyfy
Copy link
Author

yfyyfy commented May 31, 2023

I'm sorry. I failed to respond to @FelipeLema's advice to fix the typo.
I checked the progress on #1842 and tried it in one of my projects.
Unfortunately, it didn't work (at least for css-stylelint).
I rebased my branch (yfyyfy:add-remote-support) on top of the latest upstream, fixed the typo and renamed some functions.

Now, I will check the difference between the two branches (both in terms of source code and behavior).
// For css-stylelint, rebased-yfyyfy:add-remote-support seems to work.

@bbatsov
Copy link
Contributor

bbatsov commented Feb 10, 2024

@yfyyfy Are you still interested in getting this to the finish line?

@gsingh93
Copy link
Contributor

gsingh93 commented Apr 8, 2024

I think this might have been fixed with the v33 release, although I'm not sure exactly what changed. Can someone else confirm it's also working for them now?

global-flycheck-mode doesn't automatically start flycheck-mode in remote buffers, but that's a seperate issue.

@gsingh93
Copy link
Contributor

gsingh93 commented Apr 8, 2024

I take that back, when I enable flycheck-mode in a remote buffer, it seems to check the buffer correctly. But after any modifications, or if I run M-x flycheck-buffer, it stops working. I just see FlyC* in my modeline instead of an error count.

I rebased this PR on master and resolved the merge conflicts (which were just dash.el changes), and it still doesn't work :(

@yfyyfy, does this still work for you?

carrete and others added 17 commits April 28, 2024 23:38
This patch does three things:

* Update `flycheck-default-executable-find` to search remote hosts when
  `default-directory` is a remote directory. See also the documentation for
  [executable-find](https://www.gnu.org/software/emacs/manual/html_node/elisp/Locating-Files.html).

* Change uses of `call-process` and `start-process` to `start-file-process`
  which is the same as these two functions, but starts the process on a remote
  host when `default-directory` is a remote directory. See also the
  documentation for
  [start-file-process](https://www.gnu.org/software/emacs/manual/html_node/elisp/Asynchronous-Processes.html).

* Passes the local part of `buffer-file-name` to the compile command,
  e.g. strips `/ssh:10.0.0.42`, since the checker is run on the remote machine
  where the file name must be local. See also the documentation for
  [file-local-name](https://www.gnu.org/software/emacs/manual/html_node/elisp/Magic-File-Names.html).
Such checkers (including python-pycompile) use a temp directory.

For executing flycheck on remote files, temp directories should be
created on remote.
Some checkers (including python-flake8 and python-pycompile) use
flycheck-parse-error-with-patterns as their error-parser.

This commit changes `flycheck-parse-error-with-patterns' behavior so
that it always returns `flycheck-error' struct with its :filename slot
in the form of local path on remote files (i.e., without
"/method:user@host:" part).
Some checkers set remote path or filename (without directory path) to
`flycheck-error' struct's :filename slot.

For example, on remote files,
python-pylint sets remote path (i.e., with "/method:user@host:" part).
python-flake8 before the previous comiit also sets remote path.
py-compile sets filename only.

In all these cases, after processing with
`flycheck-fill-and-expand-error-file-names', :filename slot of
`flycheck-error' struct was remote path.

This commit changes `flycheck-fill-and-expand-error-file-names'
behavior so that it always returns `flycheck-error' struct with its
:filename slot in the form of local path on remote files (i.e.,
without "/method:user@host:" part).
Some checkers (including python-pylint) use
`flycheck-save-buffer-to-temp' to create temp file.

For executing flycheck on remote files, their corresponding temp files
should be created on remote (therefore, they should be specified as
remote path, i.e., with "/method:user@host:" part).
On the other hand, the return value of `flycheck-save-buffer-to-temp'
should be local path (i.e., without "/method:user@host:" part) because
the value is used as an argument to checker's executable.
Flycheck uses `flycheck-locate-config-file-functions' to locate config
file. Its value is the list of functions below:
- flycheck-locate-config-file-by-path
- flycheck-locate-config-file-ancestor-directories
- flycheck-locate-config-file-home

Each locate function should use remote path (i.e., with
"/method:user@host:" part) to locate a config file.

When the return value is used as an argument to a checker's
executable, the path should be converted to local path (i.e., without
"/method:user@host:" part).

Tested with python-flake8 and python-pylint.
The original version used stdin instead of a temporary file (as with
javascript-eslint). However, behavior of eslint and stylelint is
different in terms of stdin processing.

Corresponding CLI commands for both tools are:
eslint:
eslint --format=json --stdin --stdin-filename FILENAME-TO-ASSIGN-TO-STDIN

stylelint:
stylelint --formatter json --stdin-filename FILENAME-TO-ASSIGN-TO-STDIN
or
stylelint --formatter json --stdin --stdin-filename FILENAME-TO-ASSIGN-TO-STDIN

eslint waits for stdin EOF and keeps itself alive, while stylelint
does not wait and may exit before input comes in.

When stylelint does not receive stdin,
it emits its usage message (when --stdin is not specified)
or outputs "empty source" lint message (when --stdin is specified).

For local files, the original version was able to pass input with
`flycheck-process-send-buffer' before stylelint process exited.
For remote files, stylelint process exited before
`flycheck-process-send-buffer' called.

This commit changes *-styelint commands so that they use a temporary
file (instead of stdin) as their input to avoid the above situation on
remote.
To add support for remote files, some occurrences of filenames had to
be converted to local path on remote (i.e., without
"/method:user@host:" part).
However, these filenames could be nil, while file-local-name signals
an error when its argument is nil.

This led to test failure for scss-lint and others.
(Test command: "make LANGUAGE=scss integ")

This commit adds a wrapper function of file-local-name that accepts
nil as its argument and return it as it is.
Replace all occurrences is excessive.
For example, arguments to checker executables are not expected to be
nil and thus file-local-name will suffice.
However, here, all occurrences are replaced just in case.
In `flycheck-locate-config-file-home', file-remote-p is used to
determine if the context is local or remote.
Because file-remote-p does not accept nil,
(file-remote-p (buffer-file-name))
may cause an error.

It led to unit test failure:
   FAILED  flycheck-locate-config-file/existing-file-in-home-directory
   FAILED  flycheck-locate-config-file/existing-file-in-parent-directory
   FAILED  flycheck-locate-config-file/not-existing-file

This commit adds a nil-check for buffer-file-name.
When buffer-file-name returns nil, the context is assumed to be local.
integ test for Emacs25.3 failed due to undefined functions.

Error: the following functions are not known to be defined:
file-local-name, temporary-file-directory

This commit fixes the problem by reverting to the original codes when
the functions are not available.
@yfyyfy
Copy link
Author

yfyyfy commented Apr 28, 2024

@bbatsov
I'm sorry I did't make any progress for a long time.
I'll make an effort to make more time for this.

@yfyyfy
Copy link
Author

yfyyfy commented Apr 28, 2024

@gsingh93
I'm sorry it took me so long to respond.
Which checker did you use?

I rebased the code on master (bec59ee) locally and checked if scss-stylelint and python-flake8 worked.

  • scss-stylelint seemed to work.
  • python-flake8 seemed to work, but did not respect its configuration file (.flake8).

By the way, as you might know, remote files are explicitly excluded from the targets of global-flycheck-mode (flycheck-mode-on-safe).
Please have a look at the docstring of flycheck-may-enable-mode.

The outputs of flake8 are parsed by flycheck-parse-with-patterns, which does
not take ASCII escape sequences into account.
Therefore, flake8 should return simple texts (without escape sequences).
@yfyyfy
Copy link
Author

yfyyfy commented Apr 30, 2024

I found a solution for this.

python-flake8 seemed to work, but did not respect its configuration file (.flake8).

In my case, remote flake8 returned colored output.
Adding --color=never solved this issue.

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.

None yet

6 participants