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

Refactor espup #182

Merged
merged 67 commits into from
Feb 21, 2023
Merged

Refactor espup #182

merged 67 commits into from
Feb 21, 2023

Conversation

SergioGasquez
Copy link
Member

@SergioGasquez SergioGasquez commented Feb 10, 2023

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>:
    • Xtensa Rust (if any Xtensa target is provided)
    • Nightly Rust (if any RISC-V target is provided)
    • Xtensa LLVM clang
    • GCC for the provided targets (to be served as linker in no_std apps)
      • This would be removed once LLD can be used
  • espup update will update the Xtensa rust under $RUSTUP_HOME/toolchain/<toolchain_name>:
    • It will use latest if no -v/xtensa_version is provided
    • If the semver of the version trying to install is the same as the version installed it won't install it but it will print a warning message (as we use an extended version of semver and they might be differences)
  • espup uninstall will delete $RUSTUP_HOME/toolchain/<toolchain_name>

Other changes:

  • Added an option (-s/--std) that avoids installing GCC (it will be installed by esp-idf-sys)
  • You can modify the Xtensa toolchain name (defaults to esp) by using the -a/name option
  • [Windows]: Environment variables are automatically injected into your system when installing and cleared when uninstalling
  • Removed the config file
  • Dependencies cleanup
  • Removed -x/--llvm-version option as we only supported one version.
  • By default, only libs or LLVM are installed, you can opt-in to install the whole llvm with -e/--extended-llvm
  • Other minor improvements like error naming, logs updated, running some commands in parallel...
  • cd.yml now also publishes to crates.io

Copy link

@bjoernQ bjoernQ left a 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

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Looks good!

@SergioGasquez
Copy link
Member Author

Few important things to note, once we merge it and release a new version:

  • We will need to update instructions on the book
  • The Xtensa toolchain action also requires an update since the --profile-minimal option is gone (we use it by default)

Copy link
Member

@MabezDev MabezDev left a 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 :)

@jessebraham
Copy link
Member

jessebraham commented Feb 20, 2023

  • The Xtensa toolchain action also requires an update since the --profile-minimal option is gone (we use it by default)

This is a bit problematic, given that any deployments using the current version of the toolchain will start failing once this is published.

@bjoernQ
Copy link

bjoernQ commented Feb 20, 2023

* The Xtensa toolchain action also requires an update since the `--profile-minimal` option is gone (we use it by default)

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?
Maybe we could re-add that option and print a warning that this is deprecated and a no-op now? I know that is ugly

@SergioGasquez
Copy link
Member Author

This is a bit problematic, given that any deployments using the current version of the toolchain will start failing once this is published.

If they are pinned to 1.5.0 it will. Shall we keep the flag even though it does nothing?

@jessebraham
Copy link
Member

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?

@SergioGasquez
Copy link
Member Author

SergioGasquez commented Feb 20, 2023

Forget what I've said about xtensa-toolchain, we are actually not using that flag, even though at some point we did but it looks like when removing the install.sh I forgot to add it, so xtensa-toolchain would work with no changes required.

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.

@SergioGasquez SergioGasquez merged commit 5040e11 into esp-rs:main Feb 21, 2023
@SergioGasquez SergioGasquez deleted the refactor/espup branch February 21, 2023 13:48
@SergioGasquez
Copy link
Member Author

SergioGasquez commented Feb 21, 2023

Merged the PR, created a pre-release, and tested the action in different repos:

Ran the CI 5 times in each repo with no issues.
I also had to merge #188 as it was causing some transient errors, as we had in the past (#137).

I'll proceed to mark it as latest release and publish it to crates.io

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.

Minify espup Revisit espup update and espup uninstall Windows: Inject environment variables
5 participants