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

Use cargo-metadata command to learn about workspace root #55

Closed
wants to merge 1 commit into from

Conversation

matklad
Copy link

@matklad matklad commented Feb 13, 2018

Hi! recently Cargo changed the way it reports errors (they are now realative to the workspace root), and this commits fixes cargo.el to support it.

The idea is to ask Cargo itself about where's the workspace root, and fallback to old Cargo.toml discovery, if that fails.

@Dushistov
Copy link
Contributor

Great, at now with rustc 1.24.0-beta.12 (ed2c0f084 2018-02-12) all features of cargo.el have started work again.

@Dushistov
Copy link
Contributor

@matklad

After some further testing looks there is issue with this PR.
With this patch cargo-process-check and all other commands run
againts workspace, not current project.
And this is break build. Because of some of my workspace crates require different --target=.

For example consider such structure:

a
b
ios
android

before this PR, I can run cargo-process-check against a and b without problems.
For ios and android (that depends on a and b) I need to run cargo check --target=X.

But at now I can not run cargo-process-check for any of those crates.

@davemilter
Copy link

@Dushistov @matklad

There is a much simpler example. See attachment.
With current beta and this patch you can not run cargo-process-check inside
crate without errors, if another crate in workspace depends on this crate and you breaks API.

So before that PR and rustc 1.24 you can do some refactoring inside one crate,
run cargo-process-check and when all ready starts adopt all it's dependencies inside workspace,
but now this ability is broken.
See example workspace in attachment:

1.zip

But anyway, thanks @matklad for your efforts to fix #51

@matklad
Copy link
Author

matklad commented Feb 13, 2018

So, it seems to me that we should be aware of two directories here:

  1. package-root -- the directory with Cargo.toml for the current package.
  2. worksapce-root -- the directory with current workspace.

I believe you can get the directories using the following functions:

(defun cargo-process--package-root ()
  "Find the root of the current Cargo package."
  (let* ((root (locate-dominating-file (or buffer-file-name default-directory) "Cargo.toml")))
    (and root (file-truename root))))

(defun cargo-process--workspace-root ()
  "Find the worksapce root using `cargo metadata`."
  (let* ((metadata-text (shell-command-to-string
			 "cargo metadata --format-version 1 --no-deps"))
	 (metadata-json (json-read-from-string metadata-text))
	 (workspace-root (alist-get 'workspace_root metadata-json))
	 (fallback (cargo-process--package-root))
	 (root (or workspace-root fallback)))
    (and root (file-truename root))))

Next, we should run cargo using package-dir as CWD, and process errors using workspace-dir. The problem is, I don't quite understand how exactly the two directories are set in Emacs :)

I believe deafult-directory here is for process cwd:

(default-directory (or project-root default-directory)))

But where's the dir for error links?

@matklad
Copy link
Author

matklad commented Feb 13, 2018

FWIW, the correct way to handle compiling a crate would be to look at cargo metada for workspace-members key and pass a --package argument to cargo, but looking for Cargo.toml from CWD could be a good approximation.

@Dushistov
Copy link
Contributor

@matklad

But where's the dir for error links?

I belive default directory (cwd of compilation command) is root directory for errors links.
And this is really hard to modify because of compilation-mode is part of emacs.

@matklad
Copy link
Author

matklad commented Feb 13, 2018

Hm, interesting!

I think one possible solution here is always launch cargo inside workspace-dir, but pass it an additional --manifest-path $package_dir/Cargo.toml argument.

Dushistov added a commit to Dushistov/cargo.el that referenced this pull request Feb 22, 2018
This change is based on kwrooijen#55
It uses cargo-process--custom-path-to-bin instead of hard coded
"cargo", plus it uses --manifest-path to run "cargo" against only one crate.
@kwrooijen
Copy link
Owner

This should be fixed with #56

@kwrooijen kwrooijen closed this May 21, 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

Successfully merging this pull request may close these issues.

4 participants