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

refactor(cli): Use cargo metadata to detect the workspace root and target directory, closes #4632, #4928. #4932

Merged
merged 5 commits into from
Aug 21, 2022

Conversation

FabianLars
Copy link
Member

  1. I don't know how this will influence custom runners.
  2. Do we want to keep some of the old logic as a fall back? I don't really see much value in that, unless they help for 1), though we should probably fix that more explicitly then.
  3. There's a TODO here, mainly because i forgot to remove that before committing, but should this function return an Error if cargo metadata fails? If not i think we should return the PathBuf directly instead of a Result.
  4. Although it started as a fix for the target dir, i changed the logic for the workspace root too, should this be reverted? I can't think of a situation where cargo metadata could fail where our custom logic wouldn't.

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No, hopefully.

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

@FabianLars FabianLars marked this pull request as ready for review August 14, 2022 20:48
@FabianLars FabianLars requested a review from a team as a code owner August 14, 2022 20:48
@FabianLars
Copy link
Member Author

Didn't notice that i opened it as a draft. Anywayyy not ready to merge (there is a TODO comment), but ready for review :)

@lucasfernog
Copy link
Member

Might be a bit too crazy but if we see any issues with cargo metadata in the future, we could parse the cargo output directly with the JSON message format: https://github.com/vadimcn/vscode-lldb/blob/bc36db0bafd1b77d1cc9aa2dc9d69b221c7d4ae8/extension/cargo.ts#L137

@FabianLars
Copy link
Member Author

FabianLars commented Aug 21, 2022

Let's hope we won't see any issues then 😂

@FabianLars
Copy link
Member Author

ah whoops, it doesn't compile yet, sorryy

@FabianLars
Copy link
Member Author

So it does compile locally and the error in the workflow logs doesn't make any sense to me. Command is imported and the error is reported on a completely wrong/unrelated line

@FabianLars
Copy link
Member Author

ahhh i guess it's because of the new interface/rust/desktop.rs file - Why doesn't github scream at me because of merge conflicts or something :(

@lucasfernog
Copy link
Member

It does some magic stuff merging with dev when testing lol

@lucasfernog lucasfernog merged commit fea70ef into dev Aug 21, 2022
@lucasfernog lucasfernog deleted the refactor/cargo-metadata branch August 21, 2022 13:42
@lucasfernog
Copy link
Member

Thanks for implementing it :) this is super helpful for the mobile implementation too, since cargo-mobile sets a target in .cargo/config.

@lucasfernog
Copy link
Member

Actually I need to make a small change in the target option to fix that :D will work on that later

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.

2 participants