esp-rs / espup

Tool for installing and maintaining Espressif Rust ecosystem.
Apache License 2.0
224 stars 23 forks source link

espup should clean up old path entries automatically #330

Open bugadani opened 1 year ago

bugadani commented 1 year ago

Installing a new toolchain (even just a pre-release) leaves old path entries behind. This means that long-term use of espup will eventually run into #270.

While this is related to #270 I consider this a separate issue. #270 can be hit first the time if the user starts with a particularly big path variable, but repeated use should not result in considerable, linear growth of the length of $Env:PATH.

I should note that it's not sufficient for espup update to do the cleanup, as pre-releases need to be installed with espup install.

SergioGasquez commented 1 year ago

Do you have any suggestions on how to solve this issue? The main concern that I have when cleaning the path is: How do I know that espup added that entry, and it's not there because the user also uses esp-idf?

I should note that it's not sufficient for espup update to do the cleanup, as pre-releases need to be installed with espup install.

Pre-releases should also be installed with espup update

bugadani commented 1 year ago

Pre-releases should also be installed with espup update

The announcement was not at all clear about this.

https://matrix.to/#/!LdaNPfUfvefOLewEIM:matrix.org/$ZAzqDb185Cpjil1OOwdcAoSxLkm8E0rDUj1d_gPwGd8?via=matrix.org&via=tchncs.de&via=mozilla.org

Do you have any suggestions on how to solve this issue?

My best idea is to write a file to the installation folder that lists what was added (and that doesn't list duplicates/skipped entries), so espup could read, when removing that version, what it needs to delete. If you don't need to add a path because of esp-idf, don't list it in that file. This should be okay since espup only supports a single installed toolchain version, so there's no "race condition" when things are installed/uninstalled out of order.

I guess it's not perfect because users can manually mess everything up, but it's up to them to do so.

SergioGasquez commented 1 year ago

This should be okay since espup only supports a single installed toolchain version, so there's no "race condition" when things are installed/uninstalled out of order.

espup supports several installed toolchain versions, using the -a, --name parameter.

The main problem with the file approach is: Say that an esp-idf user wants to try Rust, before using espup he already has the path set for esp-idf, then he runs espup, uses the Xtensa Rust toolchain, but he wants to uninstall Rust stuff, then espup might delete the initial esp-idf path.

The environment variables are quite complex to handle at the moment, but if we manage to use LLD as our linker (https://github.com/esp-rs/esp-hal/pull/357), which is a WIP, all this would become much easier as we only need to set the LIBCLANG_PATH environment variable.

bugadani commented 1 year ago

but he wants to uninstall Rust stuff, then espup might delete the initial esp-idf path

I might not have been clear with this, but I probably wouldn't store the env vars that are already added by esp-idf:

and that doesn't list duplicates/skipped entries

But since --name breaks my idea, nevermind :)

SergioGasquez commented 1 year ago

I will try to gather some information about the status of LLD support, if we are close to using it, we can leave this issue as unresolved as in the near future it won't apply. Otherwise, let's try to find a good way to solve this!