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

gh-127146: Emscripten Include compiler version in _PYTHON_HOST_PLATFORM #127992

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Dec 16, 2024

test_sysconfigdata_json is failing. This doesn't entirely fix it but it moves the failure later -- before this change it doesn't even find the sysconfigdata file because of a descrepancy between what uname() returns and what configure.ac sets _PYTHON_HOST_PLATFORM to. After, there are some small differences.

…PLATFORM

`test_sysconfigdata_json` is failing. This doesn't entirely fix it but it moves
the failure later -- before it doesn't even find the sysconfigdata file because
of a descrepancy between what `uname()` returns and what `configure.ac` sets
`_PYTHON_HOST_PLATFORM` to. After, there are some small differences.
@hoodmane
Copy link
Contributor Author

To actually fix the test, we could either skip the check for prefix-related keys or regenerate the sysconfigdata with the cli after the build is completed. The first change would look like:

@@ -651,9 +652,13 @@ def test_sysconfigdata_json(self):
         system_config_vars = get_config_vars()
 
         # Ignore keys in the check
-        for key in ('projectbase', 'srcdir'):
-            json_config_vars.pop(key)
-            system_config_vars.pop(key)
+        ignore_keys = ('projectbase', 'srcdir')
+        if is_emscripten:
+            ignore_keys += ("exec_prefix", "installed_base", "installed_platbase", "prefix", "platbase", "userbase")
+
+        for key in ignore_keys:
+            json_config_vars.pop(key, None)
+            system_config_vars.pop(key, None)
 
         self.assertEqual(system_config_vars, json_config_vars)

@freakboy3742 maybe we could do this for android and ios too?

configure.ac Outdated Show resolved Hide resolved
Co-authored-by: Kleis Auke Wolthuizen <[email protected]>
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The change itself makes sense; one note inline about formatting, and it looks like CI is failing because autoreconf hasn't been run since your most recent update.

configure.ac Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Dec 17, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@hoodmane
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Dec 17, 2024

Thanks for making the requested changes!

@freakboy3742: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from freakboy3742 December 17, 2024 09:13
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Thanks for those fixes - looks great!

@freakboy3742 freakboy3742 merged commit b92f101 into python:main Dec 17, 2024
44 of 47 checks passed
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows Server 2022 NoGIL 3.x has failed when building commit b92f101.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1241/builds/4076) and take a look at the build logs.
  4. Check if the failure is related to this commit (b92f101) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1241/builds/4076

Failed tests:

  • test_venv

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "C:\Users\Administrator\buildarea\3.x.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\test\test_venv.py", line 615, in test_multiprocessing
    out, err = check_output([self.envpy(real_env_dir=True), '-c',
               ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        'from multiprocessing import Pool; '
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        'pool = Pool(1); '
        ^^^^^^^^^^^^^^^^^^
        'print(pool.apply_async("Python".lower).get(3)); '
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        'pool.terminate()'])
        ^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Administrator\buildarea\3.x.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\test\test_venv.py", line 58, in check_output
    raise subprocess.CalledProcessError(
        p.returncode, cmd, out, err)
subprocess.CalledProcessError: Command '['C:\\Users\\Administrator\\AppData\\Local\\Temp\\test_python_sfi65_yv\\tmpjhxztw7a\\Scripts\\python3.14t_d.exe', '-c', 'from multiprocessing import Pool; pool = Pool(1); print(pool.apply_async("Python".lower).get(3)); pool.terminate()']' returned non-zero exit status 1.


Traceback (most recent call last):
  File "C:\Users\Administrator\buildarea\3.x.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\test\test_venv.py", line 615, in test_multiprocessing
    out, err = check_output([self.envpy(real_env_dir=True), '-c',
               ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        'from multiprocessing import Pool; '
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        'pool = Pool(1); '
        ^^^^^^^^^^^^^^^^^^
        'print(pool.apply_async("Python".lower).get(3)); '
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        'pool.terminate()'])
        ^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Administrator\buildarea\3.x.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\test\test_venv.py", line 58, in check_output
    raise subprocess.CalledProcessError(
        p.returncode, cmd, out, err)
subprocess.CalledProcessError: Command '['C:\\Users\\Administrator\\AppData\\Local\\Temp\\test_python_juj1et4h\\tmp36bh_36p\\Scripts\\python3.14t_d.exe', '-c', 'from multiprocessing import Pool; pool = Pool(1); print(pool.apply_async("Python".lower).get(3)); pool.terminate()']' returned non-zero exit status 1.


Traceback (most recent call last):

@freakboy3742
Copy link
Contributor

I can't see why this change would have any impact on Windows being unable to start multiprocessing...

@hoodmane
Copy link
Contributor Author

Presumably a flake =)

@hoodmane hoodmane deleted the emscripten-host-platform branch January 6, 2025 19:20
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
…PLATFORM (python#127992)

Modifies _PYTHON_HOST_PLATFORM to include the compiler version under
Emscripten. The Emscripten compiler version is the platform version
compatibility identifier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants