Skip to content

bpo-43372: Re-generate frozen code for __hello__. #24708

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

Closed
wants to merge 5 commits into from

Conversation

nascheme
Copy link
Member

@nascheme nascheme commented Mar 2, 2021

The marshaled data for code objects has changed with bpo-42246, commit
877df85. Update expected code sizes in ctypes test_frozentable.

https://bugs.python.org/issue43372

@hroncok
Copy link
Contributor

hroncok commented Mar 2, 2021

The Check if generated files are up to date CI appears to run regen-all with an older Python version.

@hroncok
Copy link
Contributor

hroncok commented Mar 2, 2021

python3 ./Tools/freeze/regen_frozen.py ./Python/frozen_hello.h

That is Python 3.9.1

@hroncok
Copy link
Contributor

hroncok commented Mar 2, 2021

See nascheme#2 -- I think that should do.

@nascheme
Copy link
Member Author

nascheme commented Mar 2, 2021

See nascheme#2 -- I think that should do.

Doesn't seem to work, unfortunately:

./python ./Python/makeopcodetargets.py \
	./Python/opcode_targets.h.new
./python: error while loading shared libraries: libpython3.10d.so.1.0: cannot open shared object file: No such file or directory

That CI test is broken, IMHO. I wonder is we should spilt fixing the CI into a separate PR. Sort of a chicken and egg problem I guess since each PR will fail unless both applied.

@hroncok
Copy link
Contributor

hroncok commented Mar 2, 2021

My attempt fails with:

./python: error while loading shared libraries: libpython3.10d.so.1.0: cannot open shared object file: No such file or directory

@nascheme
Copy link
Member Author

nascheme commented Mar 2, 2021

Another option might be to disable those CI tests until we can fix them. They were added in this change, it seems:

commit d20b7ed9c1fabac3fdebb7ec362fe4f022a54639
Author: Filipe Laíns <lains@archlinux.org>
Date:   Fri Nov 20 14:14:16 2020 +0000

    [bpo-42212](https://bugs.python.org/issue42212): Check if generated files are up-to-date in GitHub Actions (GH-23042)
    
    See https: //github.com/python/core-workflow/issues/380
    
    Signed-off-by: Filipe Laíns <lains@archlinux.org>

Using some different version of Python to regen stuff does not seem safe to me. It just happened to mostly work until now. Using the freshly build Python (i.e. ./python) seems like the correct approach but we have to figure out the correct incantation. I don't know enough about the CI setup to know what's wrong with your change.

@nascheme
Copy link
Member Author

nascheme commented Mar 2, 2021

@hroncok it seems the issue is the --enable-shared. If we set LD_LIBRARY_PATH to . then ./python can run. The change to add the --enable-shared was done by Victor in ac7d016 so I suspect it is needed. We might be able to fix it with just:

PYTHON_FOR_REGEN=./python LD_LIBRARY_PATH=. make -j4 regen-all

I'm going to try updating my PR and hope the github CI rules work.

@hroncok
Copy link
Contributor

hroncok commented Mar 3, 2021

It passed. Thanks!

@nascheme
Copy link
Member Author

nascheme commented Mar 3, 2021

It seems all is working now. The failure in "Tests / Ubuntu" seems unrelated to these changes (SSL test failed).

Something I noticed: it seems inconsistent how regen-stdlib-module-names works compared to the rest of the regen-all targets. The others use PYTHON_FOR_REGEN but regen-stdlib-module-names uses $(RUNSHARED) ./$(BUILDPYTHON). Maybe not a big deal but seems strange to me. Either we could change regen-frozen to use the build Python or maybe we should change them all to use PYTHON_FOR_REGEN. Perhaps I should ask Victor.

@nascheme
Copy link
Member Author

nascheme commented Mar 3, 2021

Marking as "DO-NOT-MERGE". I think my other PR is a cleaner fix. Assuming that one is okay, we can close this one.

@nascheme
Copy link
Member Author

nascheme commented Mar 5, 2021

Closing since this is not the best approach. #24759 seems better.

@nascheme nascheme closed this Mar 5, 2021
@vstinner
Copy link
Member

vstinner commented Mar 8, 2021

Something I noticed: it seems inconsistent how regen-stdlib-module-names works compared to the rest of the regen-all targets. The others use PYTHON_FOR_REGEN but regen-stdlib-module-names uses $(RUNSHARED) ./$(BUILDPYTHON).

It's not a bug, but it's made on purpose. For example, if you use Python 3.8 as PYTHON_FOR_REGEN, using PYTHON_FOR_REGEN to build the "stdlib module names" list would be wrong (Python 3.8 has different/less stdlib modules). You really must use the just built Python to build this list.

Do you think that a comment is required in Makefile.pre.in to prevent future mistakes?

@vstinner
Copy link
Member

vstinner commented Mar 8, 2021

As you noticed, Travis CI and the GitHub Actions use two steps. Example from .travis.yml:

  - PYTHON_FOR_REGEN=python3.8 make -j4 regen-all
  - make regen-stdlib-module-names
  • (1) build Python
  • (2) run "make regen-stdlib-module-names" on the newly built Python

At least, running "make regen-all" displays this message:

Note: make regen-stdlib-module-names and autoconf should be run manually

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.

5 participants