Skip to content

bpo-45866: pegen strips directory of "generated from" header #29777

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 26, 2021
Merged

bpo-45866: pegen strips directory of "generated from" header #29777

merged 1 commit into from
Nov 26, 2021

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 25, 2021 •
edited by bedevere-bot
Loading

"make regen-all" now produces the same output when run from a
directory other than the source tree: when building Python out of the
source tree.

https://bugs.python.org/issue45866

@vstinner
Copy link
Member Author

cc @pablogsal

@vstinner
bpo-45866: pegen strips directory of "generated from" header
0ef701f 
"make regen-all" now produces the same output when run from a
directory other than the source tree: when building Python out of the
source tree.
@hroncok
Copy link
Contributor

hroncok commented Nov 25, 2021

For the record, this fixes the minor problem of the dirty working tree when the files are regenerated from out-of-tree, but it does not fix the missing _freeze_module error.

arhadthedev
arhadthedev reviewed Nov 25, 2021
View reviewed changes
Tools/peg_generator/pegen/c_generator.py
@@ -416,7 +417,8 @@ def out_of_memory_goto(self, expr: str, goto_target: str) -> None:

def generate(self, filename: str) -> None:
self.collect_rules()
self.print(f"// @generated by pegen from {filename}")
basename = os.path.basename(filename)
Copy link
Member

Choose a reason for hiding this comment

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

Can we use pathlib instead of os.path?

from pathlib import Path
# [...]
        basename = Path(filename).name

Copy link
Member

Choose a reason for hiding this comment

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

pathlib has a much more complex import tree and relies on more modules. os.path has the benefit that it works with a minimal interpreter at an early stage of the bootstrapping process.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, pathlib can be used, but I prefer os.path :-) For this specific change, I don't think that pathlib is much better.

arhadthedev
arhadthedev reviewed Nov 25, 2021
View reviewed changes
Tools/peg_generator/pegen/python_generator.py
@@ -212,7 +213,8 @@ def generate(self, filename: str) -> None:
self.collect_rules()
header = self.grammar.metas.get("header", MODULE_PREFIX)
if header is not None:
self.print(header.rstrip("\n").format(filename=filename))
basename = os.path.basename(filename)
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@vstinner
Copy link
Member Author

For the record, this fixes the minor problem of the dirty working tree when the files are regenerated from out-of-tree, but it does not fix the missing _freeze_module error.

Right, I prefer to write a separated change for the other issue.

@pablogsal
Copy link
Member

LGTM

I liked that the file shows the full path so people can easily find the file, but I understand the downside so I am ok with this PR

@vstinner vstinner merged commit 253b7a0 into python:main Nov 26, 2021
@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @vstinner, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 253b7a0a9fef1d72a4cb87b837885576e68e917c 3.10

@vstinner vstinner deleted the pegen_basename branch November 26, 2021 10:50
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Nov 26, 2021
@bedevere-bot
Copy link

GH-29792 is a backport of this pull request to the 3.10 branch.

vstinner added a commit that referenced this pull request Nov 26, 2021
@vstinner
bpo-45866: pegen strips directory of "generated from" header (GH-29777)…
b6defde 
… (GH-29792)

"make regen-all" now produces the same output when run from a
directory other than the source tree: when building Python out of the
source tree.

(cherry picked from commit 253b7a0)
vstinner added a commit that referenced this pull request Nov 26, 2021
@vstinner
bpo-45866: pegen strips directory of "generated from" header (GH-29777)…
93a540d 
… (GH-29792) (GH-29797)

"make regen-all" now produces the same output when run from a
directory other than the source tree: when building Python out of the
source tree.

(cherry picked from commit 253b7a0)
(cherry picked from commit b6defde)
remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021
@vstinner @remykarem
bpo-45866: pegen strips directory of "generated from" header (pythonG…
af11d9a 
…H-29777)

"make regen-all" now produces the same output when run from a
directory other than the source tree: when building Python out of the
source tree.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiran tiran tiran left review comments

@arhadthedev arhadthedev arhadthedev left review comments

@pablogsal pablogsal pablogsal approved these changes

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

Assignees

@vstinner vstinner

Labels
awaiting merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@vstinner @hroncok @pablogsal @miss-islington @bedevere-bot @tiran @arhadthedev @the-knights-who-say-ni