Skip to content
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.

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

regen.vcxproj cannot regenerate some necessary files #87733

Closed
pjx206 mannequin opened this issue Mar 20, 2021 · 22 comments
Closed

regen.vcxproj cannot regenerate some necessary files #87733

pjx206 mannequin opened this issue Mar 20, 2021 · 22 comments
Assignees
Labels
3.10 only security fixes build The build process and cross-build OS-windows

Comments

@pjx206
Copy link
Mannequin

pjx206 mannequin commented Mar 20, 2021

BPO 43567
Nosy @gvanrossum, @zooba, @tonybaloney, @pablogsal, @tonybaloney, @pjx206
PRs
  • bpo-43567: Improved generated code refresh on Windows #25120
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/zooba'
    closed_at = <Date 2021-04-06.22:57:11.973>
    created_at = <Date 2021-03-20.11:02:18.427>
    labels = ['build', '3.10', 'OS-windows']
    title = 'regen.vcxproj cannot regenerate some necessary files'
    updated_at = <Date 2021-04-06.22:57:11.972>
    user = 'https://github.com/pjx206'

    bugs.python.org fields:

    activity = <Date 2021-04-06.22:57:11.972>
    actor = 'steve.dower'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2021-04-06.22:57:11.973>
    closer = 'steve.dower'
    components = ['Build', 'Windows']
    creation = <Date 2021-03-20.11:02:18.427>
    creator = 'pjx206'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 43567
    keywords = ['patch']
    message_count = 22.0
    messages = ['389155', '389743', '389765', '389766', '389770', '389778', '389781', '389835', '389841', '389863', '389866', '389868', '389870', '389880', '389924', '389927', '389929', '389935', '389954', '390037', '390383', '390384']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'steve.dower', 'anthonypjshaw', 'pablogsal', 'anthony shaw', 'pjx206']
    pr_nums = ['25120']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'compile error'
    url = 'https://bugs.python.org/issue43567'
    versions = ['Python 3.10']

    @pjx206
    Copy link
    Mannequin Author

    pjx206 mannequin commented Mar 20, 2021

    I tried to modify Grammar/python.gram, Grammar/Tokens, Parser/Python.asdl, to add a new token to the grammar. And when using build.bat --regen, only parser.c is newly generated. Other files, as mentioned in https://devguide.python.org/grammar/ that should be updated:ast.c, Python-ast.h etc.
    regen.vcxproj is now not capable for the new PEG parser. So an update is need.

    @pjx206 pjx206 mannequin added 3.10 only security fixes build The build process and cross-build labels Mar 20, 2021
    @gvanrossum
    Copy link
    Member

    Steve: let's talk about this in person today.

    @pablogsal
    Copy link
    Member

    regen.vcxproj is now not capable for the new PEG parse

    Hummmm, that file should handle the peg parser already:

    <Target Name="_RegenPegen" BeforeTargets="Build">
    <!-- Regenerate Parser/parser.c -->
    <SetEnv Name="PYTHONPATH" Prefix="true" Value="$(PySourcePath)Tools\peg_generator\" />
    <Exec Command="&quot;$(PythonExe)&quot; -m pegen -q c &quot;$(PySourcePath)Grammar\python.gram&quot; &quot;$(PySourcePath)Grammar\Tokens&quot; -o &quot;$(IntDir)parser.c&quot;" />
    <Copy SourceFiles="$(IntDir)parser.c" DestinationFiles="$(PySourcePath)Parser\parser.c">
    <Output TaskParameter="CopiedFiles" ItemName="_UpdatedParse" />
    </Copy>
    <Warning Text="Pegen updated. You will need to rebuild pythoncore to see the changes." Condition="'@(_UpdatedParse)' != ''" />

    @vstinner
    Copy link
    Member

    By the way, it would be nice if someone could review my change in this project related to pycore_ast.h ;-) (commit 94faa07) I mean checking if pycore_ast.h is updated as expected.

    @gvanrossum
    Copy link
    Member

    I've found build.bat --regen unreliable myself. I went over it with Steve and one issue that came up is that it uses the python.exe that is built to run the code generation scripts; OTOH on Linux/Mac these scripts are run using a suitable pre-existing Python binary, e.g. a recent enough system Python. (However, the frozen files require a binary (_freeze_importlib).

    It will take some time to refactor things so that this is more usable.

    Perhaps instead of (or in addition to) doing everything via .vcxproj files, we could have a simple(r) regen.bat script that runs all the regen scripts (or at least the non-freeze ones) using the system Python?

    FWIW my own issues weren't with the parser, they were with regenerating opcode.h and opcode_targets.h.

    @gvanrossum
    Copy link
    Member

    Workaround:

    For opcode.py changes, I can probably use the following strategy:

    • build.bat
    • Add new opcode to opcode.py
    • build.bat --regen
    • build.bat
    • Make changes to compile.c and ceval.c to implement new opcode
    • build.bat

    For the parser perhaps you can adopt a similar strategy, where you first
    add new tokens and get the code generation working?

    @pjx206
    Copy link
    Mannequin Author

    pjx206 mannequin commented Mar 30, 2021

    I did:

    I agree that a regen.bat that using a system python is better solution. regen.vcxproj is confusing.

    @gvanrossum
    Copy link
    Member

    Can you past the full output from your command?

    Also, try making a dummy change (e.g. add a blank line) to each of the input files -- this will trigger the rebuild.

    PS. Please don't remove nosy folks unless they request so.

    @gvanrossum
    Copy link
    Member

    @Steve, Anthony: I am now in a state where I have *no* changes anywhere, build.bat completes without errors, but build.bat --regen produces this error (after first doing a regular build):

    C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\VC\VCTargets\Microsoft.Cpp.Platform.targets(67,5): error MSB8020: The build to
    ols for v142 (Platform Toolset = 'v142') cannot be found. To build using the v142 build tools, please install v142 build tools. Alternatively, you may u
    pgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C:\Users\gv
    anrossum\cpython\PCbuild\regen.vcxproj]

    Help!

    @gvanrossum
    Copy link
    Member

    FWIW, could this issue recently merged by Victor be related?
    bpo-43244: Remove ast.h, asdl.h, Python-ast.h headers (GH-24933)

    @pjx206
    Copy link
    Mannequin Author

    pjx206 mannequin commented Mar 31, 2021

    build.bat --regen produces such error because it uses build tool from visual studio to build regen.vcxproj, and you do not have v142 toolset. I'm using vs2019 and have v142 toolset installed and regen.vcxproj workd, just cannot actually regenerate files needed completely.

    @gvanrossum
    Copy link
    Member

    So is there a bug in regen.vcxproj, or in https://devguide.python.org/setup/#windows-compiling ? The latter says VS 2017.

    @pjx206: please stop clearing out the nosy list. You've done that twice now. If you do it again, I'll close this issue.

    @pjx206
    Copy link
    Mannequin Author

    pjx206 mannequin commented Mar 31, 2021

    The regen.vcxproj was modified but not test well. In the first version of regen.vcxproj there were such build targets:

    _RegenGrammar(BeforeTargets="Build") -> _RegenAST_H -> _RegenAST_C -> _RegenOpcodes -> _RegenTokens -> _RegenKeywords -> _RegenSymbols

    In commit 1ed83ad: bpo-40939: Remove the old parser (GH-20768),
    "_RegenGrammar" was removed, but there's still "_RegenAST_H" as AfterTargets of "_RegenGrammar".

    @gvanrossum sorry about removing nosy folks, This website displays in my language so I did not realize what is "nosy list".

    @vstinner
    Copy link
    Member

    @gvanrossum
    Copy link
    Member

    Anthony, can you help us out here? Is there a problem with regen.vcsproj, either by depending on VS 2019 (which we don't officially support yet) or by missing the changes in the names of some generated files?

    @zooba
    Copy link
    Member

    zooba commented Mar 31, 2021

    I've attached my PR that streamlines this, it should be good (as in, reliable/fast) to do the regen on every build, but the "build.bat --regen" command is retained to force it.

    @gvanrossum
    Copy link
    Member

    @jiaxin Please test steve's PR.

    @tonybaloney
    Copy link
    Mannequin

    tonybaloney mannequin commented Mar 31, 2021

    Guido, regen.vcxproj targets 142 as the SDK version, which is most likely a mistake.
    The other projects are part of the main PCBuild.sln solution, which has a variable for the base SDK version.

    If you need to change it quickly, you can either open it in VS and right click on the project and change the properties to an older SDK, or edit the vcxproj file itself.

    I can make this change in a patch, but because its a standalone project, it doesn't share the base SDK version with the others.

    I've reviewed Steve's patch and it would fix this because it changes regen into a build target instead of a build project, so it doesn't specify the SDK version at all.
    Steve's implementation is also much, much cleaner!

    @pjx206
    Copy link
    Mannequin Author

    pjx206 mannequin commented Apr 1, 2021

    Steve's PR zooba:bpo-43567 works as expected. Really clean implementation!
    Thanks!

    @gvanrossum
    Copy link
    Member

    Jiaxin: "Resolution: works for me" is meant for the triager to indicate that there is no bug, or at least that the repro given in the bug report doesn't trigger the bug in the triager's environment. It does *not* mean that a patch works for the submitter of the issue. But thanks for letting us know that Steve's patch works for you! :-)

    @zooba
    Copy link
    Member

    zooba commented Apr 6, 2021

    New changeset 7482838 by Steve Dower in branch 'master':
    bpo-43567: Improved generated code refresh on Windows (GH-25120)
    7482838

    @zooba
    Copy link
    Member

    zooba commented Apr 6, 2021

    We're updating all files now, and doing it automatically on build (or manually with --regen).

    @zooba zooba closed this as completed Apr 6, 2021
    @zooba zooba self-assigned this Apr 6, 2021
    @zooba zooba closed this as completed Apr 6, 2021
    @zooba zooba self-assigned this Apr 6, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes build The build process and cross-build OS-windows
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants