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

fix: run choosenim update stable before downloading nim #279

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ganderzz
Copy link

Resolves #264

Run choosenim update stable before downloading and installing nim.
This prevents a local stable pointing to an old version of nim.`

Run `choosenim update stable` before downloading and installing nim.
This prevents a local `stable` pointing to an old version of nim.`
Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

In general the right idea, but see my comments. Thank you for working on this!

Comment on lines 67 to 69
# Update any existing local stable version to the latest nim stable version.
"$temp_prefix/$filename" update stable

Copy link
Owner

Choose a reason for hiding this comment

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

This can prompt, so I think it should go in the if branches below.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, you can override the chosen channel as you can see below, you cannot assume stable.

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha. Updated. Not sure I totally got the first comment, so let me know and I'll get things updated!

@dom96
Copy link
Owner

dom96 commented Nov 4, 2021

So I think you need it in both branches (one with -y and one without). I'd merge this but this needs testing, can you test it with choosenim not installed + choosenim installed and include the output you get? :)

@ganderzz
Copy link
Author

@dom96 Updated. :)

With choosenim installed
Screen Shot 2021-11-09 at 6 51 52 PM

Without

Screen Shot 2021-11-09 at 6 54 20 PM

Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Sorry, but thinking about this some more I think this logic should be implemented inside choosenim. i.e. if choosenim stable is ran with the --firstInstall flag then run update implicitly. That way it's easier to keep this logic sound.

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.

Possible issue detecting older choosenim install
2 participants