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

Add env var SMING_TOOLCHAINS to allow easier change of toolchains ins… #2894

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

slaff
Copy link
Contributor

@slaff slaff commented Oct 23, 2024

…tall path.

@slaff slaff added this to the 6.0.0 milestone Oct 23, 2024
Copy link

what-the-diff bot commented Oct 23, 2024

PR Summary

  • Improvement of export.sh script
    The export.sh script has been updated for a better organization of tools used in the project. Instead of keeping them in various places, they will now reside in one common directory referred to as SMING_TOOLCHAINS.

  • Modification of Environment Variable References
    The paths for ESP_HOME, IDF_PATH, IDF_TOOLS_PATH, and PICO_TOOLCHAIN_PATH were previously hardcoded, i.e., fixed and unchangeable. We changed it to refer to the SMING_TOOLCHAINS directory. This action centralizes all the tools under one banner making it easier to manage the various components.

  • Update in config.rst Instructions
    New instructions have been added to config.rst on how to set SMING_TOOLCHAINS for toolchain installation. This will offer guidance to future contributors about how to set up the toolchain in the proposed common directory, ensuring a streamlined process for toolchain installations.

Copy link
Contributor

@mikee47 mikee47 left a comment

Choose a reason for hiding this comment

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

I'm not convinced that adding another configuration variable is an improvement, as it adds an additional layer of complexity. Did you follow any of the discussion in #2887 (specifically #2887 (comment)) ?

@slaff slaff force-pushed the feature/alternative-install-path-linux branch from 61ba78b to 30548f4 Compare October 24, 2024 09:11
@slaff
Copy link
Contributor Author

slaff commented Oct 24, 2024

I'm not convinced that adding another configuration variable is an improvement, as it adds an additional layer of complexity.

The end user does not need to know or change anything. Only those that would love to have the toolchains installed in a folder other than /opt should set the variable before the installations and persist it. But the latter type of users should already be skilled enough to set and persist an env variable.

But first I will need to check why the CI tests on Ubuntu are failing :)

@slaff
Copy link
Contributor Author

slaff commented Oct 25, 2024

The latest commit in this PR clarified the need to set only one mandatory env var SMING_HOME. The others are not mandatory and the end-user does not need to know/care about them.

Only if the user wants to install the toolchains in a location other then /opt then she/he can set:

  • Either SMING_TOOLCHAINS for all toolchain installations to go to one main folder.
  • Or use the existing env vars to set different installation/location path for the different toolchains.

I'm not convinced that adding another configuration variable is an improvement, as it adds an additional layer of complexity.

@mikee47 the changes in the scripts are, IMHO, subtle and neither increase the complexity of the code nor put extra burden to the end user. I hope the latest documentation change is making this PR a bit easier to understand.

@mikee47
Copy link
Contributor

mikee47 commented Oct 25, 2024

@slaff I'm fine with the change, as you say it's optional and has utility.

In #2887, @berhoel raised the issue of making /opt user-writeable, and I agree that's a bad thing.
This should not be necessary were it not for the IDF, which does need to be writeable by regular users.

The Esp8266 and Rp2040 SDKs are both added as submodules to Sming. These are 11MiB and 13MiB, respectively.

The IDF is about 1.5GiB (!). Note that whilst optimising CI build times I added the tools/ci/clean-tools.py script which reduces this to about 370MiB. Doing this for a default install may be unsafe as user applications may wish to use discarded parts of the IDF.

So the IDF is only lumped in with the toolchains because of its size. In addition, syncing all its submodules takes a long time so for these reasons it's just not practical to have that pulled in as a submodule.

Whilst SMING_TOOLCHAINS allows everything to be moved somewhere else, clearly /opt remains the preferred location for the toolchains but the IDF should go somewhere else. But NOT inside the Sming repo. Doing this is easy enough by overriding the variables we care about before calling export.sh. e.g. #2887 (comment).

@slaff
Copy link
Contributor Author

slaff commented Oct 25, 2024

Whilst SMING_TOOLCHAINS allows everything to be moved somewhere else, ...

Just the toolchains.

clearly /opt remains the preferred location for the toolchains ...

I agree with you.

... but the IDF should go somewhere else. But NOT inside the Sming repo.

Absolutely. I double that too.

I'm fine with the change, as you say it's optional and has utility.

@mikee47 shall I merge the PR as is or you would like to see additional documentation changes? I mean we have documented the best practices, our installation scripts and tutorials adhere to these practices and every new user will start working with them. If there are advanced users that would like another directory for the toolchain(s) of choice then it is their own responsibility.

@mikee47
Copy link
Contributor

mikee47 commented Oct 25, 2024

@slaff This PR is fine as-is, thanks.

@slaff slaff merged commit 66a258b into SmingHub:develop Oct 25, 2024
32 checks passed
@slaff slaff deleted the feature/alternative-install-path-linux branch October 25, 2024 11:44
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.

3 participants