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

Clippy codespaces fix #1780

Closed
wants to merge 3 commits into from
Closed

Conversation

matthri
Copy link
Contributor

@matthri matthri commented Nov 18, 2023

Closes #1776

I apologise in advance for the long text, but as far as I can tell there are some things going on and the panic of the clippy exercise is just a symptom.

Current status: When creating a Codespace or running a DevContainer the Rustlings repository is automatically cloned in the workspace and after that the install script is run.
The install script is then cloning the repository again which leads to a second Rustlings repository inside the already existing repository.
The Rustlings executable is build and installed by the install script from the nested (/rustlings/rustlings) repository which is set to the current release tag.
The outer Rustlings repository can be set to another version which is by default the current main version, which is what causes the problems in this case.

In main the path to the Clippy exercise got updated but since then no new release was tagged. This leads to the rustlings executable (which is build based on the "older" release) to expect a different path than the version present in the outer rustlings repository.

Since in my opinion doing a second clone of an already existing repository in a subdirectory is kind of unnecessary and confusing for the user I have added a flag to the install.sh script which prevents the clone and does the Rustlings install using the already present repository.

The updated DevContainer definition with this flag prevents the panic problem but while implementing this I realized another problem where I am not sure what's the best way forward.

Assuming someone created their own copy of the Rustlings repository (e.g. by forking it), so they can track their progress in their own repository, then in this version there are no tags present.
This leads to the install script failing since we can't checkout the latest release tag.

I think there are three options to solve this and I am not sure which is the preferred way:

  1. Automatically pull the tags from rust-lang/rustlings and use them to checkout the latest release.
    (easy for the user but I am not sure if it is nice to automatically mess with the repo of the user)
  2. Fail the script run and prompt the user to pull the tags
    (a bit inconvenient but at least no automatic modification)
  3. (this one is currently implemented) Use main as fallback for the installation if no tags are found

And last but not least I have updated the updateContent and postAttach commands in the DevContainer definition to set the cargo paths since otherwise the first creation of the container is failing because of cargo not being found.

Thank you very much if you have read this far ^^, I would appreciate feedback on the above considerations :D

@shadows-withal
Copy link
Member

Thanks for the PR! For the options you present here, I think going the "middle way" of having the user fetch tags manually is fine, as long as we output a reasonable detailed help text telling the user what to do. With those changes, I think this should be mergeable!

@matthri
Copy link
Contributor Author

matthri commented Dec 22, 2023

I updated the setup to use the "middle way" and abort with a hopefully helpful error message.

I didn't update the "can't fetch latest tag from releases and also no valid local tags present" case mainly for two reasons:

  1. Don't change the current behavior when not urgently needed
  2. The most likely cause of "can't fetch latest tag from releases" are some issue's internet connection issues. In that case the suggestion to fetch the tags from the remote repository is not helpful since this will also fail.
    So in my opinion here the best option is the current "fallback to main" behavior.

@@ -158,7 +188,16 @@ then
Version="tags/${Version}"
fi
else
Version="tags/${Version}"
if [[ -e ".git/refs/tags/${Version}" ]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this'll always fail because it's not in the Rustlings directory at this time (for normal invocations of the script)

@mo8it
Copy link
Contributor

mo8it commented May 12, 2024

Closing because this is not relevant in v6

@mo8it mo8it closed this May 12, 2024
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.

Clippy exercises panic in codespaces
3 participants