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

MSBuild Extensions in CPython NuGet Package has Bad Expression #84339

Closed
commonsensesoftware mannequin opened this issue Apr 2, 2020 · 7 comments
Closed

MSBuild Extensions in CPython NuGet Package has Bad Expression #84339

commonsensesoftware mannequin opened this issue Apr 2, 2020 · 7 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes build The build process and cross-build OS-windows stdlib Python modules in the Lib dir topic-installation

Comments

@commonsensesoftware
Copy link
Mannequin

commonsensesoftware mannequin commented Apr 2, 2020

BPO 40158
Nosy @pfmoore, @tjguk, @merwok, @zware, @zooba, @dstufft, @miss-islington, @commonsensesoftware
PRs
  • bpo-40158: Fix CPython MSBuild Properties in NuGet Package #19343
  • [3.8] bpo-40158: Fix CPython MSBuild Properties in NuGet Package (GH-19343) #19349
  • [3.7] bpo-40158: Fix CPython MSBuild Properties in NuGet Package (GH-19343) #19350
  • 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 = None
    closed_at = <Date 2020-04-03.20:05:25.121>
    created_at = <Date 2020-04-02.18:00:31.787>
    labels = ['3.7', 'expert-installation', '3.9', '3.8', 'build', 'library', 'OS-windows']
    title = 'MSBuild Extensions in CPython NuGet Package has Bad Expression'
    updated_at = <Date 2020-04-03.22:20:16.321>
    user = 'https://github.com/commonsensesoftware'

    bugs.python.org fields:

    activity = <Date 2020-04-03.22:20:16.321>
    actor = 'steve.dower'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-04-03.20:05:25.121>
    closer = 'steve.dower'
    components = ['Build', 'Demos and Tools', 'Distutils', 'Installation', 'Windows']
    creation = <Date 2020-04-02.18:00:31.787>
    creator = 'sydefekt'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 40158
    keywords = ['patch']
    message_count = 7.0
    messages = ['365610', '365613', '365647', '365685', '365721', '365732', '365733']
    nosy_count = 8.0
    nosy_names = ['paul.moore', 'tim.golden', 'eric.araujo', 'zach.ware', 'steve.dower', 'dstufft', 'miss-islington', 'sydefekt']
    pr_nums = ['19343', '19349', '19350']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'compile error'
    url = 'https://bugs.python.org/issue40158'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @commonsensesoftware
    Copy link
    Mannequin Author

    commonsensesoftware mannequin commented Apr 2, 2020

    CPython provides a NuGet package as a mechanism to support non-installed Python distributions. The package includes MSBuild support to integrate with its build process.

    The expressions on lines 32 and 33 in the file:

    https://github.com/python/cpython/blob/master/PC/layout/support/props.py

    are both missing closing parentheses, which results in literal text instead of the resolve file paths. This appears to be introduced in version 3.7.2 of the package onward, including the current pre-release 3.9.0-a5.

    In addition, several build conditions use the form " $(Property) == 'value' ", but should instead use " '$(Property)' == 'value' ". By not surrounding the property value with '', the condition may resolve as " == '' ", which is an invalid expression and will cause a build failure. This doesn't appear to have caused an issue yet, but it easily could.

    If there is no further discussion or objection, I can submit a PR with the required fixes.

    @commonsensesoftware commonsensesoftware mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes build The build process and cross-build stdlib Python modules in the Lib dir topic-installation OS-windows labels Apr 2, 2020
    @zooba
    Copy link
    Member

    zooba commented Apr 2, 2020

    The closing parentheses are needed - a PR would be appreciated for that.

    The quotes around a variable reference are unnecessary. At a parser level, it just changes it from a variable reference to a string literal with substitutions (unlike most shells, which do a textual substitution before parsing).

    @commonsensesoftware
    Copy link
    Mannequin Author

    commonsensesoftware mannequin commented Apr 2, 2020

    In testing the fix, another issue has arisen. It appears the specified expression will never yield a usable path.

    Expression 1:
    $([msbuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), "python_d.exe"))

    Expression 2:
    $([msbuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), "python.exe"))

    The package has the abridged structure of:

    ┌─ build
    │ │
    │ └─ native
    │ │
    │ └─ python.props

    └─ tools

    └─ python.exe

    Based on this hierachy, neither exe will resolve because they do not have the same common ancestor. Additionally,
    I found that "python_d.exe" is always assumed for "Configuration=Debug", but "python_d.exe" does not exist in
    the package (that I could find). I'm not sure if this means the path is wrong, "python_d.exe" was accidentally
    omiitted, or this property assignment simply should not exist. This current behavior will ultimately result in
    the build integration failing because "python_d.exe" is resolved, but it doesn't exist.

    Interestingly, the property specified in versions 3.7.1 and earlier appear to define the correct path as:

    <PythonHome>$(MSBuildThisFileDirectory)\..\..\tools</PythonHome>

    My suggestion is to revert back to this older variant. If "python_d.exe" isn't needed, then it should be removed.
    If it is needed, then the path needs to be fixed. To make the path more robust, I also recommend resolving this
    path using one of the following forms:

    <PythonHome>$([MSBuild]::NormalizePath($(MSBuildThisFileDirectory)\..\..\tools))</PythonHome>

    OR

    <PythonHome>$([System.IO.Path]::GetFullPath($(MSBuildThisFileDirectory)\..\..\tools))</PythonHome>

    In my particular case, the tooling I'm plugging this into wasn't happy unless the path was absolute. There
    doesn't appear to be any reason or downside to resolving the path ahead of time. I can easily workaround
    that issue, but I suspect resolving an absolute path may be useful to other package consumers as well.

    As soon as I know what the final form of the property should be, I'll submit the PR and update this issue with the link

    @zooba
    Copy link
    Member

    zooba commented Apr 3, 2020

    Either of those fixes look good. I normally use IO.Path personally, which might be because it's been around longer, but anything that works on VS 2017 and later should be fine.

    @zooba
    Copy link
    Member

    zooba commented Apr 3, 2020

    New changeset 6e623ff by Chris Martinez in branch 'master':
    bpo-40158: Fix CPython MSBuild Properties in NuGet Package (GH-19343)
    6e623ff

    @zooba zooba closed this as completed Apr 3, 2020
    @zooba zooba closed this as completed Apr 3, 2020
    @zooba
    Copy link
    Member

    zooba commented Apr 3, 2020

    New changeset 7f70456 by Miss Islington (bot) in branch '3.7':
    bpo-40158: Fix CPython MSBuild Properties in NuGet Package (GH-19343)
    7f70456

    @zooba
    Copy link
    Member

    zooba commented Apr 3, 2020

    New changeset e6685ad by Miss Islington (bot) in branch '3.8':
    bpo-40158: Fix CPython MSBuild Properties in NuGet Package (GH-19343)
    e6685ad

    @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.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes build The build process and cross-build OS-windows stdlib Python modules in the Lib dir topic-installation
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant