-
Notifications
You must be signed in to change notification settings - Fork 26
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 espup #182
Refactor espup #182
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow that's a lot. Haven't spotted any obvious things when looking at the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Few important things to note, once we merge it and release a new version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :)
This is a bit problematic, given that any deployments using the current version of the toolchain will start failing once this is published. |
Will it pick-up the new version automatically? |
If they are pinned to |
I think for the time being maybe just adding a deprecation warning and having it not do anything as @bjoernQ mentioned is fine. I just don't want to keep breaking CI and forcing users (and us) to update over and over, you know? |
Forget what I've said about Sorry for the noise. Still, tomorrow I would tag the release as pre-release and use a fork of the action to verify that its working properly. |
As discussed in #154 we are going for a minimal approach, hence I refactored what we have, and here is the resulting behavior:
espup install
installs under$RUSTUP_HOME/toolchain/<toolchain_name>
:espup update
will update the Xtensa rust under$RUSTUP_HOME/toolchain/<toolchain_name>
:latest
if no-v/xtensa_version
is providedespup uninstall
will delete$RUSTUP_HOME/toolchain/<toolchain_name>
Other changes:
-s/--std
) that avoids installing GCC (it will be installed byesp-idf-sys
)esp
) by using the-a/name
option-x/--llvm-version
option as we only supported one version.-e/--extended-llvm
cd.yml
now also publishes to crates.io