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

Error: NoTargetDir when running in a workspace with rust-toolchain 1.65.0 version #188

Closed
Veetaha opened this issue Jul 17, 2023 · 9 comments · Fixed by #204
Closed

Error: NoTargetDir when running in a workspace with rust-toolchain 1.65.0 version #188

Veetaha opened this issue Jul 17, 2023 · 9 comments · Fixed by #204
Labels
A-marker-cargo Area: All things connected to `cargo_marker` C-bug Category: Something isn't working
Milestone

Comments

@Veetaha
Copy link
Contributor

Veetaha commented Jul 17, 2023

The error is simply

$ cargo marker
Error: NoTargetDir

Our cargo workspace is sitting on an older version of rust toolchain, although we are planning to upgrade soon, but the error is not informative, it doesn't show what actually went wrong. Only by inspecting the code of cargo-marker and experimenting I figured out the rust version was causing this.

To reproduce this you may create a crago workspace with rust-toolchain file with 1.65.0 in it.

The version of cargo-marker is 0.1.1 and marker_lints is 0.1.1 as well

@xFrednet
Copy link
Member

Thank you for the issue! The error handling in cargo_marker really has to be improved. I believe that one comes from the fact that it can't find the target directory. It expects it to be right next to the Cargo.toml file. It's supposed to be a sanity check to prevent markers from creating unwanted filed. However, currently it doesn't take into account that the target directory can be changed for cargo. (Put that on the ToDo list ^^)

Could you try creating a directory called target next to the Cargo.toml file?

The rust-toolchain file should not affect marker. The driver needs a specific nightly version, but Marker tires to hide this. cargo marker can be called with any version, and should only internally switch to the nightly version to compile the lints and run the driver. To the project, it should look like any other cargo command. This solution is not perfect, but the best I could come up with in the meantime.

@xFrednet xFrednet added C-bug Category: Something isn't working A-marker-cargo Area: All things connected to `cargo_marker` labels Jul 17, 2023
@xFrednet xFrednet added this to the v0.2.0 milestone Jul 17, 2023
@Veetaha
Copy link
Contributor Author

Veetaha commented Jul 18, 2023

@xFrednet the target directory exists. My setup is no different from any other cargo workspace in this regard.
You may check for yourself, but running the following commands:

$ cargo new foo
     Created binary (application) `foo` package
$ cd foo
$ echo 1.65.0 > rust-toolchain
$ cargo build
   Compiling foo v0.1.0 (/home/veetaha/junk/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 0.35s
$ cargo marker --lints 'marker_lints = "0.1.1"'
Error: NoTargetDir

@xFrednet
Copy link
Member

xFrednet commented Jul 18, 2023

Interesting, I'll investigate this. Thnak you :)

@NathanFrasier
Copy link
Contributor

NathanFrasier commented Jul 25, 2023

FWIW: I was looking into using marker for a large app at work, and ran into the same issue, I made a minimal clone of the marker_lints crate and removed all of the testing code, and running cargo marker creates the same error. Our rust toolchain is 1.58.1 at the moment. I changed to 1.71.0 and re-ran cargo marker and it linted correctly.

@xFrednet
Copy link
Member

Thank you for the feedback!

This will be fixed in the next version, planned for the next rust release. I plan to check for a target directory next to the Cargo.toml file and also add a configuration for it.

@NathanFrasier
Copy link
Contributor

NathanFrasier commented Jul 27, 2023

Doing a little digging and some good old fashioned dbg!() statement troubleshooting. It looks like this is due to the cargo metadata command invoking rustc with --print=split-debuginfo which lines up because I vaguely remember that only being stabilized recently. @xFrednet Am I wildly off base in saying the path forward is trying to get cargo-metadata to not pass that print flag? It seems like this wouldn't be a case where we need to bump the MSRV.

@xFrednet
Copy link
Member

Am I wildly off base in saying the path forward is trying to get cargo-metadata to not pass that print flag?

What path forwarding do you mean in this case?

If you're referring to the cargo_path used when CargoMetadata is invoked, that path is mostly set to consistently use the same version of cargo for commands. Due to the way rustc's internals are wired, Marker sadly needs to run with nightly.

If you're asking why cargo uses --print=split-debuginfo internally, I'm not sure. I would guess that it already has infrastructure to read the split debug info, and it's easier to just reuse that code than to parse the console output of rustc.


The goal of the function, that throws the error, is to find the target directory. I used CargoMetadata because it seemed like the easiest approach. cargo-marker should have a low MSRV to allow all projects to use it. (This should be tested in CI, but that's a story for another time ^^)

I can think of the following solutions to investigate:

  1. Try to figure out, if the called version of rustc can be overridden, it should work just fine if we instruct cargo to also use the nightly rustc version to fetch the debug info. (Try setting RUSTUP_TOOLCHAIN variable in MetadataCommand::env)
  2. Manually invoke cargo metadata with marker*::Toolchain::cargo_command() and use cargo_metadata to parse the output.
  3. Determine the target directory differently (Maybe check for a ./target folder)
  4. Allow the user to manually specify the target directory.

I believe 1 and 2 would be the best solutions if they work. We might also want to investigate if cargo-marker has other faulty calls to MetadataCommand which were just hidden by this error. I would also like option 4 to be implemented at one point, but that can very well be new issue :)


Btw. since you started digging though Marker, I've started writing some contributor documentation: #199 Not sure if it contains any additional information for you, since you already got the project running, but it might still be useful. Thank you for helping with this issue!

@NathanFrasier
Copy link
Contributor

NathanFrasier commented Jul 27, 2023

What path forwarding do you mean in this case?

Ah, I'm sorry. I need to be more aware of my expressions, not everyone online uses my local jargon.
I could have been clearer with "Am I correct in assuming that the way to resolve this bug is..." You have answered that question despite my poor phrasing.

I'll be looking into option 1 first, (or maybe 2 if that's just not possible) as it seems extremely likely to me that there are other places this inconsistency has occurred and that the find target dir is just the one that happened to crop up first.

It also seems like since cargo-metadata is just a wrapper over rustc, we really want everything to line up version wise, otherwise it seems extremely likely that we'll find a similar version issue in the future.

@xFrednet
Copy link
Member

Ah, I'm sorry. I need to be more aware of my expressions, not everyone online uses my local jargon.

"The path forward" is a common phrase in English, so please continue using it, also on GH. My mind was just so focussed on file paths, that it didn't consider the sentence as a whole ^^

This bug is now fixed with your PR. I've created a new issue #205 for the fourth option, to allow users to specify the target directory manually :)

@bors bors bot closed this as completed in e8d0c70 Jul 28, 2023
bors bot added a commit that referenced this issue Aug 18, 2023
217: Fixed toolchain version drift, improved error handling, config loading and refactoring. r=xFrednet a=nitkach

### This PR solves several problems

## Problem 1

After resolving this issue #188, this problem was revealed.
I didn't create an issue, I just fixed it in this PR.

When `cargo` of the new version tries to call `rustc` of the old version and pass it arguments that it does not support, the program breaks.
This happens when executing a command in `cargo_metadata_command()`.

Description of the situation in the diagram:

![toolchain-fiasco](https://github.com/nitkach/marker/assets/108859698/65be44f1-19f7-4881-8c1c-d8294db74c39)

Inside `MetadataCommand::new()` creates `cargo metadata` which calls `rustc`.
It, in turn, takes the toolchain from `PATH="~/.rustup/toolchains/1.65.0-*"`

## Problem 2
Also in repository with 1.65.0 in rust-toolchain file the following error occurs:
```
$ cargo marker

Compiling Lints:
error: the `-Z` flag is only accepted on the nightly channel of Cargo, but this is the `stable` channel
See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.
Error: LintCrateBuildFail
```
<!-- Reproduces if there is no `cargo_marker` in the current `Cargo.toml` dependencies.
The `PATH` additionally specifies the path to `cargo-marker`. -->

Diagram of command calls and environment variables:

![toolchain-unstable](https://github.com/nitkach/marker/assets/108859698/aeec07b2-c6a9-44c9-a0b6-46c8f0260f82)

The command `cargo build ... -Z unstable-options ...` uses a stable toolchain, despite the specified `RUSTUP_TOOLCHAIN=nightly-2023-07-03`.

Then `rustc` is called, the stable toolchain is taken from `PATH` and a command execution error occurs.
Cannot use the arguments available on the nightly version of `cargo` in the stable channel.

---

To fix this, I changed the `cargo_command()` of `Toolchain`.
Now the
```
rustup run {toolchain} cargo
```
is being created.
This makes `rustup` a proxy for `cargo`, guaranteed to call `cargo` with the specified `toolchain`.

---

### Cargo workspace manifest location

Now the location of the workspace manifest file is obtained by executing the `cargo_locate_project` method on the `Cargo` type.
The `rustup run {toolchain} cargo locate-project --workspace` command is executed.

---

#### Error handling
I have plan to refactor error handling in future PR.
I have removed the error codes from `ExitStatus`, as they do not work as I assume. In the terminal, it always shows that `(exit code: 1)` is returned.

https://github.com/nitkach/marker/blob/b95435d77065210bbe38e6995f35bd165f280612/cargo-marker/src/exit.rs#L81-L83
Adding the `Fatal` variant with a description and the source of the error makes creating errors easier.

---

#### Parsing TOML as strongly typed data structures
For parsing `toml`, I added the use of rust types, which deserializes the document into a structure with fields.

---

#### `Display` error for toml file while parsing

I also noticed that it is possible to output errors in more readable way

`Debug`:
```
Can't parse config: Error { inner: Error { inner: TomlError { message: "data did not match any variant of untagged enum LintDependency", original: Some("[workspace]\nmembers = [\n  \"cargo-marker\",\n  \"marker_adapter\",\n  \"marker_api\",\n  \"marker_rustc_driver\",\n  \"marker_utils\",\n  \"marker_lints\",\n  \"marker_uitest\",\n  \"marker_uilints\",\n]\nresolver = \"2\"\n\n[workspace.metadata.marker.lints]\nmarker_lints = { foo = \"./marker_lints\" }\n"), keys: ["workspace", "metadata", "marker", "lints", "marker_lints"], span: Some(245..271) } } }
```
`Display`:
```
Can't parse config: TOML parse error at line 15, column 16
   |
15 | marker_lints = { foo = "./marker_lints" }
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum LintDependency
```

---

#### Some code changes

I'm learning how to use iterators. Where the `mut` variable and the `for` loop were used, I replaced that with iterators.
In some places, an immutable `Vec<T>` was created, which I changed to an `[T]`.


Co-authored-by: Nikita <nikita_tkachev_2001@mail.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-marker-cargo Area: All things connected to `cargo_marker` C-bug Category: Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants