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

CI failing on all build-standalone steps #397

Closed
tsibley opened this issue Sep 30, 2024 · 2 comments
Closed

CI failing on all build-standalone steps #397

tsibley opened this issue Sep 30, 2024 · 2 comments
Assignees

Comments

@tsibley
Copy link
Member

tsibley commented Sep 30, 2024

First caught by a scheduled run:

error: adding PythonExecutable to FileManifest

Caused by:
    Caused by:
    0: building Python executable
        0: building Python executable
    1: building executable with Rust project
        1: building executable with Rust project
    2: obtaining embedded python context
        2: obtaining embedded python context
    3: converting aiohappyeyeballs._staggered to resource
    4: compiling in-memory bytecode
    5: compiling error: invalid syntax (aiohappyeyeballs._staggered, line 98)
        3: converting aiohappyeyeballs._staggered to resource
        4: compiling in-memory bytecode
        5: compiling error: invalid syntax (aiohappyeyeballs._staggered, line 98)
       --> ./pyoxidizer.bzl:131:5
        |
    131 |     files.add_python_resource(".", exe)
        |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PythonExecutable.to_file_manifest()

I noted it last week and hoped it would go away on its own in a few days. It didn't, so I diagnosed the issue this morning.

The problem is that aiohappyeyeballs._staggered (newly added in a recent release which we're now picking up, I assume) contains syntax (except*) that's only valid on Python 3.11 and newer, but we're building our standalone binaries with Python 3.10. The module isn't actually executed at runtime on older Python versions, but PyOxidizer doesn't know that and tries to pre-compile all Python modules it processes. For example, you can get the same result by running python -m py_compile …/aiohappyeyeballs/_staggered.py with Python ≤3.10.

This compilation issue is reported upstream as other systems (e.g. GCP functions) want to pre-compile as well.

We can probably solve this two ways:

  1. Upgrade our standalone builds to Python 3.12. This is easy to try and probably free of unintended consequential side-effects.
  2. Instruct PyOxidizer not to pre-compile this one module. It won't be executed anyway as long as we're on 3.10.
@tsibley tsibley self-assigned this Sep 30, 2024
@tsibley
Copy link
Member Author

tsibley commented Sep 30, 2024

Bumping the Python version used for standalone builds isn't trivial, as 3.10 is the latest built-in version available. But 3.12 builds exist and we could add it into PyOxidizer ourselves in our config. I didn't try it yet, though.

I thought we couldn't exclude bytecode generation for a specific module, at first, as the Starlark PythonPackagingPolicy interface doesn't support the Rust PythonPackagingPolicy's register_no_bytecode_module method nor its no_bytecode_modules attribute. That is, the following in our pyoxidizer.bzl could work, but won't until an update of the Starlark interface.

packaging_policy.register_no_bytecode_module("aiohappyeyeballs._staggered")

But then I realized that setting a per-resource option:

resource.add_bytecode_optimization_level_zero = False

would have the same effect, as all the internal no_bytecode_modules attribute is used for is to force the bytecode_optimization_level_* attributes to false.

Doing this for aiohappyeyeballs._staggered

diff --git a/pyoxidizer.bzl b/pyoxidizer.bzl
index f0ced4a..80186e9 100644
--- a/pyoxidizer.bzl
+++ b/pyoxidizer.bzl
@@ -103,10 +103,18 @@ def exe_resource_policy_decision(policy, resource):
     # and data resources of these packages on the filesystem as well.
     pkgs_requiring_file = ["botocore", "boto3", "docutils.parsers.rst", "docutils.writers"]
 
+    # Some modules should be excluded from bytecode compilation, e.g. because
+    # they contain syntax unsupported on our Python version (3.10) and aren't
+    # used at runtime anyway.
+    modules_no_bytecode = ["aiohappyeyeballs._staggered"]
+
     if type(resource) == "PythonModuleSource":
         if resource.name in pkgs_requiring_file or any([resource.name.startswith(p + ".") for p in pkgs_requiring_file]):
             resource.add_location = "filesystem-relative:lib"
 
+        if resource.name in modules_no_bytecode:
+            resource.add_bytecode_optimization_level_zero = False
+
     if type(resource) in ("PythonPackageResource", "PythonPackageDistributionResource"):
         if resource.package in pkgs_requiring_file or any([resource.package.startswith(p + ".") for p in pkgs_requiring_file]):
             resource.add_location = "filesystem-relative:lib"

indeed lets the build complete successfully. \o/

Another option I thought about would be constraining the version of aiohappyeyeballs to before it included the problematic module,

diff --git a/pyoxidizer-constraints.txt b/pyoxidizer-constraints.txt
new file mode 100644
index 0000000..cf55af7
--- /dev/null
+++ b/pyoxidizer-constraints.txt
@@ -0,0 +1 @@
+aiohappyeyeballs <=2.4.1
diff --git a/pyoxidizer.bzl b/pyoxidizer.bzl
index 80186e9..587a913 100644
--- a/pyoxidizer.bzl
+++ b/pyoxidizer.bzl
@@ -79,7 +79,7 @@ def make_exe():
     # `add_python_resources()` adds these objects to the binary, with a load
     # location as defined by the packaging policy's resource location
     # attributes.
-    exe.add_python_resources(exe.pip_install([NEXTSTRAIN_CLI_DIST]))
+    exe.add_python_resources(exe.pip_install(["--constraint", "pyoxidizer-constraints.txt", NEXTSTRAIN_CLI_DIST]))
 
     # For Windows, always bundle the required Visual C++ Redistributable DLLs
     # alongside the binary.¹  If they can't be found on the build machine, the

but this seems less good in the long run.

My next issue is that while the build succeeds, I run into this when I execute the binary:

$ ./build/x86_64-unknown-linux-gnu/debug/installation/nextstrain version
Traceback (most recent call last):
  File "runpy", line 187, in _run_module_as_main
  File "runpy", line 146, in _get_module_details
  File "runpy", line 110, in _get_module_details
  File "nextstrain.cli", line 21, in <module>
  File "nextstrain.cli.command", line 1, in <module>
  File "nextstrain.cli.command.deploy", line 8, in <module>
  File "nextstrain.cli.command.remote", line 28, in <module>
  File "nextstrain.cli.command.remote.upload", line 24, in <module>
  File "nextstrain.cli.remote", line 13, in <module>
  File "nextstrain.cli.remote.s3", line 52, in <module>
  File "nextstrain.cli.authn", line 20, in <module>
ImportError: cannot import name 'config' from 'nextstrain.cli.authn' (unknown location)

Sigh. I don't yet know if this is a local-me-only problem (hopefully!), or if CI would have the same issue, but I'll soon find out.

@tsibley
Copy link
Member Author

tsibley commented Sep 30, 2024

Oh jeez upstream just fixed this 40 minutes ago by releasing aiohappyeyeballs 2.4.3 which reworks (again) their "staggered" implementation and it no longer includes the ≥3.11 except* syntax. I realized this because #398 succeeded when I expected it to fail. #399 won't be necessary.

"Good" news is that whatever's giving me that ImportError I ran into locally isn't affecting CI. Not going to pull that thread today…

@tsibley tsibley closed this as completed Sep 30, 2024
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 a pull request may close this issue.

1 participant