CMake: better detection of memap dependencies #14205

multiplemonomials · 2021-01-28T03:47:41Z

Summary of changes

The current CMake build system is a bit naive about executing memap. It just assumes that Python was found and that memap's dependencies (prettytable and intelhex) are installed. If this is not true, you'll get errors.

This PR adds a module I wrote a while ago, CheckPythonPackage.cmake, and uses it to verify that the correct python dependencies are installed. If they aren't, a warning will be printed and memap won't be run.

Broken out from #14010

Impact of changes

CMake will now work if the python dependencies are not installed instead of erroring.

Migration actions required

None needed.

Documentation

None needed.

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@0xc0170

ciarmcom · 2021-01-28T04:00:19Z

@multiplemonomials, thank you for your changes.
@0xc0170 @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

0xc0170

@hugueskamba as we are porting post build hooks, this comes handy. please review

0xc0170 · 2021-01-28T09:01:31Z

tools/cmake/app.cmake

+# Find Python and needed packages
+find_package(Python3)
+include(${CMAKE_CURRENT_LIST_DIR}/CheckPythonPackage.cmake)
+check_python_package(intelhex HAVE_INTELHEX)


we should read requirement.txt in tools/cmake as we might need few packages more (we are adding post build hooks that rely on some security modules).

The below could check if any of req. are not installed, print a warning otherwise we got all we need

The requirements.txt in the root directory should also be checked as it is the one that has the required packages for post build operations..

This is possible but it's a bit harder because it means CMake needs to parse the "platform_system==XXXX" declarations that are used in that file.

The requirements.txt in the root directory should also be checked as it is the one that has the required packages for post build operations..

Let's keep Cmake reqs separate and do proper clean up of deps (drop in replacement once the old tools are removed).

tools/cmake/python_packagecheck.py

tools/cmake/app.cmake

tools/cmake/python_packagecheck.py

tools/cmake/CheckPythonPackage.cmake

hugueskamba · 2021-01-28T09:27:38Z

tools/cmake/app.cmake

+# Find Python and needed packages
+find_package(Python3)
+include(${CMAKE_CURRENT_LIST_DIR}/CheckPythonPackage.cmake)
+check_python_package(intelhex HAVE_INTELHEX)


The requirements.txt in the root directory should also be checked as it is the one that has the required packages for post build operations..

tools/cmake/CheckPythonPackage.cmake

tools/cmake/python_packagecheck.py

multiplemonomials · 2021-01-28T09:48:54Z

@0xc0170 I just added a requirements.txt parser based on your request. I will warn though, it's a bit fragile and could break if weird package names are used.

multiplemonomials · 2021-01-28T09:53:26Z

Also, what specifically is your policy about extra lines, could you please explain? I seem to have run afoul of it pretty badly.

multiplemonomials · 2021-01-28T10:02:27Z

Oh one other thing. While you guys are here, do you know why the requirements.txt has so many pinned versions and "must be older than X" versions? This really causes problems trying to set up an environment, and in my experience it seems to run just fine with the latest/default versions of all packages.

0xc0170 · 2021-01-28T10:18:02Z

Oh one other thing. While you guys are here, do you know why the requirements.txt has so many pinned versions and "must be older than X" versions? This really causes problems trying to set up an environment, and in my experience it seems to run just fine with the latest/default versions of all packages.

We should start fixing this. Therefore we create own req file for CMake, so we can add only what we use and test the latest packages. For some historic reasons older versions were used, it's not true anymore in many cases.

@0xc0170 I just added a requirements.txt parser based on your request. I will warn though, it's a bit fragile and could break if weird package names are used.

Can you illustrate ?

Also, what specifically is your policy about extra lines, could you please explain? I seem to have run afoul of it pretty badly.

I don't follow, can you reference the lines ? We got one extra line at end of every file (I noticed Github showed in some files here there is no empty line at the end of the files).

multiplemonomials · 2021-01-28T10:23:37Z

Can you illustrate ?

Added it to this PR: 5d77b3e

I don't follow, can you reference the lines ? We got one extra line at end of every file (I noticed Github showed in some files here there is no empty line at the end of the files).

Hugues asked me to add some extra blank lines, which I did. I'm just wondering since I'm required to add them, what is the formal policy on where blank lines are needed?

multiplemonomials · 2021-01-28T10:32:37Z

Oh wait, you meant illustrate about the weird package names. I made the following assumptions:

No package names contain spaces or equals signs
For a package named X, lowercase(X) is the importable name of that package
The requirements file has either a newline, a "<", a ">", or a "=" after each package name.

hugueskamba · 2021-01-28T11:28:55Z

Can you illustrate ?

Added it to this PR: 5d77b3e

I don't follow, can you reference the lines ? We got one extra line at end of every file (I noticed Github showed in some files here there is no empty line at the end of the files).

Hugues asked me to add some extra blank lines, which I did. I'm just wondering since I'm required to add them, what is the formal policy on where blank lines are needed?

I asked to add a new line at the end of the file.
https://stackoverflow.com/questions/2287967/why-is-it-recommended-to-have-empty-line-in-the-end-of-a-source-file

tools/cmake/CheckPythonPackage.cmake

ciarmcom · 2021-01-31T16:50:19Z

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please start CI to get the PR merged.

tools/cmake/requirements.txt

Pull request has been modified.

0xc0170 · 2021-02-02T10:50:44Z

CI restarted

rajkan01 · 2021-02-02T10:44:25Z

tools/cmake/CheckPythonPackage.cmake

+
+# set OUTPUT_VAR to whether PACKAGENAME was found
+function(check_python_package PACKAGENAME OUTPUT_VAR)
+


@multiplemonomials In Mbed OS CMake code we are not using newline after function starts, Could you please remove the new line?

rajkan01 · 2021-02-02T10:45:46Z

tools/cmake/CheckPythonPackage.cmake

+
+    if(DEFINED ${OUTPUT_VAR})
+        if(${OUTPUT_VAR})
+


@multiplemonomials In Mbed OS CMake code we are not using newline after if condition, Could you please remove the new line?

rajkan01 · 2021-02-02T11:36:08Z

tools/cmake/CheckPythonPackage.cmake

+            if("${PY_INTERP_FOR_${OUTPUT_VAR}}" STREQUAL "${Python3_EXECUTABLE}")
+                set(NEED_TO_RUN_CHECK FALSE)
+            endif()
+


@multiplemonomials, Same comment as before, could you please remove the new line?

rajkan01 · 2021-02-02T11:36:18Z

tools/cmake/CheckPythonPackage.cmake

+
+        set(${OUTPUT_VAR} ${HAVE_PACKAGE} CACHE BOOL "Whether the Python package ${PACKAGENAME} was found" FORCE)
+        mark_as_advanced(${OUTPUT_VAR})
+


@multiplemonomials, Same comment as before, could you please remove the new line?

rajkan01 · 2021-02-02T11:36:35Z

tools/cmake/app.cmake

+# Check python packages from requirements.txt
+file(STRINGS ${CMAKE_CURRENT_LIST_DIR}/requirements.txt PYTHON_REQUIREMENTS)
+foreach(REQUIREMENT ${PYTHON_REQUIREMENTS})
+


@multiplemonomials, Same comment as before, could you please remove the new line?

rajkan01 · 2021-02-02T11:36:43Z

tools/cmake/app.cmake

+
+	# Look for a string from the start of each line that does not contain "<", ">", "=", or " ".
+	if(REQUIREMENT MATCHES "^([^<>= ]+)")
+


@multiplemonomials, Same comment as before, could you please remove the new line?

rajkan01 · 2021-02-02T11:36:50Z

tools/cmake/app.cmake

+		if(NOT HAVE_PYTHON_${PACKAGE_NAME_UCASE})
+			message(WARNING "Missing Python dependency ${PACKAGE_NAME}")
+		endif()
+


@multiplemonomials, Same comment as before, could you please remove the new line?

rajkan01 · 2021-02-02T11:37:08Z

tools/cmake/app.cmake

+
+		message(FATAL_ERROR "Cannot parse line \"${REQUIREMENT}\" in requirements.txt")
+


@multiplemonomials, Same comment as before, could you please remove the new line?

rajkan01 · 2021-02-02T11:37:20Z

tools/cmake/app.cmake

+	set(HAVE_MEMAP_DEPS TRUE)
+else()
+	set(HAVE_MEMAP_DEPS FALSE)
+


@multiplemonomials, Same comment as before, could you please remove the new line?

Co-authored-by: Hugues Kamba <[email protected]>

…can be replaced with command line

multiplemonomials · 2021-02-03T22:17:10Z

OK just rebased

Pull request has been modified.

0xc0170 · 2021-02-04T08:44:54Z

CI restarted

mbed-ci · 2021-02-04T09:11:11Z

Jenkins CI Test : ❌ FAILED

Build Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	❌

0xc0170 · 2021-02-04T16:03:11Z

CI was aborted. We had some issue with it, I'll restart today.

0xc0170 · 2021-02-04T20:05:55Z

CI started

mbed-ci · 2021-02-04T21:54:17Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 5 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jan 28, 2021

ciarmcom requested review from 0xc0170 and a team January 28, 2021 04:00

ciarmcom added the needs: review label Jan 28, 2021

0xc0170 reviewed Jan 28, 2021

View reviewed changes

0xc0170 changed the title ~~Better detection of memap dependencies~~ CMake: better detection of memap dependencies Jan 28, 2021

0xc0170 requested review from rajkan01 and hugueskamba January 28, 2021 09:04

hugueskamba reviewed Jan 28, 2021

View reviewed changes

tools/cmake/CheckPythonPackage.cmake Show resolved Hide resolved

0xc0170 previously approved these changes Jan 29, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Jan 29, 2021

ciarmcom added the stale Stale Pull Request label Jan 31, 2021

rajkan01 reviewed Feb 1, 2021

View reviewed changes

tools/cmake/requirements.txt Outdated Show resolved Hide resolved

rajkan01 previously requested changes Feb 2, 2021

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Feb 2, 2021

multiplemonomials and others added 11 commits February 3, 2021 14:11

Add intelhex to requirements.txt as it's a dependency of memap.

c193527

Fix license header

c667747

Parse requirements.txt in CMake

947c804

Reformat tools/cmake/CheckPythonPackage.cmake

fafb08c

Co-authored-by: Hugues Kamba <[email protected]>

Fix indentation error in tools/cmake/CheckPythonPackage.cmake

049f1a7

Co-authored-by: Hugues Kamba <[email protected]>

Add extra lines as requested

e61a45a

Fix incorrect comment in tools/cmake/python_packagecheck.py

8eca0cf

Co-authored-by: Hugues Kamba <[email protected]>

Realized that companion Python script is actually not needed at all, …

df60d42

…can be replaced with command line

Add intelhex version info (same as ARMmbed#14201)

0a67fef

Whitespace formatting changes.

9e2b04e

Rebase against master

7b03aea

multiplemonomials force-pushed the master branch from f60f0a4 to 7b03aea Compare February 3, 2021 22:16

0xc0170 added needs: CI and removed needs: work labels Feb 4, 2021

0xc0170 approved these changes Feb 5, 2021

View reviewed changes

mergify bot added the ready for merge label Feb 5, 2021

0xc0170 removed the needs: CI label Feb 5, 2021

0xc0170 merged commit dd33463 into ARMmbed:master Feb 5, 2021

mergify bot removed the ready for merge label Feb 5, 2021

mbedmain added release-version: 6.8.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Feb 16, 2021


		# set OUTPUT_VAR to whether PACKAGENAME was found
		function(check_python_package PACKAGENAME OUTPUT_VAR)


		set(${OUTPUT_VAR} ${HAVE_PACKAGE} CACHE BOOL "Whether the Python package ${PACKAGENAME} was found" FORCE)
		mark_as_advanced(${OUTPUT_VAR})


		# Look for a string from the start of each line that does not contain "<", ">", "=", or " ".
		if(REQUIREMENT MATCHES "^([^<>= ]+)")


		message(FATAL_ERROR "Cannot parse line \"${REQUIREMENT}\" in requirements.txt")

CMake: better detection of memap dependencies #14205

CMake: better detection of memap dependencies #14205

Uh oh!

Conversation

multiplemonomials commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Jan 28, 2021

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

multiplemonomials commented Jan 28, 2021

Uh oh!

multiplemonomials commented Jan 28, 2021

Uh oh!

multiplemonomials commented Jan 28, 2021

Uh oh!

0xc0170 commented Jan 28, 2021

Uh oh!

multiplemonomials commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

multiplemonomials commented Jan 28, 2021

Uh oh!

hugueskamba commented Jan 28, 2021

Uh oh!

Uh oh!

ciarmcom commented Jan 31, 2021

Uh oh!

Uh oh!

0xc0170 commented Feb 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

multiplemonomials commented Jan 28, 2021 •

edited

Loading

multiplemonomials commented Jan 28, 2021 •

edited

Loading