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

Don't require IDF_TOOLS_PATH location to be writable when idf.py is executed (IDFGH-7791) #9329

Closed
igrr opened this issue Jul 11, 2022 · 9 comments
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@igrr
Copy link
Member

igrr commented Jul 11, 2022

Environment

  • IDF version: master branch, but probably applies to release/v4.x as well
  • Build System: idf.py
  • Operating System: Linux

Problem Description

As found in #9328 (comment), if the tools have been installed by one user to a system-wide location, "regular" user can't successfully build the project using those tools.

Expected Behavior

Build succeeds. If some features (like idf_env.json) aren't available because IDF_TOOLS_PATH is not writeable, they are skipped.

Actual Behavior

Build fails because IDF_TOOLS_PATH is not writeable.

Steps to reproduce

See the discussion in the linked issue.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Jul 11, 2022
@github-actions github-actions bot changed the title Don't require IDF_TOOLS_PATH location to be writable when idf.py is executed Don't require IDF_TOOLS_PATH location to be writable when idf.py is executed (IDFGH-7791) Jul 11, 2022
@stkw0
Copy link

stkw0 commented Jul 18, 2022

I made IDF_TOOLS_PATH writable to test if once this issue is solved it will work. Sadly, after fixing this it will still cause troubles, it requires a python virtual environment to be created even when it's not needed because deps are already installed system-wide. It fails with:
ERROR: $IDF_TOOLS_PATH//python_env/idf5.0_py3.9_env/bin/python doesn't exist! Please run the install script or "idf_tools.py install-python-env" in order to create it

I don't know if I should create a new issue related to this

@igrr
Copy link
Member Author

igrr commented Jul 18, 2022

I think this part of the behavior is expected, install.sh will indeed create the virtual environment under IDF_TOOLS_PATH as mentioned in #9328 (comment).

Since IDF depends on quite a few python packages (and specific versions of them, as well), installing them into the global system environment is not realistic — not without potentially causing compatibility issues with other software in the system. Even if there are no conflicts right now, they might occur in the future. We would like to avoid having bug reports opened against ESP-IDF which could be avoided by having an isolated Python environment.

Could you please explain why you would like to avoid having a virtual environment (even the one inside IDF_TOOLS_PATH)? As far as I know there is no significant overhead in that, compared to global installation. python interpreter in the venv is just a link to the actual binary installed globally.

@stkw0
Copy link

stkw0 commented Jul 18, 2022

The tools that I used to develop our software with 4.x versions required the tools to be installed system-wide. This allowed different IDEs to be well integrated with IDF by default. With version 5.0 the previous behavior is broken and so is all the tools and the workflow in general.

Moreover I think that, in Linux, the standard way is to leave the package manager in control of the dependencies of the software. The decision of using or not a virtual environment, in my opinion, should be made by the user and the software should not block the installation of a package in the standard way. I understand the issue and that the recommended approach may be to use a virtual environment, I'm totally happy with that, but I still want to preserve the ability to install it from the distribution package manager. I also may want to use the dependencies from the package manager (possibly with security patches or other patches) rather than the pip packages (in this case I understand that it will make sense for you to not give support to issues that didn't used the virtual environment approach)

@igrr
Copy link
Member Author

igrr commented Jul 18, 2022

This allowed different IDEs to be well integrated with IDF by default. With version 5.0 the previous behavior is broken and so is all the tools and the workflow in general.

I think you can probably add the virtual environment bin subdirectory path to the PATH variable (via env.d), then the IDEs should be able to use the interpreter from the virtual environment.

Moreover I think that, in Linux, the standard way is to leave the package manager in control of the dependencies of the software. The decision of using or not a virtual environment, in my opinion, should be made by the user and the software should not block the installation of a package in the standard way.

Thanks for the explanation, I understand your argument. I am not sure I agree with it. (i.e. i don't consider apt to be the standard way of installation of Python packages, just as apt is not the standard way to install Java or Rust packages...)
However, I'll keep the issue open while we discuss whether we can accommodate your request without increasing the cost of supporting related bug reports.

in this case I understand that it will make sense for you to not give support to issues that didn't used the virtual environment approach

The problem with that is that the issue will still initially get reported as a bug, and we will need to spend time to investigate it before we find that the root cause. We don't often get complete information about the environment in the issue report, even for the things that are mentioned in the issue template.

@stkw0
Copy link

stkw0 commented Jul 18, 2022

I strongly believe that yes, distro package manager are the standard way. If language package managers are mixed with distros package manager there are all sorts of security issues, compatibility issues and so. In short I think that distro package managers are the standard way for users to install software (e.g: to install firefox I use {apt,yum,whatever} not a language specific package manager). Language package managers are useful for developers. For instance if I were a IDF developer I will use a virtual environment for developing IDF and then will make it easier for maintainers to package my software for the different distributions, that is, indeed, how almost all linux software works.

I think that to make it work it won't be enought to add bin directory to PATH as the other directories where the libraries are present should be also on the PATHs. Even if it works, I may get exposed to vulnerabilities. If my distribution release a security fix for python-socketio for example and I am using the one inside the virtual environment I may think that I'm safe when the reality is that I'm not because a out-of-date version is running inside the virtual environment. Which means I need to keep track manually of the issues that may be released for every package that is not tracked by my distribution package manager. Is not a very nice solution from a user perspective.

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Aug 1, 2022
@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable labels Sep 19, 2022
@someburner
Copy link
Contributor

@igrr

Can you confirm that the above commit results in idf.py not trying to download espidf.constraints.v5.0.txt into the IDF_TOOLS_PATH? I'm using release/v5.0 branch and not sure if that has made that in there yet.

I don't see why a constraints file should be downloaded and/or checked when doing export.sh? Shouldn't that be done just once during install? In fact I don't see why packages should be checked for being up-to-date at all. Or are the required packages and/or versions project-dependent?

For people building on a local machine these things are fine but this is not desirable behavior on a CI machine for those of us not using a docker-like workflow. The toolchain should be installed once and stay the same forever, and updated manually if needed, including python libraries.

Maybe this is a matter of opinion but I don't like the idea of pulling from external sources as part of the build procedure every time. Not that I am auditing these packages but seems like a security risk.

@igrr
Copy link
Member Author

igrr commented Sep 27, 2022

@someburner no, this commit doesn't address downloading and checking the constraint file. It only allows having a read-only tools directory.

We are going to change the behavior of export.sh to not fetch the new constraints file if IDF commit hasn't changed, and the install script has been run for this commit successfully once.

@someburner
Copy link
Contributor

@igrr that sounds good to me, thanks for the info!

@tpambor
Copy link
Contributor

tpambor commented Nov 30, 2022

@igrr This bug still seems be present in v5.0 branch. Using the espressif/idf:release-v5.0 docker image together with a non-root user I get:

developer@7e231e32deab:/workspace$ source /opt/esp/idf/export.sh 
Detecting the Python interpreter
Checking "python3" ...
Python 3.8.10
"python3" has been detected
Checking Python compatibility
Checking other ESP-IDF version.
ERROR: File /opt/esp/idf-env.json is not accessible to write. 

Probably this should be backported from master to v5.0 branch.

espressif-bot pushed a commit that referenced this issue Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

5 participants