Fixed toolchain version drift, improved error handling, config loading and refactoring. #217
Conversation
xFrednet
added
C-enhancement
There was a problem hiding this comment.
Hey @nitkach, thank you for the PR and welcome to the project!
Could you explain how you tested this PR? I have a lab project, with a toolchain.toml file setting the channel to 1.65.0 (like explained in your images) and cargo marker passes just fine without the drift (or at least without any errors).
Then a general note, with the last PR touching this whole toolchain thing, I contemplated if it would be better to invoke cargo marker again with the right toolchain. I have the feeling it would make the code a bit cleaner, but also add a layer of complexity. This might also not help with the version drift, since the old toolchain version would still be in the PATH variable. I thought it was still worth mentioning, since you seem to have a good understanding of the toolchain magic now ^^
Please reach out, if anything is unclear or you would like some support :)
I am really surprised to see the difference in behavior of rustup between unix and windows here, because this bug doesn't reproduce for me on my Linux devbox as well, we assumed it did, and didn't verify it on linux, but it is windows-specific, in fact. The difference is that the rustup proxy prepends the toolchain directory to If you log the Here is a cross-platform nushell script. You can run it with # Replace this with your path to marker git repo
let marker_repo = "D:/dev/marker"
let sandbox_dir = "D:/junk/sandbox"
# Add master build of marker to Path
$env.Path = (
$env.Path
| split row (char esep)
| prepend $"($marker_repo)/target/debug"
)
cd $marker_repo
cargo build
rm -rf $sandbox_dir
mkdir $sandbox_dir
cd $sandbox_dir
cargo new version-drift-repro
cd version-drift-repro
"1.65.0" | save rust-toolchain
$"[workspace.metadata.marker.lints]
marker_lints = { path = '0.1.1' }" | save --append Cargo.toml
cargo marker setup --auto-install-toolchain
cargo marker
There are a couple of problems caused by this. First of all the DetailsThis is because on windows If you get past by that, install the toolchain in a separate directory elsewhere where there is no DetailsGiven all of that, I adviced @nitkach to just not specify any As for the metadata command, I think it is reasonable to use whatever the toolchain is installed in the user's workspace to read the metadata. We also don't want to accidentally autoinstall the Under that undescriptive |
|
Thank you for the explanation and support @Veetaha
That explains the difference. Handling all OS specific cases in At this point, I feel like you have a better understanding than me, when it comes to |
I think today my specialty is "unraveling convoluted bugs in distributed systems", where |
|
Hey @nitkach and @Veetaha, I'm planning the v0.2.0 release for Thursday next week. Do you know if you'll be able to address the last comments until then? I think there are only two open ones:
Once they're addressed and the branch has been rebased (and some commits squashed), we can merge this as well. #214 Will most likely be merged today, so it might be better to wait with the rebase until then. It's totally fine if you don't have the time. Also, please reach out if I can assist in any way! :) |
There was a problem hiding this comment.
This version looks good to me, thank you for the update!
Before this is merged, would you mind squashing your commits? You don't have to reduce it to a single one, but having descriptive commit messages would awesome. Marker usually also uses a Rebase Workflow meaning that PRs should generally not include merge commits.
Resolving merge commits in the middle of a commit history can sadly be tricky. I would suggest just squashing them with the rest of your commits into one. At least that's the easiest way. If you require any help, please reach out!
Once that is done, everything should be good to go :)
|
Merge commit is more convenient for reviewers because it's just a new commit to the branch. This way it's possible to review only the new incremental changes since the last review. 
However, when merging into # The result of merging the PR should produce a commit which is formatted
# from the PR title and PR description.
use_squash_merge = trueI know bors is deprecated, so I don't think it makes sense to configure it to do this. It's better to transition to merge queues. So @nitkach let's squash to commits manually. |
…g and refactoring This commit contains quite a lot if changes. See them all in the PR description rust-marker#217
…g and refactoring This commit contains quite a lot if changes. See them all in the PR description rust-marker#217
|
@xFrednet forget what I said about reviewing convenience. It's not worth it. Merge commits are so hard to wrap your head around, that it's better to always use rebase-and-squash. I thought it was simple until I had to squash the commits. I rebased everything into a single commit. I am going to try my grasp at merge queues CI for this repo, and will hopefully find a way to make squashing automatic (IIRC merge queues support something like that). @xFrednet could you merge this please? |
GH sucks when it comes to displaying rebases correctly. I'm used to roughly remembering what was written beforehand, and the viewed file checkbox also stays checked, if the file hasn't been changed by the rebased.
In Clippy we tried the My semester is starting next week, but I hope to stay as active as I did, during the past few weeks of vacation. Ain't no rest for the wicket ^^ |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
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
cargoof the new version tries to callrustcof 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:
Inside
MetadataCommand::new()createscargo metadatawhich callsrustc.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:
Diagram of command calls and environment variables:
The command
cargo build ... -Z unstable-options ...uses a stable toolchain, despite the specifiedRUSTUP_TOOLCHAIN=nightly-2023-07-03.Then
rustcis called, the stable toolchain is taken fromPATHand a command execution error occurs.Cannot use the arguments available on the nightly version of
cargoin the stable channel.To fix this, I changed the
cargo_command()ofToolchain.Now the
is being created.
This makes
rustupa proxy forcargo, guaranteed to callcargowith the specifiedtoolchain.Cargo workspace manifest location
Now the location of the workspace manifest file is obtained by executing the
cargo_locate_projectmethod on theCargotype.The
rustup run {toolchain} cargo locate-project --workspacecommand 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
Fatalvariant 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.Displayerror for toml file while parsingI also noticed that it is possible to output errors in more readable way
Debug:Display:Some code changes
I'm learning how to use iterators. Where the
mutvariable and theforloop were used, I replaced that with iterators.In some places, an immutable
Vec<T>was created, which I changed to an[T].