Skip to content

Neomake - workspaces are broken #259

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

Closed
dpc opened this issue Sep 7, 2018 · 15 comments
Closed

Neomake - workspaces are broken #259

dpc opened this issue Sep 7, 2018 · 15 comments

Comments

@dpc
Copy link

dpc commented Sep 7, 2018

I'm opening this issue just to get some visibility of people that are both Rust user and VimL-versed.

neomake/neomake#1916

Let's say you have project a with a/b subcrate.

If you cd a/b and vim, neomake stops working well, because rustc still displays error paths as b/src/lib.rs (relative to workspace root) while neomake/Vim would like them to be relative to current dir, so just src/lib/rs.

All it takes to fix (I think) is fixing the path somewhere here: https://github.com/neomake/neomake/blob/master/autoload/neomake/makers/ft/rust.vim#L136 by stripping the prefix.

But it's currently beyond me, how to do it right, and I don't have time to dedicate to figure it out.

Please, someone take a look. :)

@dpc
Copy link
Author

dpc commented Sep 7, 2018

The same problem happens eg. in VSCode BTW.

@blueyed
Copy link

blueyed commented Sep 9, 2018

As for Neomake, it should probably cd to the project root?
Currently it cds to the parent dir of the buffer (https://github.com/neomake/neomake/blob/28f6991f3546195e764052d5e1c731432ac8f706/autoload/neomake/makers/ft/rust.vim#L28), which also means that it should not make a difference if you cd yourself (this is for the cargo maker, but not rustc for example!). Please consider filing an issue in Neomake as a followup if e.g. this is not working with rustc for you, but with cargo.

@dpc
Copy link
Author

dpc commented Sep 9, 2018

@blueyed I'd like to help, but I am so confused that I don't know how to answer any part of your question.

As for Neomake, it should probably cd to the project root?

The problem is that with Rust workspaces the is "root workspace dir" and bunch of "root crate dirs". inside it. When I use rust.vim + neomake in a typical project that has one crate (and thus workspace root == crate root) I can cd anywhere in the project, and :ll jumps to error just fine.

When I start a workspace things work only if the current dir is the workspace root.

Not sure if it's any help.

dpc added a commit to dpc/rust.vim that referenced this issue Sep 9, 2018
All credit to @blueyed .

This fixes rust-lang#260

The investigation is in the corresponding Neomake issue:
neomake/neomake#2080

This is also related to: rust-lang#259
@blueyed
Copy link

blueyed commented Sep 9, 2018

Does it also affect cargo, or only rustc? (the former does cd in Neomake, the latter does not)
You can check :echom getloclist(0) to see the parsed location list.
It looks like an issue in Neomake's makers, which need to be adjusted probably, but we need to figure out which ones are affected.
I think cd'ing to %:p:h is not the best probably (i.e. it might not match the maker's output).
Would be good to get this analyzed per maker (i.e. use :Neomake rustc, and/or :Neomake cargo alone, but you appear to be only using cargo, aren't you?).

@dpc
Copy link
Author

dpc commented Sep 9, 2018

Does it also affect cargo, or only rustc

My naive understanding is that rustc maker would work only for standalone .rs files, which is very, very rarely how people do things in Rust. I've just tried and the error suggests that the whole thing doesn't compile since it's not aware that the file is a part of a bigger rust crate. In that case, there can be no workspaces (because they are cargo concept, AFAIK). So I guess rustc can be ignored.

If it is of any help, there is a topic about how to detect workspace dir: rust-lang/cargo#4933 . I'm also happy to assist as a guinea pig.

It looks like it's just a matter of calling cargo metadata and looking up:

"workspace_root":"/home/dpc/lab/crev"

key in the JSON output.

dpc added a commit to dpc/rust.vim that referenced this issue Sep 9, 2018
All credit to `@blueyed`.

This fixes rust-lang#260

The investigation is in the corresponding Neomake issue:
neomake/neomake#2080

This is also related to: rust-lang#259
dpc added a commit to dpc/rust.vim that referenced this issue Sep 9, 2018
All credit to `@blueyed`.

This fixes rust-lang#260

The investigation is in the corresponding Neomake issue:
neomake/neomake#2080

This is also related to: rust-lang#259
@blueyed
Copy link

blueyed commented Sep 9, 2018

@dpc
Isn't the workspace root the same as where Cargo.toml is? (this is what Neomake's cargo maker uses (bonidjukic/pinax-stripe@80e618e))

We could certainly change this to use the metadata, although that's an extra call etc (but looking for the Cargo.toml is similar already).
It would be better if cargo had an option to use absolute paths, or would make them relative to cwd after all. Apparently it was changed in rust-lang/cargo#4788.

@dpc
Copy link
Author

dpc commented Sep 9, 2018

@blueyed

> find . | grep Cargo
./Cargo.lock
./crev-data/Cargo.toml
./crev-lib/Cargo.toml
./crev-bin/Cargo.toml
./crev-common/Cargo.toml
./Cargo.toml

Both crates and workspaces itself have a Cargo.toml. cargo metadata is a static, Rust binary, so it should be as fast any other system command being called.

@blueyed
Copy link

blueyed commented Sep 9, 2018

@dpc
I see. Giving this a try.
I assume cargo metadata --no-deps --format-version 1 should be run in the buffers directory for best effects?

@dpc
Copy link
Author

dpc commented Sep 9, 2018

@blueyed I don't know the asnwer, but I expect cargo metadata should give the same results across the whole workspace.

da-x added a commit that referenced this issue Sep 9, 2018
Similarly to what other reformatters plugins do, we should use a
combination of setline() and line deletion command to carefully
replace the lines of the buffer(). There's issue #236 that seems
to not regress in this fix - the problem with '1,$d _' is that it
deleted the entire buffer. We should only delete for line numbers
that beyond the newly applied buffer content.

This has a side effect of handling #260 and #259, as expand('%')
is not relied upon.
blueyed added a commit to neomake/neomake that referenced this issue Sep 9, 2018
* rust: cargo: use cargo's metadata for workspace_root

Ref: rust-lang/rust.vim#259 (comment)

* Factor out neomake#utils#temp_cd
@da-x
Copy link
Member

da-x commented Sep 9, 2018

Yes, I also bumped in the past with the "Paths are workspace-relative rather crate-relative if a workspace exists regardless of where you run cargo build" issue.

The parsing of cargo metadata --no-deps --format-version 1 is clever. Originally the builtin 'Cbuild' has had this issue too, so it used a rather simplistic parsing method in the cargo#nearestWorkspaceCargo() function which does the work of walking up the directory tree to find the Cargo.toml that marks a workspace if one exists.

I wonder how ALE is handling this, because it seems to be handling workspaces correctly, and it uses neither methods.

@da-x
Copy link
Member

da-x commented Sep 10, 2018

Yes, however this code does not take into account whether there's an additional upper Cargo.toml which sets a workspace. Looking more into this, I figured out why it works - ALE looks into the JSON output for each warning and error, where it find absolute paths, and it appears to be matching them with the absolute paths of open buffers. So under these conditions, relative paths and current directory settings don't matter.

@blueyed
Copy link

blueyed commented Sep 10, 2018

ALE looks into the JSON output for each warning and error, where it find absolute paths

So there are absolute paths in there?
I've taken a closer look and there is src_path (we use file_name). This might be something newer.

However grepping for src_path in ALE's source didn't find something.
It appears to be only matched accidentally (it compares the tail): https://github.com/blueyed/ale/blob/f516cac47c244566db4f9b421974bdeb1310510f/autoload/ale/path.vim#L152

okhin pushed a commit to okhin/neomake that referenced this issue Sep 11, 2018
* rust: cargo: use cargo's metadata for workspace_root

Ref: rust-lang/rust.vim#259 (comment)

* Factor out neomake#utils#temp_cd
@da-x
Copy link
Member

da-x commented Oct 14, 2018

@dpc is this issue still relevant? Thanks

@dpc
Copy link
Author

dpc commented Oct 14, 2018

I've been working on my workspace enabled project for a while now, and I forgot the issue still exists, so I guess everything was fixed.

@dpc dpc closed this as completed Oct 14, 2018
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

No branches or pull requests

3 participants