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

micropython: update to 1.23.0 and fix compile on external mbedtls3 #24664

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gstrauss
Copy link
Contributor

@gstrauss gstrauss commented Jul 25, 2024

Maintainer: @jefferyto Jeffery To jeffery.to@gmail.com
Compile tested: mips_24kc_musl

Description:

  • update micropython-lib to hash for 1.23.0
  • remove micropython-lib unix-ffi patches

See micropython/micropython-lib@23df50d for upstream commit with commit message "unix-ffi: Remove "unix_ffi" argument from require(). And describe how to use add_library() instead."

This PR is a subsequent attempt at #24102, though has not yet been run-tested. (tag @orangepizza)

@gstrauss
Copy link
Contributor Author

This PR also subsumes #23795 (tag @nzmichaelh)

@gstrauss gstrauss marked this pull request as draft July 25, 2024 07:21
@gstrauss
Copy link
Contributor Author

build failure
Package 're' not found in any known library. from tools/manifestfile.py
Missing dependency? I will try to look more this weekend.

@gstrauss
Copy link
Contributor Author

@jefferyto Should the remaining patch in micropython-lib be removed, and --unix-ffi removed from micropython-lib/Makefile? (build will then succeed)

@gstrauss gstrauss marked this pull request as ready for review July 25, 2024 08:02
@nzmichaelh
Copy link

This PR also subsumes #23795 (tag @nzmichaelh)

Thanks!

@gstrauss
Copy link
Contributor Author

@jefferyto please review. Note: I removed the --unix-ffi feature that you had added via patches and now the build passes.

See micropython/micropython-lib@23df50d for upstream commit with commit message "unix-ffi: Remove "unix_ffi" argument from require(). And describe how to use add_library() instead."

@orangepizza
Copy link
Contributor

orangepizza commented Jul 26, 2024

@gstrauss it did compile but when I tried requesting by mpython it didn't have any CA certificates loaded
not sure it it was intended state though

edit: no CA cert is intended default statue of micropython, but because mbedtls3.x removed ssl mode option on client mbedtls refuse to connect

@orangepizza
Copy link
Contributor

orangepizza commented Jul 26, 2024

micropython/micropython-lib#838
looks like we may need to change default states:
for now we can edit
https://github.com/search?q=repo%3Amicropython%2Fmicropython-lib+tls.SSLContext%28tls.PROTOCOL_TLS_CLIENT%29&type=code
those files so requests can look as our ca certificate bundle
edit: it only support a single CA in that file, so this isn't for general https request:

@gstrauss
Copy link
Contributor Author

@orangepizza that default in the new python-only ssl code is an awful default for security. Thank you for highlighting it.

Do you happen to know if this is a change in behavior from micropython-lib v1.22.0? Attempting to modify this PR to use micropython-lib v1.22.0 reveals an issue I did not immediately know how to solve, which is a missing "re" python module for unix-ffi, so the build fails.

You're welcome to copy or resubmit a different PR if you come up with a different solution. I'm just trying to help a little bit with the openwrt mbedtls 3.x migration.

@orangepizza
Copy link
Contributor

orangepizza commented Jul 27, 2024

It always had awful default but mbedtls 3 killed cert_none on tls 1.3 so now we have non working default: while it's documented to not support multiple certs in cacert file mbedtls context itself supports, so I'm not sure if it actually not working on full unix (this have microcontrollers that wouldnt) but looking at micropython mbedtls hook I see no reason it'd break buy multiple certs in pem, it calls mbedtls_x509_crt_parse directly on buffer

@BKPepe BKPepe requested a review from jefferyto August 1, 2024 10:39
@Ansuel
Copy link
Member

Ansuel commented Oct 22, 2024

@gstrauss totally missed this...

can you include the additional patch and rename it to a better name?

#25184

@gstrauss
Copy link
Contributor Author

@Ansuel I will take a look but might have some questions.

I just updated micropython/micropython#15547 to try to get micropython submodule lib/mbedtls point to mbedtls 3.6.2. Hopefully it will be part of micropython 1.24.0.

@gstrauss
Copy link
Contributor Author

@Ansuel I renamed 010-cdefs.patch to 010-lib-berkeley-db-cdefs.patch
and renamed 011-lib-berkeley-db-fix-no-init.patch to 011-lib-berkeley-db-fix-uninit-var.patch
Please review and let me know if you would like me to change the commit message for the part of the patches I took from #25184 to fix the compiler uninitialized variable errors.

@gstrauss gstrauss force-pushed the micropython-mbedtls-3.x branch 2 times, most recently from cb1be35 to b092247 Compare October 24, 2024 05:49
@gstrauss
Copy link
Contributor Author

@jefferyto please review. Note: I removed the --unix-ffi feature that you had added via patches and now the build passes.

All tests pass except for (temporary?) provisioning error for Test Build / Test i386_pentium-mmx (pull_request)
#3 ERROR: failed to authorize: failed to fetch oauth token: unexpected status from POST request to https://auth.docker.io/token: 502 Bad Gateway

See micropython/micropython-lib@23df50d for upstream commit with commit message "unix-ffi: Remove "unix_ffi" argument from require(). And describe how to use add_library() instead."

@gstrauss
Copy link
Contributor Author

FYI: the non-working micropython default (with mbedtls 3.x) has been reported upstream by @orangepizza
micropython/micropython#14447 (comment)

Copy link
Member

@jefferyto jefferyto left a comment

Choose a reason for hiding this comment

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

This PR is a subsequent attempt at #24102, though has not yet been run-tested.

Thanks for working on this, but please run-test.

lang/python/micropython-lib/Makefile Outdated Show resolved Hide resolved
@jefferyto
Copy link
Member

Thanks for working on this, but please run-test.

Running micropython-unix adds the unix-ffi packages directory into the library path, so calling add_library() should not be necessary and the unix-ffi versions of packages should be used by default. (Running micropython does not add the unix-ffi packages directory into the library path so the regular libraries should be used.)

@neheb
Copy link
Contributor

neheb commented Oct 27, 2024

Oh, unfortunate. Please rebase.

@gstrauss
Copy link
Contributor Author

Running micropython-unix adds the unix-ffi packages directory into the library path, so calling add_library() should not be necessary and the unix-ffi versions of packages should be used by default. (Running micropython does not add the unix-ffi packages directory into the library path so the regular libraries should be used.)

I am not well-versed in micropython enough to comment whether or not this approach is recommended (or not) by micropython developers, or if there are alternative approaches which might be used instead.

I'll remove my commit which removed --unix-ffi, will rebase, and will re-run build tests later tonight.

Thanks for working on this, but please run-test.

I am a C programmer and have written foreign function interfaces (FFI) for various scripting languages. However, I am not an expert in micropython and so the limit of my confidence is running the automated tests. If they have good coverage, then that is great. If they do not, then I politely request the help of others to test and validate.

@gstrauss gstrauss force-pushed the micropython-mbedtls-3.x branch 2 times, most recently from 07d0052 to 5c72154 Compare October 27, 2024 23:53
@gstrauss
Copy link
Contributor Author

I removed the patches to micropython. This PR now contains only changes to micropython-lib. @Ansuel one of us should make a separate PR for your patch to quell compiler uninitialized variable warnings, and whether or not to add my patch which removes the copy of cdefs.h that you added and replaces with a small number of bog-standard macros.

In this PR, powerpc CI tests are failing to download packages.

Use of uninitialized value $1 in open at /builder/scripts/download.pl line 36.
Use of uninitialized value $1 in open at /builder/scripts/download.pl line 36.
Can't open file No such file or directory at /builder/scripts/download.pl line 36.

Other platforms are failing with

2024-10-28T00:02:18.9176002Z Generating bytecode version 6
2024-10-28T00:02:18.9176634Z unix-ffi/email.charset
2024-10-28T00:02:18.9177102Z Traceback (most recent call last):
2024-10-28T00:02:18.9178441Z   File "/builder/staging_dir/target-x86_64_musl/host/lib/micropython/tools/manifestfile.py", line 388, in include
2024-10-28T00:02:18.9179934Z     exec(f.read(), self._manifest_globals(kwargs))
2024-10-28T00:02:18.9180629Z   File "<string>", line 4, in <module>
2024-10-28T00:02:18.9182005Z   File "/builder/staging_dir/target-x86_64_musl/host/lib/micropython/tools/manifestfile.py", line 449, in require
2024-10-28T00:02:18.9183630Z     raise ValueError("Package '{}' not found in any known library.".format(name))
2024-10-28T00:02:18.9184866Z ValueError: Package 'email.encoders' not found in any known library.
2024-10-28T00:02:18.9185496Z 
2024-10-28T00:02:18.9185922Z During handling of the above exception, another exception occurred:
2024-10-28T00:02:18.9186557Z 
2024-10-28T00:02:18.9186757Z Traceback (most recent call last):
2024-10-28T00:02:18.9188110Z   File "/builder/build_dir/target-x86_64_musl/micropython-lib-20240525~50ed36fb/tools/build.py", line 455, in <module>
2024-10-28T00:02:18.9189266Z     main()
2024-10-28T00:02:18.9190372Z   File "/builder/build_dir/target-x86_64_musl/micropython-lib-20240525~50ed36fb/tools/build.py", line 451, in main
2024-10-28T00:02:18.9191985Z     build(args.output, args.unix_ffi, hash_prefix_len=max(4, args.hash_prefix), mpy_cross_path=args.mpy_cross)
2024-10-28T00:02:18.9193277Z   File "/builder/build_dir/target-x86_64_musl/micropython-lib-20240525~50ed36fb/tools/build.py", line 327, in build
2024-10-28T00:02:18.9193927Z     manifest.execute(manifest_path)
2024-10-28T00:02:18.9194643Z   File "/builder/staging_dir/target-x86_64_musl/host/lib/micropython/tools/manifestfile.py", line 251, in execute
2024-10-28T00:02:18.9195284Z     self.include(manifest_file)
2024-10-28T00:02:18.9195964Z   File "/builder/staging_dir/target-x86_64_musl/host/lib/micropython/tools/manifestfile.py", line 399, in include
2024-10-28T00:02:18.9196785Z     raise ManifestFileError("Error in manifest file: {}: {}".format(manifest_path, e))
2024-10-28T00:02:18.9198236Z manifestfile.ManifestFileError: Error in manifest file: /builder/build_dir/target-x86_64_musl/micropython-lib-20240525~50ed36fb/unix-ffi/email.charset/manifest.py: Package 'email.encoders' not found in any known library.
2024-10-28T00:02:18.9238249Z make[2]: *** [Makefile:136: /builder/build_dir/target-x86_64_musl/micropython-lib-20240525~50ed36fb/.built] Error 1
2024-10-28T00:02:18.9239624Z make[2]: Leaving directory '/feed/lang/python/micropython-lib'
2024-10-28T00:02:18.9241952Z time: package/feeds/packages_ci/micropython-lib/compile#0.76#0.66#1.35
2024-10-28T00:02:18.9249623Z     ERROR: package/feeds/packages_ci/micropython-lib failed to build.
2024-10-28T00:02:18.9254974Z make[1]: *** [package/Makefile:177: package/feeds/packages_ci/micropython-lib/compile] Error 1
2024-10-28T00:02:18.9256026Z make[1]: Leaving directory '/builder'
2024-10-28T00:02:18.9261055Z make: *** [/builder/include/toplevel.mk:241: package/micropython-lib/compile] Error 2

More work is needed for unix-ffi integration.

@jefferyto I think micropython-lib with mbedtls has been broken in 23.05 for a while, so have you considered removing it instead of fixing it?

@Neustradamus
Copy link

@jefferyto
Copy link
Member

@gstrauss please try replacing 001-build-unix-ffi.patch with:

--- a/tools/build.py
+++ b/tools/build.py
@@ -289,7 +289,7 @@ def _update_index_package_metadata(index
     index_package_json["path"] = package_path
 
 
-def build(output_path, hash_prefix_len, mpy_cross_path):
+def build(output_path, unix_ffi, hash_prefix_len, mpy_cross_path):
     import manifestfile
     import mpy_cross
 
@@ -315,7 +315,10 @@ def build(output_path, hash_prefix_len,
 
     # For now, don't process unix-ffi. In the future this can be extended to
     # allow a way to request unix-ffi packages via mip.
-    lib_dirs = ["micropython", "python-stdlib", "python-ecosys"]
+    lib_dirs = ["unix-ffi"] if unix_ffi else ["micropython", "python-stdlib", "python-ecosys"]
+
+    if unix_ffi:
+        manifestfile.BASE_LIBRARY_NAMES = ("unix-ffi",) + manifestfile.BASE_LIBRARY_NAMES
 
     mpy_version, _mpy_sub_version = mpy_cross.mpy_version(mpy_cross=mpy_cross_path)
     mpy_version = str(mpy_version)
@@ -446,6 +449,7 @@ def main():
 
     cmd_parser = argparse.ArgumentParser(description="Compile micropython-lib for serving to mip.")
     cmd_parser.add_argument("--output", required=True, help="output directory")
+    cmd_parser.add_argument("--unix-ffi", action="store_true", help="process unix-ffi packages")
     cmd_parser.add_argument("--hash-prefix", default=8, type=int, help="hash prefix length")
     cmd_parser.add_argument("--mpy-cross", default=None, help="optional path to mpy-cross binary")
     cmd_parser.add_argument("--micropython", default=None, help="path to micropython repo")
@@ -455,7 +459,7 @@ def main():
         sys.path.append(os.path.join(args.micropython, "tools"))  # for manifestfile
         sys.path.append(os.path.join(args.micropython, "mpy-cross"))  # for mpy_cross
 
-    build(args.output, hash_prefix_len=max(4, args.hash_prefix), mpy_cross_path=args.mpy_cross)
+    build(args.output, args.unix_ffi, hash_prefix_len=max(4, args.hash_prefix), mpy_cross_path=args.mpy_cross)
 
 
 if __name__ == "__main__":

* update micropython-lib to hash for 1.23.0
* remove micropython-lib unix-uffi patches

Co-authored-by: Jeffery To <jeffery.to@gmail.com>

Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
@gstrauss
Copy link
Contributor Author

@jefferyto Thank you. Your suggestion did the trick. I added you as co-author on the commit.

Only test that fails is on powerpc_464fp platform with temporary error:

Use of uninitialized value $1 in open at /builder/scripts/download.pl line 36.
Use of uninitialized value $1 in open at /builder/scripts/download.pl line 36.
Can't open file No such file or directory at /builder/scripts/download.pl line 36.

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.

7 participants