Improve submodule error messages for Github archives #9571

Harry-Ramsey · 2024-09-17T14:39:58Z

This commit improves the error messages informing users that have downloaded Github archives to instead download a release archive. This is due to Github not supporting submodules within archives and no trivial way for users to use git to download them.

Issue: #9538.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

changelog not required because: minimal build change that does not affect API.
development PR provided
framework PR not required
3.6 PR provided: [Backport 3.6] Improve submodule error messages for Github archives #9750.
2.28 PR not required: no submodules in version 2.28.
tests not required because: build configuration change.

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

Multiple simple commits
- please structure your PR into a series of small commits, each of which does one thing
Avoid force-push
- please do not force-push to update your PR - just add new commit(s)
See our Guidelines for Contributors for more details about the review process.

eleuzi01 · 2024-09-24T13:11:19Z

CMakeLists.txt

@@ -301,7 +301,11 @@ if(LIB_INSTALL_DIR)
 endif()

 if (NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/framework/CMakeLists.txt")
-    message(FATAL_ERROR "${CMAKE_CURRENT_SOURCE_DIR}/framework/CMakeLists.txt not found. Run `git submodule update --init` from the source tree to fetch the submodule contents.")
+    if (NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/.git/")
+        message(FATAL_ERROR "${CMAKE_CURRENT_SOURCE_DIR}/.git/ not found. Please ensure you have downloaded a release version of MBedTLS from Github.")


Suggested change

message(FATAL_ERROR "${CMAKE_CURRENT_SOURCE_DIR}/.git/ not found. Please ensure you have downloaded a release version of MBedTLS from Github.")

message(FATAL_ERROR "${CMAKE_CURRENT_SOURCE_DIR}/.git/ not found. Please ensure you have downloaded a release version of Mbed TLS from GitHub.")

Thanks, I have fixed this now.

eleuzi01 · 2024-09-24T13:13:23Z

Makefile

+        ifeq (,$(wildcard .git))
+          define error_message
+$(MBEDTLS_PATH)/.git/ not found.
+Please ensure you have downloaded a release version of MbedTLS from Github.


In most of our docs we have Mbed TLS (with a space), might be good to keep it consistent?

Thanks, I have fixed this now.

eleuzi01

LGTM, thanks!

This commit improves the error messages informing users that have downloaded Github archives to instead download a release archive. This is due to Github not supporting submodules within archives and no trivial way for users to use git to download them. Signed-off-by: Harry Ramsey <[email protected]>

davidhorstmann-arm

LGTM, thanks!

mpg · 2024-10-28T11:22:15Z

CMakeLists.txt

@@ -323,7 +323,11 @@ if(LIB_INSTALL_DIR)
 endif()

 if (NOT EXISTS "${MBEDTLS_FRAMEWORK_DIR}/CMakeLists.txt")
-    message(FATAL_ERROR "${MBEDTLS_FRAMEWORK_DIR}/CMakeLists.txt not found. Run `git submodule update --init` from the source tree to fetch the submodule contents.")
+    if (NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/.git/")
+        message(FATAL_ERROR "${CMAKE_CURRENT_SOURCE_DIR}/.git/ not found. Please ensure you have downloaded a release version of Mbed TLS from GitHub.")


I'm not sure about the ".git not found" part of this error message, from a user's perspective: if I go and download a release tarball, it won't contain a .git subdirectory either, so there seems to be a mismatch between reported problem and suggested solution.

Suggestion: start this message with ""${MBEDTLS_FRAMEWORK_DIR}/CMakeLists.txt not found (and this does not appear to be a git checkout)." - and add "(and this appears to be a git checkout)" to the other one.

Not sure about the 2nd sentence either: most likely this will happen because people have downloaded an archive from github, except they've picked the wrong one. So I think the message should be more precise, like "Please ensure you have downloaded the right archive from the release page on github."

mpg · 2024-10-28T11:24:31Z

Makefile

@@ -11,6 +11,19 @@ $(MBEDTLS_PATH)/framework/exported.make not found.
 Run `git submodule update --init` to fetch the submodule contents.
 This is a fatal error
        endef
+        ifeq (,$(wildcard .git))


I think the definition above (lines 9-13) is redundant with the one on lines 21-25. I'd keep only the second one.

@mpg

It appears @mpg has some suggested changes

This commit further improves error message clarity. Signed-off-by: Harry Ramsey <[email protected]>

eleuzi01

LGTM, I think it might be a good time to start on the backport

mpg

LGTM, thanks for addressing my feedback!

mpg

Missed something earlier - only notice while looking at the 3.6 backport.

mpg · 2024-11-04T11:16:40Z

Makefile

-Run `git submodule update --init` to fetch the submodule contents.
+        ifeq (,$(wildcard .git))
+            define error_message
+${MBEDTLS_FRAMEWORK_DIR}/CMakeLists.txt not found (and does appear to be a git checkout). Run `git submodule update --init` from the source tree to fetch the submodule contents.


Sorry I failed to noticed earlier: this should be framework/exported.make in order to match what we actually tested above.

This commit improves the makefile error message when using make, it no longer incorrectly reports that CMakeLists.txt cannot be found instead of exported.make. Signed-off-by: Harry Ramsey <[email protected]>

mpg

Not there yet. I think we should manually test the error messages to make sure they work. I'm not sure what the consequence of trying to use ${MBEDTLS_FRAMEWORK_DIR} would be, but we don't want to find out from user reporting about confusing errors ;)

mpg · 2024-11-04T11:47:08Z

Makefile

-Run `git submodule update --init` to fetch the submodule contents.
+        ifeq (,$(wildcard .git))
+            define error_message
+${MBEDTLS_FRAMEWORK_DIR}/exported.make not found (and does appear to be a git checkout). Run `git submodule update --init` from the source tree to fetch the submodule contents.


${MBEDTLS_FRAMEWORK_DIR} is a cmake variable, not a make variable. The old message used $(MBEDTLS_PATH)/framework/exported.make which looks right to me.

Hopefully all resolved now, thanks

This commit replaces an undefined variable ${MBEDTLS_FRAMEWORK_DIR} for ${MBEDTLS_PATH}. Signed-off-by: Harry Ramsey <[email protected]>

mpg

LGTM, thanks!

mpg · 2024-11-05T08:43:32Z

@Harry-Ramsey A few minor points on PR management:

please apply labels on backport PRs too (I've just done it this time)
when a PR is linked to an issue that's on a board, the PR shouldn't go on that board too (as we already have the issue there). Instead PRs with priority high or very-high go on "Roadmap pull requests (new board)" and priority medium don't need to go on a board (again, assuming they fix an existing issue that's already on a board).

eleuzi01

LGTM, thanks!

gilles-peskine-arm · 2024-12-09T18:44:59Z

Makefile

-        define error_message
-$(MBEDTLS_PATH)/framework/exported.make not found.
-Run `git submodule update --init` to fetch the submodule contents.
+        ifeq (,$(wildcard .git))


Is something wrong with the error message selection? #9809 (comment)

After testing and some initial digging, this seems to be an issue with Make wildcard checks. It appears that they are not useful for checking if a directory exists and instead a different approach will be needed. I shall investigate a bit further in the morning about it.

That test looks right for checking whether .git exists in the current directory. But isn't the condition inverted? $(wildcard .git) is empty when .git is missing. Also, do we want .git or $(MBEDTLS_PATH)/.git at this point?

Harry-Ramsey force-pushed the improve-submodule-error-messages-development branch from 7b6ef73 to 91f7ce6 Compare September 17, 2024 14:41

Harry-Ramsey added enhancement needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) labels Sep 17, 2024

Harry-Ramsey self-assigned this Sep 17, 2024

eleuzi01 reviewed Sep 24, 2024

View reviewed changes

eleuzi01 added needs-backports Backports are missing or are pending review and approval. priority-medium Medium priority - this can be reviewed as time permits labels Sep 24, 2024

Harry-Ramsey force-pushed the improve-submodule-error-messages-development branch from ef0d38b to 9dbc616 Compare September 24, 2024 14:48

Harry-Ramsey force-pushed the improve-submodule-error-messages-development branch from 9dbc616 to 4544666 Compare October 4, 2024 12:12

eleuzi01 previously approved these changes Oct 14, 2024

View reviewed changes

Harry-Ramsey force-pushed the improve-submodule-error-messages-development branch from 4544666 to e05cf2e Compare October 18, 2024 07:21

davidhorstmann-arm previously approved these changes Oct 28, 2024

View reviewed changes

davidhorstmann-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Oct 28, 2024

mpg reviewed Oct 28, 2024

View reviewed changes

Further improve error message clarity

6d9c389

This commit further improves error message clarity. Signed-off-by: Harry Ramsey <[email protected]>

Harry-Ramsey dismissed eleuzi01’s stale review via 6d9c389 October 28, 2024 14:10

eleuzi01 previously approved these changes Oct 28, 2024

View reviewed changes

eleuzi01 added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed approved Design and code approved - may be waiting for CI or backports labels Oct 28, 2024

mpg approved these changes Oct 29, 2024

View reviewed changes

eleuzi01 removed the needs-review Every commit must be reviewed by at least two team members, label Oct 29, 2024

eleuzi01 added the approved Design and code approved - may be waiting for CI or backports label Oct 29, 2024

gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Oct 29, 2024

Harry-Ramsey mentioned this pull request Nov 4, 2024

[Backport 3.6] Improve submodule error messages for Github archives #9750

Merged

5 tasks

mpg requested changes Nov 4, 2024

View reviewed changes

mpg added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Nov 4, 2024

Harry-Ramsey dismissed eleuzi01’s stale review via 51b75a5 November 4, 2024 11:38

Improve makefile error message

66ce986

This commit improves the makefile error message when using make, it no longer incorrectly reports that CMakeLists.txt cannot be found instead of exported.make. Signed-off-by: Harry Ramsey <[email protected]>

Harry-Ramsey force-pushed the improve-submodule-error-messages-development branch from 51b75a5 to 66ce986 Compare November 4, 2024 11:40

mpg requested changes Nov 4, 2024

View reviewed changes

Fix undefined variable in makefile

0f07e76

This commit replaces an undefined variable ${MBEDTLS_FRAMEWORK_DIR} for ${MBEDTLS_PATH}. Signed-off-by: Harry Ramsey <[email protected]>

mpg approved these changes Nov 5, 2024

View reviewed changes

mpg added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Nov 5, 2024

mpg requested a review from eleuzi01 November 5, 2024 08:35

eleuzi01 approved these changes Nov 5, 2024

View reviewed changes

eleuzi01 added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. labels Nov 5, 2024

mpg added this pull request to the merge queue Nov 5, 2024

Merged via the queue into Mbed-TLS:development with commit e71f3c3 Nov 5, 2024
6 checks passed

Harry-Ramsey deleted the improve-submodule-error-messages-development branch November 5, 2024 13:04

gilles-peskine-arm reviewed Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve submodule error messages for Github archives #9571

Improve submodule error messages for Github archives #9571

Harry-Ramsey commented Sep 17, 2024 •

edited

Loading

eleuzi01 Sep 24, 2024

Harry-Ramsey Sep 24, 2024 •

edited

Loading

eleuzi01 Sep 24, 2024

Harry-Ramsey Sep 24, 2024

eleuzi01 left a comment

davidhorstmann-arm left a comment

mpg Oct 28, 2024

mpg Oct 28, 2024

eleuzi01 left a comment

mpg left a comment

mpg left a comment

mpg Nov 4, 2024

mpg left a comment

mpg Nov 4, 2024

Harry-Ramsey Nov 4, 2024

mpg left a comment

mpg commented Nov 5, 2024

eleuzi01 left a comment

gilles-peskine-arm Dec 9, 2024

Harry-Ramsey Dec 9, 2024 •

edited

Loading

gilles-peskine-arm Dec 10, 2024

	message(FATAL_ERROR "${CMAKE_CURRENT_SOURCE_DIR}/.git/ not found. Please ensure you have downloaded a release version of MBedTLS from Github.")
	message(FATAL_ERROR "${CMAKE_CURRENT_SOURCE_DIR}/.git/ not found. Please ensure you have downloaded a release version of Mbed TLS from GitHub.")

Improve submodule error messages for Github archives #9571

Improve submodule error messages for Github archives #9571

Conversation

Harry-Ramsey commented Sep 17, 2024 • edited Loading

PR checklist

Notes for the submitter

Choose a reason for hiding this comment

Harry-Ramsey Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eleuzi01 left a comment

Choose a reason for hiding this comment

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eleuzi01 left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg commented Nov 5, 2024

eleuzi01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harry-Ramsey Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harry-Ramsey commented Sep 17, 2024 •

edited

Loading

Harry-Ramsey Sep 24, 2024 •

edited

Loading

Harry-Ramsey Dec 9, 2024 •

edited

Loading