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.

Updating affected versions. Probably affects 3.x too.

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.

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.

I mean pgen's dependency on graminit.h, not the parser's. :)

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.

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.

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.

And here's the patch for review.

Brett, any feedback on this one?

I will take a look when I can.

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.

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.

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!

This still an issue?

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.

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.

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.

Equivalent patch for 2.7

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?

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

Ah, the fact parsetok.c gets compiled twice into different object files is the detail I was overlooking. Thanks for the clarification.

Pablo, is this still an issue?

I think there is nothing missing now that the C version of pgen is gone. And parsetok is already rebuilt when gramminit.h changes:

Python/compile.o Python/symtable.o Python/ast_unparse.o Python/ast.o Python/future.o Parser/parsetok.o: $(srcdir)/Include/graminit.h           ↪ $(srcdir)/Include/Python-ast.h

Feel free to re-open if we are missing something :)