◐ Shell
reader mode source ↗
Skip to content

bpo-28411: Remove "modules" field from Py_InterpreterState.#1638

Merged
ericsnowcurrently merged 35 commits into
python:masterfrom
ericsnowcurrently:remove-modules-from-interpreter-state
Sep 4, 2017
Merged

bpo-28411: Remove "modules" field from Py_InterpreterState.#1638
ericsnowcurrently merged 35 commits into
python:masterfrom
ericsnowcurrently:remove-modules-from-interpreter-state

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented May 17, 2017

Copy link
Copy Markdown
Member

@mention-bot

Copy link
Copy Markdown

@ericsnowcurrently, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @serhiy-storchaka and @tim-one to be potential reviewers.

@ericsnowcurrently

Copy link
Copy Markdown
Member Author

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I like the overall change. I don't expect any major performance slowdown.

I just have a few suggestions on the implementation.

@ericsnowcurrently ericsnowcurrently force-pushed the remove-modules-from-interpreter-state branch from b073e48 to 1c472f7 Compare May 20, 2017 06:42
@serhiy-storchaka serhiy-storchaka self-requested a review May 22, 2017 11:19
@ericsnowcurrently ericsnowcurrently force-pushed the remove-modules-from-interpreter-state branch 3 times, most recently from b7d3895 to 19388d4 Compare May 25, 2017 18:31
51 hidden items Load more…
@ericsnowcurrently

Copy link
Copy Markdown
Member Author

I didn't expect the Spanish Inquisition!

@bedevere-bot

Copy link
Copy Markdown

Nobody expects the Spanish Inquisition!

@Haypo, @serhiy-storchaka: please review the changes made to this pull request.

@pitrou

pitrou commented Sep 4, 2017

Copy link
Copy Markdown
Member

Actually, I did.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

@ericsnowcurrently ericsnowcurrently merged commit 86b7afd into python:master Sep 4, 2017
@ericsnowcurrently ericsnowcurrently deleted the remove-modules-from-interpreter-state branch September 4, 2017 23:54
jimmylai pushed a commit to jimmylai/cpython that referenced this pull request Sep 5, 2017
* 'master' of https://github.com/python/cpython: (32 commits)
  Conceptually, roots is a set.  Also searching it as a set is a tiny bit faster (python#3338)
  bpo-31343: Include sys/sysmacros.h (python#3318)
  bpo-30102: Call OPENSSL_add_all_algorithms_noconf (python#3112)
  Prevent a few make suspicious warnings. (python#3341)
  Include additional changes to support blurbified NEWS (python#3340)
  Simplify NEWS entry to prevent suspicious warnings. (python#3339)
  bpo-31347: _PyObject_FastCall_Prepend: do not call memcpy if args might not be null (python#3329)
  Revert "bpo-17852: Maintain a list of BufferedWriter objects.  Flush them on exit. (python#1908)" (python#3337)
  bpo-17852: Maintain a list of BufferedWriter objects.  Flush them on exit. (python#1908)
  Fix terminology in comment and add more design rationale. (python#3335)
  Add comment to explain the implications of not sorting keywords (python#3331)
  bpo-31170: Update libexpat from 2.2.3 to 2.2.4 (python#3315)
  bpo-28411: Remove "modules" field from Py_InterpreterState. (python#1638)
  random_triangular:  sqrt() is more accurate than **0.5 (python#3317)
  Travis: use ccache (python#3307)
  remove IRIX support (closes bpo-31341) (python#3310)
  Code clean-up.  Remove unnecessary pre-increment before the loop starts. (python#3312)
  Regen Moduls/clinic/_ssl.c.h (pythonGH-3320)
  bpo-30502: Fix handling of long oids in ssl. (python#2909)
  Cache externals, depending on changes to PCbuild (python#3308)
  ...
GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017
@ericsnowcurrently ericsnowcurrently restored the remove-modules-from-interpreter-state branch September 14, 2017 06:04
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Sep 14, 2017
ericsnowcurrently added a commit that referenced this pull request Sep 14, 2017
…3565)

PR #1638, for bpo-28411, causes problems in some (very) edge cases. Until that gets sorted out, we're reverting the merge. PR #3506, a fix on top of #1638, is also getting reverted.
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Sep 14, 2017
A bunch of code currently uses PyInterpreterState.modules directly instead of PyImport_GetModuleDict(). This complicates efforts to make changes relative to sys.modules. This patch switches to using PyImport_GetModuleDict() uniformly. Also, a number of related uses of sys.modules are updated for uniformity for the same reason.

Note that this code was already reviewed and merged as part of python#1638. I reverted that and am now splitting it up into more focused parts.
@hroncok

hroncok commented May 11, 2018

Copy link
Copy Markdown
Contributor

Maybe just mention the change in https://docs.python.org/dev/whatsnew/3.7.html#porting-to-python-3-7

What this ever put there? I've juts been bit by the changed API of _PyImport_FixupBuiltin (in gdb).

https://bugzilla.redhat.com/show_bug.cgi?id=1577396

@vstinner

Copy link
Copy Markdown
Member

@hroncok: Would you mind to open a new issue at bugs.python.org? This PR has been closed since last September, so people will unlikely notice your comment :-(

@hroncok

hroncok commented May 12, 2018

Copy link
Copy Markdown
Contributor

Sure, i was just checking. Here it is: https://bugs.python.org/issue33470

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.

9 participants