Add current working directory [:cwd] property

[s3bs]

This is a first iteration of trying to implement #312. It give the possibility to add :cwd to the flycheck-define-checker. :cwd can be a fixed path or a function that returns a absolute path. The function is called just before the checker. The default-directory is only changed in the lexical environment of the checker execution.
In addition there is a :environment property, where one can define additional environment variables.
ie ("var1=value1" "var2=value2")

Edit: Connects to #312

[swsnr]

@s3bs Thank you. Unfortunately that's only about a third of what's actually needed. Particularly we need to “remember” the working directory and use it later when we expand relative filenames from the syntax checker output. Otherwise Flycheck will fail to parse relative filenames in the syntax checker output.

Please do remove the :environment property from this pull request, by amending your commits and force-pushing to your branch. It's a different and unrelated feature that needs to be added independently after discussion (for I'm not convinced that we need it).

[swsnr]

See also flycheck/flycheck-haskell#50 (comment). Inviting @mrkkrp

[mrkkrp]

@s3bs, Thank you for working on this. In fact, I also started to work on the issue, but I don't have anything to commit yet. I hope this PR will evolve to the point when it will be merged. Would be really great.

[s3bs]

a) My use case for the :environment is that I don't start emacs from the command line, so it doesn't have the env variables set as you would have by a login shell. I need those variables to set the path for a license file. ... I will removed this from the pull request

b) as for the implementation of the functionality for :cwd I would propose to attach a buffer to the process as it is done in async.el. With that we can pass the information of 'default-directory easily. I seen a comment in the code referencing #298 about side effects attaching a process to a buffer. From what I can see is that the current-buffer was used to be attached to the process and this caused problems. What would stop us from using a complete new buffer that will be hidden in the background with 'generate-new-buffer. Let me know if I missed something in regards to the side effects #298.

@mrkkrp. Great that you are working on this as well. Lets try to not duplicate the effort here and code it twice.

[swsnr]

@s3bs We track the source buffer in a process property already, but we definitely can't change the default-directory of the source buffer. That's a no-go, for it's global side-effects alone. We have to track our syntax checking working directory separately. And I'm definitely not in favour of just let binding default-directory around the whole error parsing code. I'd rather take this opportunity to get rid of this particular piece of global state in Flycheck, and explicitly pass the directory when we need it (e.g. to expand-file-name).

And since I'd like to avoid attaching ever more properties to the process I proposed the idea of a syntax checking context that keeps track of all of these properties in a single cl-defstruct.

---

As for 1., take a look at https://github.com/purcell/exec-path-from-shell which lets you copy environment variables from your shell. But even if you don't want to use this package, I don't see why you could just call setenv for the variable in your Emacs configuration.

Thank you for removing it from this pull request.

[s3bs]

@lunaryorn I was not talking about the default-directory of the source-buffer, hence why I referenced async.el as code example. They use 'generate-new-buffer, attach that new buffer to the process. I was proposing to set this new buffer's default-directory.

I'll have a look into cl-defstruct.

As for the environment variables, you should probably open a new issue. I'm not using set-env because I only want to temporarily modify my environment variables and not clutter the environment, which could potentially cause problems with other things.

[swsnr]

@s3bs I'm not sure whether I understand you: Why do we need a fresh buffer to track the working directory? Why can't we just attach the directory to a process property?

[swsnr]

@s3bs As for environment variables, please do open a new issue, but I must admit that I'm very sceptical whether it's worth adding to Flycheck. In your issue, please spend some effort on providing a proper motivation for this feature. Ideally, demonstrate a concrete use case that is impossible to implement properly without this feature.

Thank you 😊

[s3bs]

@lunaryorn I must have misunderstood you. I was not quite sure with what you meant with " I'd rather take this opportunity to get rid of this particular piece of global state in Flycheck, and explicitly pass the directory when we need it "

Inviting @rejeep to this discussion as he started #312

[swsnr]

@s3bs I'm sorry, I was apparently not clear in my comments.

I'm less concerned with how we pass the working directory of the checker along with the process: We can just attach it to the process as a property.

However, after the process is finished we have to use this directory to expand file names at some places. And instead of changing default-directory here, I'd prefer if we could explicitly give it as function argument where needed.

[s3bs]

@lunaryorn I think I'm a bit lost in your code. One part that confuses me is flycheck-error-filename. It is probably some sort of closure, but I just don't get it at the moment how that one works. Loosing my elisp foo. :(

[swsnr]

@s3bs flycheck-error-filename is just an accessor for the filename slot of the flycheck-error struct—search the code for cl-defstruct to find the definition (unfortunately struct definitions are not indexed in the imenu) and read more about Structures in the manual :) Likewise there's flycheck-error-line for the line slot, or flycheck-error-level for the level slot, etc.

Please don't hesitate to ask further questions.

[s3bs]

@lunaryorn As I see it we need the information in which directory the checker is started (cwd), everywhere were a error struct is passed, since all filenames in errors can be relative to that calling path. Currently all this happens under a with-current-buffer and therefore we have the default-directory of the buffer that the syntax-checker is called from.
I would propose the following solution:

1. when starting the process that executes the syntax checker, we create a buffer local variable in a format like flycheck-<checkername>-context, that contains the cwd. This way its a unique identifier and supports multiple checker in one buffer. Since all the error structs have the checker name and the buffer reference in them, we can easily access this information when the error filenames are processed. Once all the errors are process the buffer local variable is deleted.

[swsnr]

Alternatively we can require that all paths in flycheck-error be absolute, and expand relative names right after parsing. Then we'd only need the directory once.

[s3bs]

I don't have a testcase were the checker produces relative file names in the error messages. Can someone proved such testcase.
@lunaryorn Is there a way to run travis-ci from the local repository before pushing it to github?

[swsnr]

setf sets the value of a “generalized variable”. Essentially it's just a consistent syntax to set many different types of variables, include process sentinels. If you expand the macro you'll find that it actually expands to (set-process-sentinel process …).

Whether you use generalized variables or not is ultimately a matter of taste; personally I prefer the consistency it gives.

If this answers your question, please remove this comment again 😊

[s3bs]

thanks for beeing so detailed in your answers.

[swsnr]

@s3bs Great work, this patch looks really good now! 👍 There are a couple of minor issues on the diff, which I'd like you to address, but over all it's really a good implementation that I'll definitely merge! Thank you very very much 👍 😍

As for Travis CI, you can run the test suite locally with:

$ cask install
$ test/run.el '(not (tag external-tool))'

This runs all tests that do not need specific syntax checker tools. It's perfectly fine to rely on Travis CI, though 😃

We also have a Vagrant based VM that includes all syntax checker tools for a complete test, but that's too much for this change. We'll run the complete tests before merging anyway, there's no need for you to go through the hassle of setting it up ☺️

[aaronjensen]

btw @s3bs the lambda is never called. I put a message in there and it is never invoked.

[s3bs]

@aaronjensen I just tried that and it worked fine. Have you re-evaluated flycheck-define-checker? Can we take this to the flycheck gitter chat as I want to avoid cluttering this pull request with messages of debugging this issue.

[aaronjensen]

Looks like this is actually a problem w/ using flycheck-define-checker inside with-eval-after-load. I dont' understand it, but naming the lambda works around it, I don't think this is a problem w/ flycheck or this branch.

[swsnr]

@aaronjensen Ah, yes, that is a known issue, see #594. I didn't consider it too important, but it appears to be a nuisance, so I'll try to fix it soon.

[aaronjensen]

@lunaryorn ah, good to know. #emacs helped me figure out it was a macro expansion problem, I still don't quite understand the issue, but yes, a fix would be great, especially since configuring in with-eval-after-load is probably pretty common.

By the way, do you have a suggestion for @s3bs on my first comment:

file-name in the :error-patterns is still relative to the file's directory whereas I would expect it to be relative to the new default-directory.

[swsnr]

@aaronjensen No, unfortunately not. I've still not reviewed and tested this patch. Albeit being small, it's complex in its interactions with the rest of Flycheck, which makes testing somewhat time-consuming, and I simply didn't have the time to do that.

[aaronjensen]

Got it, well let me know if there's more that I can do to help. I think that the :error-patterns issue should probably be a blocker.

[purcell]

Anything I can do to help here? The change looks clean, the discussion here suggests it's close to being ready, and I see a wide variety of people excited about this change.

[cpitclaudel]

No need to put the close-parens ) on a separate line here

[cpitclaudel]

I've added a few stylistic comments to the patch, which otherwise looks very nice (great job @s3bs!). @aaronjensen raised a concern with :error-patterns, but I'm not sure I fully understand it.

[aaronjensen]

@aaronjensen raised a concern with :error-patterns, but I'm not sure I fully understand it.

The issue is that the path for a file generally is relative to the default-directory, but flycheck, with this patch, does not understand that. In other words, say you have a project like:

.
├── acronym
│   ├── README.md
│   ├── acronym.exs
│   └── acronym_test.exs
├── anagram
    ├── README.md
    ├── anagram.exs
    └── anagram_test.exs

If the default-directory for acronym.exs gets set to whatever . is, then the error tool may report errors as having occurred in acronym/acronym.exs. However, flycheck will expect file-name to be acronym.exs and it will not associate errors from acronym/acronym.exs with the appropriate file.

Here is an example checker:

(flycheck-define-checker elixir-dialyzer
    "Erlang syntax checker based on dialyzer."
    :command ("mix" "dialyzer")
    :default-directory (lambda () (locate-dominating-file default-directory "mix.exs"))
    :predicate
    (lambda ()
      (and
       (buffer-file-name)
       (file-exists-p "mix.exs")
       (file-exists-p "deps/dialyxir")
       (file-exists-p ".local.plt")))
    :error-patterns
    ((error line-start
            (file-name)
            ":"
            line
            ":"
            (message)
            line-end))
    :modes elixir-mode)

and some sample output:

dialyzer --no_check_plt --plt .local.plt -Wno_opaque -Wno_return -Wunderspecs -Wunknown -Wrace_conditions --fullpath /Users/aaronjensen/Source/curadora/booking/_build/dev/lib/booking/ebin
  Compiling some key modules to native code... done in 0m0.26s
  Proceeding with analysis...
web/partner/request/hotel_details.ex:167: The call erlang:'+'(3,x@1::'atom') will never return since it differs in the 2nd argument from the success typing arguments: (number(),number())
web/partner/request/hotel_details.ex:171: The call 'Elixir.Juniper.Request.HotelDetails':foo('atom') will never return since it differs in the 1st argument from the success typing arguments: (number())

If my default-directory is manually set via setq to the directory containing the web directory, this works fine. However, if it is set with this new feature, it does not. It expects the file-name to be only hotel_details.ex

I hope this clears things up some, thanks!

[swsnr]

@aaronjensen The problem is that flycheck-fill-and-expand-error-file-names in flycheck-finish-current-syntax-check isn't called with the proper working directory, but as things stand this is somewhat hard to add because Flycheck doesn't know about the working directory anymore at this point.

@purcell I'm sorry for everyone that's excited about this change, but I'm sure they'd be much less excited about a broken Flycheck.

This change interacts with many parts of Flycheck, and what's worse, using the wrong directory for relative file names could just make Flycheck silently throw away valid errors for the current buffer, which is obviously a no-go.

That's why I want to test this change myself to make sure that it really works as intended. I haven't had the time for this yet.

And then there's the issue discovered by @aaronjensen which @s3bs would need to fix first. I don't have the time to do that myself.

[swsnr]

Closing in favour of #973. Thanks @sergv