Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in

#22359: Remove incorrect uses of recursive make

Can't Edit
Can't Publish+Mail
Start Review
5 years, 9 months ago by jonas.wagner
4 years, 1 month ago
loewis, brett.cannon, doko, AntoinePitrou, Arfrever, xdegaye, devnull_psf.upfronthosting.co.za, Martin Panter, koobs, jonas.wagner_epfl.ch, WanderingLogic

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Total comments: 2

Patch Set 5 #

Patch Set 6 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
configure View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
configure.ac View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
Makefile.pre.in View 1 2 3 4 5 3 chunks +20 lines, -7 lines 0 comments Download


Total messages: 1
Martin Panter
4 years, 1 month ago #1
File Makefile.pre.in (right):

Makefile.pre.in:272: GRAMMAR_H=	$(srcdir)/Include/graminit.h
I understand these are placed in the output directory on purpose. It would be
good to avoid messing with the source files in a normal build (especially if
they are read-only). This could do with a comment though. If you look at the
history, this has switched back and forth too many times:

https://hg.python.org/cpython/rev/740290a56190 (support VPATH)
https://hg.python.org/cpython/rev/c9f23f80fc93 (generate in build dir)
https://hg.python.org/cpython/rev/67ed8a6905c3 (generate in srcdir but ignore
https://hg.python.org/cpython/rev/2a773bef40c2 (recognize error)
https://hg.python.org/cpython/rev/62044cd5b19b (generate in build dir)

I guess you changed this because the rules do not create these files any more in
cross-compile mode. Non-phony rules should generate their target files, so in
this case I think you should copy or link the files into the build directory if
they are not already there.

Makefile.pre.in:726: $(srcdir)/Lib/importlib/_bootstrap_external.py
Python/importlib_external.h; \
There is a second tab as indentation here. I wonder if it would be better to
change it to spaces (the line above uses spaces)?
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+