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 make target python-requirements to install Python requirements of project #2019

Merged
merged 6 commits into from
Feb 11, 2020

Conversation

aemseemann
Copy link
Contributor

@aemseemann aemseemann commented Jan 10, 2020

This is a proposal to add a new make target to installs all python requirements of the project from 'requirements.txt' files of the Components it depends on.

Open tasks I would like to collect some feedback first:

  • CI server integration: Since a generic target that invokes python-requirements for every sample is quite slow, it's probably best to explicitly invoke it only for those samples known to have non-standard python requirements.
  • Check if more Components/Samples could make use of this mechanism. - Done: Full-text search for 'python' revealed just the terminal Component and the HttpServer_FirmwareUpload sample.

@slaff
Copy link
Contributor

slaff commented Jan 13, 2020

@aemseemann Let's split this PR. Move all stuff related to the configuration of the python interpreter into a separate PR (for example: feature/config-python-interpreter). And then rebase this PR on top of the feature/config-python-interpreter PR.

This is a proposal to add a new make target to installs all python requirements of the project from 'requirements.txt' files of the Components it depends on.

What will happen if a component has example that requires python modules, but Sming and the current project itself do not require it? Do we have to explicitly list all requirements.txt files or what would you suggest?

@mikee47
Copy link
Contributor

mikee47 commented Jan 13, 2020

Instead of explicitly invoking make python-requirements, we could arrange things so this is done automatically on first use. With submodules, we do this by touching a .submodule file so perhaps a similar scheme could work for this?

@aemseemann
Copy link
Contributor Author

@slaff: for the PYTHON config PR see #2022 .

What will happen if a component has example that requires python modules, but Sming and the current project itself do not require it? Do we have to explicitly list all requirements.txt files or what would you suggest?

(Hope I understood your question correctly) The make target is supposed to install the python requirements of the Sming project it is invoked on. The requirements.txt of all Components/libraries directly or indirectly consumed by the project are collected, either by autodetecting a requirements.txt in the Component's root dir or using the file(s) explicitly provided by the Component via COMPONENT_PYTHON_REQUIREMENTS. The latter allows the requirements to depend on configuration variables. See #2006 for example, where the OtaUpgrade library requires pynacl only if signing and/or encryption are enabled.

If a library/Component comes with an example project ('example' subdir in tree below), its Python requirements are irrelevant for the project consuming the library/Component and hence are not installed.
However, if the example is itself a Sming project it can provide its own requirements.txt (in addition to the requirements file of its parent library).

Sming/Libraries/MyLib
|- example
| |- ... (sources, component.mk, Makefile including $(SMING_HOME)/project.mk)
| |_ requirements.txt
| - ... (component source)
| - component.mk
|_ requirements.txt

@aemseemann
Copy link
Contributor Author

aemseemann commented Jan 14, 2020

@mikee47:
I don't think it is a good idea if the build system does something unexpected, at least not automatically, and modifying the user's system (everything beyond the working copy) would certainly qualify as unexpected IMO.

There are too many ways the user might want to manage its python installation. Some users might be fine with a user/system wide installation. Some might prefer a virtualenv (for this project only or shared with other Sming working copies...). Others might want to use Distro packages instead of pip.

For the latter kind, it might be helpful to add another target to just dump all the requirements.txt files relevant for the project. <-- Edit: done with commit bb48e01: make list-python-requirements

@aemseemann
Copy link
Contributor Author

* CI server integration: One idea to add a similar target that recurses into every sample and invokes `python-requirements` for each of them.

I tried to add a python-requirements target to $(SMING_HOME)/Makefile that recurses into every sample (similar to make samples). However, that target takes quite some time to complete, even if all requirements are already installed, so I think for the CI build scripts, it is probably best to stick with the current solution and invoke make python-requirements only where needed (currently only HttpServer_FirmwareUpload).

@aemseemann aemseemann force-pushed the feature/python-requirements branch 3 times, most recently from 2987dda to 51fa858 Compare January 15, 2020 20:52
Sming/project.mk Outdated
@@ -160,6 +163,7 @@ COMPONENT_VARS :=
COMPONENT_RELINK_VARS :=
COMPONENT_TARGETS :=
COMPONENT_DEPENDS :=
override undefine COMPONENT_PYTHON_REQUIREMENTS
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't work with MinGW. Why not just COMPONENT_PYTHON_REQUIREMENTS := ...

Copy link
Contributor

Choose a reason for hiding this comment

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

(Specifically, undefine not supported in GNU Make 3.81)

Copy link
Contributor Author

@aemseemann aemseemann Feb 1, 2020

Choose a reason for hiding this comment

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

The difference between an undefined vs. an empty COMPONENT_PYTHON_REQUIREMENTS is auto detection of 'requirements.txt', i.e. in the empty case no auto-detection is performed.

In the latest commit, I moved auto-detection before component evaluation. This way, the component can simply clear the variable if it wishes to suppress an auto-detected requirements.txt. No undefine necessary & successfully tested with make 3.81.

Sming/project.mk Outdated
@@ -184,6 +188,11 @@ CMP_$1_LIBNAME := $$(COMPONENT_LIBNAME)
CMP_$1_INCDIRS := $$(COMPONENT_INCDIRS)
CMP_$1_DEPENDS := $$(COMPONENT_DEPENDS)
CMP_$1_RELINK_VARS := $$(COMPONENT_RELINK_VARS)
ifeq ($$(origin COMPONENT_PYTHON_REQUIREMENTS),undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

... and ifndef COMPONENT_PYTHON_REQUIREMENTS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would have much clearer, but no longer necessary.

@aemseemann aemseemann force-pushed the feature/python-requirements branch from ff18455 to 23f9e68 Compare February 1, 2020 19:41
@aemseemann
Copy link
Contributor Author

@mikee47: The Travis build failure seems to be related to the install step of the python requirements for building the docs. Unfortunately I do not understand what the actual problem is. Can you shed some light on what's going on there? (If I remember correctly, it's not the first time I encounter this error, but it always disappeared when triggering another build.)

@mikee47 mikee47 force-pushed the feature/python-requirements branch from 23f9e68 to 63d2b50 Compare February 1, 2020 21:27
@mikee47
Copy link
Contributor

mikee47 commented Feb 1, 2020

@aemseemann Checking back through the logs the error: invalid command 'bdist_wheel' has been present for months, but doesn't usually cause a build problem (even with the docs). I've got it fixed by installing extra packages, etc. now just whittling it down to find the ones we actually need.

@mikee47 mikee47 force-pushed the feature/python-requirements branch from 63d2b50 to 2e6bcb5 Compare February 1, 2020 22:27
@mikee47
Copy link
Contributor

mikee47 commented Feb 1, 2020

@aemseemann I appear to be able to push commits directly to your branch... that was initially by accident but hope it's OK to do it here?

Various things but key ones are:

* Updating pip3 version
* Adding 'wheel' and 'cairocffi' packages
@mikee47 mikee47 force-pushed the feature/python-requirements branch from 2e6bcb5 to 50d0a8d Compare February 1, 2020 22:35
@aemseemann
Copy link
Contributor Author

aemseemann commented Feb 2, 2020

@mikee47: No problem for pushing directly to the branch, but it is quite strange indeed. I cannot see any setting that would allow this. EDIT: It's not that strange after all: By ticking "Allow edits from maintainers" for this PR I've granted push permissions to anyone with write access to the upstream repository.
Anyway, thanks for fixing the build. Now it's up to @slaff to decide if the doc fix can remain in here or should be factored out into another PR.

@slaff
Copy link
Contributor

slaff commented Feb 10, 2020

@aemseemann @mikee47 What is the state of this PR?

@aemseemann
Copy link
Contributor Author

@slaff: If you agree that @mikee47's fixes to the random documentation build failures can stay part of this PR, then its ready from my side.

@slaff slaff added this to the 4.1.1 milestone Feb 11, 2020
@slaff slaff merged commit 15193da into SmingHub:develop Feb 11, 2020
@slaff slaff mentioned this pull request May 8, 2020
4 tasks
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