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.

Sign up for GitHub

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 RiscV installation/uninstallation #107

Merged
merged 27 commits into from
Dec 23, 2022
Merged

Fix RiscV installation/uninstallation #107

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0ae42f5
fix: 🐛 Fix typo
SergioGasquez Dec 22, 2022
1f8a04e
fix: 🐛 Fix riscv uninstallation
SergioGasquez Dec 22, 2022
e2c550f
fix: 🐛 Fix riscv uninstall logs
SergioGasquez Dec 22, 2022
caef1e1
chore: 🎨 Update RISC-V logs to make them consistent
SergioGasquez Dec 22, 2022
70049af
feat: ✨ Use both `imc` and `imac` target for RISC-V targets
SergioGasquez Dec 22, 2022
d63b549
refactor: ♻️ Initial async
SergioGasquez Dec 15, 2022
2ae93f8
refactor: ♻️ Use struct to store RiscV Target info so we can impl Ins…
SergioGasquez Dec 20, 2022
4717922
refactor: ⚡️ Use RiscVTarget constructor
SergioGasquez Dec 20, 2022
c9fbf03
chore: ✨ Initial attemp to paralelize installations
SergioGasquez Dec 20, 2022
8b2fcf9
feat: ✨ Spawn a task for every install
SergioGasquez Dec 21, 2022
b15b7bb
feat: ✨ Await only the len of to_install
SergioGasquez Dec 21, 2022
039d70e
fix: 🐛 Add llvm to to_install
SergioGasquez Dec 21, 2022
ec65811
feat: ✨ Paralelize gcc installation
SergioGasquez Dec 21, 2022
a301fad
fix: 🐛 Fix receiving
SergioGasquez Dec 22, 2022
b0b338e
fix: 🐛 Remove TODO
SergioGasquez Dec 22, 2022
26986b5
fix: 🐛 Remove stopwatch
SergioGasquez Dec 22, 2022
5a322c6
fix: 🐛 Fix Windows build
SergioGasquez Dec 22, 2022
6d16b3f
fix: 🐛 Fix riscv uninstallation
SergioGasquez Dec 22, 2022
d472f26
chore: 🎨 Update RISC-V logs to make them consistent
SergioGasquez Dec 22, 2022
da77129
fix: 🐛 Fix duplicated import
SergioGasquez Dec 23, 2022
adef0ba
feat: ✨ Use both `imc` and `imac` target for RISC-V targets
SergioGasquez Dec 23, 2022
2d837b8
chore: 🎨 Update RISC-V logs to make them consistent
SergioGasquez Dec 23, 2022
e1fc141
Merge branch 'main' into fix/riscv-uninstall
SergioGasquez Dec 23, 2022
3e729ed
fix: 🐛 Export variables even when reusing GCC installation
SergioGasquez Dec 23, 2022
212a384
fix: 🐛 Return result instead of unwrapping
SergioGasquez Dec 23, 2022
25cd4f2
fix: 🐛 Only install `riscv32-esp-elf` once
SergioGasquez Dec 23, 2022
c9306bd
fix: 🐛 Fix clippy
SergioGasquez Dec 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ pub enum Error {
WrongWindowsArguments,
#[diagnostic(code(espup::failed_to_remove_directory))]
#[error(
"{} Failed to remove '{0}' direcretory. Please, manually verify that the directory is properly removed and run 'espup uninstall' again.",
"{} Failed to remove '{0}' directory. Please, manually verify that the directory is properly removed and run 'espup uninstall' again.",
emoji::ERROR
)]
FailedToRemoveDirectory(String),
Expand Down
19 changes: 17 additions & 2 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ use espup::{
gcc::{get_toolchain_name, install_gcc_targets},
llvm::Llvm,
rust::{
check_rust_installation, install_extra_crates, install_riscv_target, Crate, XtensaRust,
check_rust_installation, install_extra_crates, install_riscv_target,
uninstall_riscv_target, Crate, XtensaRust,
},
},
update::check_for_update,
Expand Down Expand Up @@ -189,7 +190,7 @@ fn install(args: InstallOpts) -> Result<()> {

exports.extend(llvm.install()?);

if targets.contains(&Target::ESP32C3) {
if targets.contains(&Target::ESP32C3) || targets.contains(&Target::ESP32C2) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a nicer way of handling this (which could be done in a separate PR) would be to implement some helpers on Target to determine the architecture. As we continue to add support for addition devices we're going to keep needing to chain expressions with || here.

Maybe if we do something like this:

impl Target {
    pub fn riscv(&self) -> bool {
        !self.xtensa()
    }

    pub fn xtensa(&self) -> bool {
        matches!(self, Target::Esp32 | Target::Esp32s2 | Target::Esp32s3)
    }
}

We can then change this line (and the others like it) to something like this:

if targets.iter().any(|t| t.riscv()) {
    //
}

Just a thought, doesn't really matter right now but will likely clean things up in the future. If you don't like this idea then that is fine of course too 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the idea! It would definitely things scale better! I will address this on a different PR though. I also plan to install RISC-V GCC toolchain (riscv32-esp-elf) for ESP32-S2 and ESP32-S3 for the ULP coprocessor.

install_riscv_target(&args.nightly_version)?;
}

Expand Down Expand Up @@ -273,6 +274,11 @@ fn uninstall(args: UninstallOpts) -> Result<()> {
.map_err(|_| Error::FailedToRemoveDirectory(llvm_path.display().to_string()))?;
}

if config.targets.contains(&Target::ESP32C3) || config.targets.contains(&Target::ESP32C2) {
info!("{} Deleting RISC-V target", emoji::WRENCH);
uninstall_riscv_target(&config.nightly_version)?;
}

if let Some(esp_idf_version) = config.esp_idf_version {
info!("{} Deleting ESP-IDF {}", emoji::WRENCH, esp_idf_version);
config.esp_idf_version = None;
Expand All @@ -293,6 +299,15 @@ fn uninstall(args: UninstallOpts) -> Result<()> {
})?;
} else {
info!("{} Deleting GCC targets", emoji::WRENCH);
if config.targets.contains(&Target::ESP32C3) || config.targets.contains(&Target::ESP32C2) {
config.targets.remove(&Target::ESP32C3);
config.targets.remove(&Target::ESP32C2);
config.save()?;
// All RiscV targets use the same GCC toolchain
let riscv_gcc_path = get_tool_path(&get_toolchain_name(&Target::ESP32C3));
remove_dir_all(&riscv_gcc_path)
.map_err(|_| Error::FailedToRemoveDirectory(riscv_gcc_path))?;
}
for target in &config.targets.clone() {
config.targets.remove(target);
config.save()?;
Expand Down
16 changes: 16 additions & 0 deletions src/toolchain/rust.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,22 @@ fn install_rust_nightly(version: &str) -> Result<()> {
Ok(())
}

/// Unnstalls the RiscV target.
pub fn uninstall_riscv_target(nightly_version: &str) -> Result<()> {
info!("{} Installing Riscv target", emoji::WRENCH);
SergioGasquez marked this conversation as resolved.
Show resolved Hide resolved
cmd!(
"rustup",
"target",
"remove",
"--toolchain",
nightly_version,
"riscv32imac-unknown-none-elf"
SergioGasquez marked this conversation as resolved.
Show resolved Hide resolved
)
.run()
.into_diagnostic()?;
Ok(())
}

#[cfg(test)]
mod tests {
use crate::toolchain::rust::{Crate, XtensaRust};
Expand Down