classification
Title: Circular dependency causes SystemError when adding new syntax
Type: behavior Stage: patch review
Components: Build Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: open Resolution: wont fix
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, martin.panter, thomaslee
Priority: normal Keywords: patch

Created on 2008-11-18 15:28 by thomaslee, last changed 2016-11-22 17:32 by brett.cannon.

Files
File name Uploaded Description Edit
graminit-dependencies.patch thomaslee, 2008-11-18 15:28 Patch to ensure dependencies of graminit.h are rebuilt if the file is regenerated
build-evilness-fix.patch thomaslee, 2008-12-02 07:45 Updated patch
build-evilness-fix-02.patch thomaslee, 2008-12-02 07:59 Yet another updated patch.
build-evilness-fix-03.patch thomaslee, 2008-12-03 05:00 Build fix including generated files.
build-evilness-fix-03-review.patch thomaslee, 2008-12-03 05:00 Patch for review.
graminit-dep.patch martin.panter, 2016-11-05 02:57 #ifndef PGEN for 3.5+ review
graminit-dep.py2.patch martin.panter, 2016-11-21 04:59 review
Messages (22)
msg76010 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2008-11-18 15:28
It's important that dependencies of grammar.h get rebuilt if graminit.h
is regenerated (e.g. the Grammar is modified). If these dependencies do
not get rebuilt, the constants associated with each type of parse node
will have inconsistent values between the different intermediate files.

The net result is that a program afflicted by this might build without
errors, but then crash unexpectedly at runtime due to the inconsistent
constant values.

The patch is quite simple and ensures that all files that currently
depend on graminit.h are rebuilt if it changes.

It also removes an unnecessary #include from Python/future.c.

I believe a similar situation might occur with Python-ast.h and the
*_kind enumerations, but have yet to run into such a specific issue.
I'll post a separate patch if I do find this to be a problem.
msg76050 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2008-11-19 14:21
Updating affected versions. Probably affects 3.x too.
msg76727 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2008-12-02 07:12
A deeper issue here is that Parser/parsetok.c has a dependency on
graminit.h. The problem is that Parser/parsetok.c is a part of the
Parser/pgen program which is actually being used to *generate*
graminit.h in the first place.

This breaks Python whenever syntax is added to or removed from
Grammar/Grammar in such a way that the value of encoding_decl changes,
because the value of encoding_decl used by pgen is different to the
value used to build python itself.

A simple work around for those wishing to change the syntax is a "make;
make clean; make". It'd obviously be nice if the build were fixed, though.
msg76728 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2008-12-02 07:45
Here's a new patch that I believe should fix this issue.

It modifies Makefile.pre.in to include a few additional dependency
declarations for source files that depend on Include/graminit.h and
Include/Python-ast.h, as well as moving encoding_decl to the top of
Grammar/Grammar.

This should ensure that changes to the syntax and/or AST nodes will not
cause cryptic errors.

Longer term, it would be great to remove the parser's dependency on
graminit.h to ensure this sort of problem is never an issue.
msg76729 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2008-12-02 07:46
I mean pgen's dependency on graminit.h, not the parser's. :)
msg76730 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2008-12-02 07:59
okay, so it turns out that putting rules in Grammar/Grammar before the
top-level non-terminals breaks things in a different way.

Attached is another (hopefully final and working) patch.
msg76762 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-12-02 18:01
Because of all the patch includes a bunch of junk for generated files
(any chance you can make a diff, Thomas, with only the stuff that really
requires review?), I have not done a real review. But a quick look does
show that the comment added to Grammar needs to be tweaked. Please use
complete sentences, including punctuation (e.g., capitalize words, end
in a period, etc.). Also don't use XXX just to grab attention for
something that is not meant to be removed.
msg76813 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2008-12-03 05:00
Thanks for the review Brett, apologies for the mess.

I'm attaching two new patches -- one for review, the other containing
the generated files. These differ very slightly from the original patch
-- mainly just removing some stuff I don't think is necessary.

You're probably aware of this, but it's important for the changes to the
generated files to be checked in too -- likewise for testing the patch.
msg76814 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2008-12-03 05:00
And here's the patch for review.
msg80075 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2009-01-18 04:08
Brett, any feedback on this one?
msg80080 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2009-01-18 06:42
I will take a look when I can.
msg81126 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2009-02-04 03:33
So I finally got around to reviewing the patch and while it looks fine,
I ended up with some failing tests: test_compiler test_quopri test_sys
test_transformer.

Do you know what is going on Thomas? This is after repeated make/make
clean calls and using your non-review patch with all of the
auto-generated changes included.
msg81452 - (view) Author: Thomas Lee (thomaslee) (Python committer) Date: 2009-02-09 09:10
This would appear to be another build quirk: Lib/symbol.py needs to be
regenerated if Grammar/Grammar changes.

Brett, do you think it would be okay for this file to be generated
automatically as part of the build process? I can't think of any good
reason why this should continue to be a manual task.

Lib/token.py is in a similar boat, except its dependency is on
Include/token.h

Whatever the decision, I'll provide another review/functional patch pair.
msg81469 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2009-02-09 18:51
On Mon, Feb 9, 2009 at 01:11, Thomas Lee <report@bugs.python.org> wrote:
>
> Thomas Lee <tom@vector-seven.com> added the comment:
>
> This would appear to be another build quirk: Lib/symbol.py needs to be
> regenerated if Grammar/Grammar changes.
>
> Brett, do you think it would be okay for this file to be generated
> automatically as part of the build process? I can't think of any good
> reason why this should continue to be a manual task.
>

It should be a build rule and not be a manual step.

> Lib/token.py is in a similar boat, except its dependency is on
> Include/token.h
>

Same answer.

> Whatever the decision, I'll provide another review/functional patch pair.

Thanks!
msg175771 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-11-17 16:51
This still an issue?
msg180098 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-01-16 18:45
Since this has been sitting here for two months as pending I'm closing as won't fix since mucking with the grammar is such a rarity and getting the build rules right is so complicated it isn't worth changing.
msg279489 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-26 04:32
I occasionally get the following error, due to Parser/parsetok.o being older than Include/graminit.h.

./python -E -S -m sysconfig --generate-posix-vars ; if test $? -ne 0 ; then  echo "generate-posix-vars failed" ;  rm -f ./pybuilddir.txt ;  exit 1 ;  fi
Could not import runpy module
Traceback (most recent call last):
  File "/home/proj/python/cpython/Lib/runpy.py", line 14, in <module>
    import importlib.machinery # importlib first so we can test #15386 via -m
  File "/home/proj/python/cpython/Lib/importlib/__init__.py", line 57, in <module>
    import types
  File "/home/proj/python/cpython/Lib/types.py", line 166, in <module>
    import functools as _functools
  File "/home/proj/python/cpython/Lib/functools.py", line 345, in <module>
    _CacheInfo = namedtuple("CacheInfo", ["hits", "misses", "maxsize", "currsize"])
  File "/home/proj/python/cpython/Lib/collections/__init__.py", line 428, in namedtuple
    exec(class_definition, namespace)
SystemError: invalid node 339 for PyAST_FromNode
generate-posix-vars failed
*** Error code 1

The best workaround for me (less brute force than removing the whole build tree) seems to be to add Parser/parsetok.c to the list of files to “touch” the timestamps of before building.

The dependency in Parser/parsetok.c on Include/graminit.h was added by <https://hg.python.org/cpython/diff/06f1e2fd05bc/Parser/parsetok.c>, presumably to support encoding declarations in Python source files. I haven’t tried, but perhaps to avoid pgen depending on its output, a quick fix would be to add #ifndef PGEN around the offending code. For Python 2, a Parser/parsetok_pgen.c wrapper file would have to added, like in revision 6e9dc970ac0e.
msg280101 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-11-05 02:57
Here is a patch for Python 3 implementing my idea. There is already code conditionally compiled out for the pgen build, so my patch just expands that to avoid the "graminit.h" include and derived functions.
msg281321 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-11-21 04:59
Equivalent patch for 2.7
msg281387 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-11-21 18:57
So what does having the tokenizer not depend on pgen accomplish? Does it break a circular dependency where the tokenizer will get rebuilt with pgen before a full build starts?
msg281458 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-11-22 10:16
I’m not sure I understand your questions Brett (Which tokenizer? What rebuilding?), but I will try to explain some relevant parts.

My main goal is to add a makefile dependency of parsetok.o on $(GRAMMAR_H). In Python 3, that is easy, and (I realize now) my problem would be solved without changing any other file. However my changes in parsetok.c eliminate an unnecessary bootstrapping problem, so I still think they are worthwhile.

For Python 3, the parsetok.c code gets compiled to parsetok_pgen.o, and after executing, also gets compiled to a separate parsetok.o file. Rebuilding the code is not a problem here.

In Python 2, the same parsetok.o object is both part of the pgen program, and the eventual Python program. In order to add my desired makefile dependency, I have to duplicate parsetok.c compilation into two makefile rules (parsetok.o and parsetok_pgen.o). So you could say I am breaking a circle _by_ rebuilding the code.

Dependencies between files with my patch applied:

Parser/parsetok_pgen.o -> Parser/pgen -> Include/graminit.h -> Parser/parsetok.o -> python
msg281493 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-11-22 17:32
Ah, the fact parsetok.c gets compiled twice into different object files is the detail I was overlooking. Thanks for the clarification.
History
Date User Action Args
2016-11-22 17:32:11brett.cannonsetmessages: + msg281493
2016-11-22 10:16:14martin.pantersetmessages: + msg281458
2016-11-21 18:57:50brett.cannonsetmessages: + msg281387
2016-11-21 04:59:31martin.pantersetfiles: + graminit-dep.py2.patch

messages: + msg281321
2016-11-05 02:57:11martin.pantersetstatus: closed -> open
files: + graminit-dep.patch
versions: + Python 3.5, Python 3.6, Python 3.7, - Python 2.6
messages: + msg280101

stage: needs patch -> patch review
2016-10-26 04:32:38martin.pantersetnosy: + martin.panter
messages: + msg279489
2013-01-16 18:45:37brett.cannonsetstatus: pending -> closed
resolution: wont fix
messages: + msg180098
2012-11-17 16:51:34brett.cannonsetstatus: open -> pending

messages: + msg175771
2009-04-02 04:14:17brett.cannonsetassignee: brett.cannon ->
2009-02-11 03:03:30brett.cannonsetpriority: normal
stage: patch review -> needs patch
2009-02-09 18:51:13brett.cannonsetmessages: + msg81469
2009-02-09 09:10:59thomasleesetmessages: + msg81452
2009-02-04 03:33:29brett.cannonsetmessages: + msg81126
2009-01-18 06:42:22brett.cannonsetassignee: brett.cannon
messages: + msg80080
2009-01-18 04:08:17thomasleesetmessages: + msg80075
2008-12-03 05:00:39thomasleesetfiles: + build-evilness-fix-03-review.patch
messages: + msg76814
2008-12-03 05:00:11thomasleesetfiles: + build-evilness-fix-03.patch
messages: + msg76813
2008-12-02 18:01:51brett.cannonsetnosy: + brett.cannon
messages: + msg76762
stage: patch review
2008-12-02 07:59:11thomasleesetfiles: + build-evilness-fix-02.patch
messages: + msg76730
2008-12-02 07:46:19thomasleesetmessages: + msg76729
2008-12-02 07:45:21thomasleesetfiles: + build-evilness-fix.patch
messages: + msg76728
title: Dependencies of graminit.h are not rebuilt when the file is regenerated -> Circular dependency causes SystemError when adding new syntax
2008-12-02 07:12:19thomasleesetmessages: + msg76727
2008-11-19 14:21:07thomasleesetmessages: + msg76050
versions: + Python 2.7
2008-11-18 15:28:11thomasleecreate