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.

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

installers on ARM64 suggest wrong folders #90900

Closed
conioh mannequin opened this issue Feb 13, 2022 · 10 comments
Closed

installers on ARM64 suggest wrong folders #90900

conioh mannequin opened this issue Feb 13, 2022 · 10 comments
Labels
3.11 only security fixes OS-windows topic-installation type-bug An unexpected behavior, bug, or error

Comments

@conioh
Copy link
Mannequin

conioh mannequin commented Feb 13, 2022

BPO 46744
Nosy @pfmoore, @tjguk, @zware, @zooba, @conioh
PRs
  • bpo-46744: Move Windows ARM64 installation directory to correct ProgramFiles folder #31677
  • bpo-46744: Support "-Win32" and make platform flags case insensitive on Windows build scripts #31803
    		•

    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 = None
created_at = <Date 2022-02-13.23:50:52.347>
labels = ['type-bug', 'expert-installation', 'OS-windows', '3.11']
title = 'installers on ARM64 suggest wrong folders'
updated_at = <Date 2022-03-16.02:36:53.862>
user = 'https://github.com/conioh'

    bugs.python.org fields:

    activity = <Date 2022-03-16.02:36:53.862>
actor = 'conio'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Installation', 'Windows']
creation = <Date 2022-02-13.23:50:52.347>
creator = 'conio'
dependencies = []
files = []
hgrepos = []
issue_num = 46744
keywords = ['patch']
message_count = 9.0
messages = ['413199', '413262', '413266', '413294', '414500', '414502', '414629', '414689', '415325']
nosy_count = 6.0
nosy_names = ['paul.moore', 'tim.golden', 'python-dev', 'zach.ware', 'steve.dower', 'conio']
pr_nums = ['31677', '31803']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue46744'
versions = ['Python 3.11']
    @conioh
    Copy link
    Mannequin Author

    conioh mannequin commented Feb 13, 2022

    Thank you for your work on bringing Python to Windows on ARM64.

    I recently installed it an noticed some strange behaviours.

    On ARM64 Windows 11 the recent prerelease (3.11.0a5, post bpo-33125) acts in a way I believe is wrong: Checking the install for all users checkbox causes the installer to suggest the C:\Program Files (Arm)\Python311-Arm64 folder, but the C:\Program Files (Arm) is intended for ARM32 programs, similarly to how the C:\Program Files (x86) is intended for x86 programs.

    The folder for programs that are native for the platform is simply C:\Program Files - which is x86 on x86 Windows, x64 on x64 Windows and ARM64 on ARM64 Windows.

    So on ARM64 Windows the ARM64 Python should go into the native Program Files folder which is C:\Program Files.

    --

    A closely related issue is that the installer for x64 Python wants to install into C:\Program Files\Python311, but I already installed the ARM64 version there.

    The x86 acts as as should be expected and wants to install into C:\Program Files (x86)\Python311-32.

    But there's no "Program Files (x64)", so where should the x64 version on ARM64 machines go?

    I argue that the x64 version should go into C:\Program Files\Python311-amd64" while the ARM64 version should go into C:\Program Files\Python311`, because the ARM64 is the native on on this platform, while the x64 is foreign, and should get an elaborate name, like the x86, which is also foreign, gets.

    (The dotnet team also had this problem, and they decided similarly.)

    Internally sys.winver and the PEP-514 Registry structure can say whatever you like, but on the filesystem it's much more appropriate for the unqualified folder to be the system native one.

    @conioh conioh mannequin added 3.11 only security fixes topic-installation OS-windows type-bug An unexpected behavior, bug, or error labels Feb 13, 2022
    @zooba
    Copy link
    Member

    zooba commented Feb 14, 2022

    Thanks for the additional info! I agree with the design you propose.

    It would be much easier to make the ARM64 install use the tagged
    directory though (C:\Program Files\Python 3.11-arm64). I'd rather not
    get into the business of detecting platforms from the installer, and
    leave that to the OS (which can decide whether the MSI is suitable or not).

    I'll try and take a look at what the detection code would be like and
    make a call.

    @conioh
    Copy link
    Mannequin Author

    conioh mannequin commented Feb 14, 2022

    I understand.

    Though I think the naming scheme that includes platform identification is better, if all installers include the platform name unconditionally, including both amd64 and arm64, that's also an improvement. (So the names will be Python311-32, Python311-amd64 and Python311-arm64 - each in the appropriate Program Files folder.)

    The convention would be uniform, there won't be a collision between arm64 and amd64 on the same system, and amd64 won't get preferential treatment even on non-amd64 systems.

    Assuming py.exe will be fixed to recognize the arm64 versions in the Registry, I would be able to change the folder from Python311-arm64 to Python311 myself in the installer if it's so important to me and everything will work just the same.

    I do wonder though - is there no platform detection code today in the installer? Would the x86 versions install into "PythonXY-32" even on 32-bit Windows? If there already is, then adding a call to IsWow64Process2 doesn't sound that difficult. Especially since that's almost the same couple of lines that will have to added to the PyLauncher anyway. Could you please point me to where the code for the Windows Python installer is so I could also take a look?

    @zooba
    Copy link
    Member

    zooba commented Feb 15, 2022

    Yeah, currently the only detection that happens is OS version, and just
    so we can block (or at least log) users who are trying to install on
    supported versions. The install directories are unconditional.

    Any dynamic detection code would have to go into
    Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp and would
    update variables defined in Tools/msi/bundle/bundle.wxs. If you find the
    code for getting the ProgramFiles(Arm) directory, you'll see an example
    (which is probably the right one to update to handle the range of
    platforms).

    However, I think we should consider the 32/64-bit install directory
    names to be a compatibility concern, and shouldn't modify them generally
    out of some kind of sense of "fairness". If it's trivial and safe to
    detect 64-bit running on ARM64 (which, as you say, it ought to be), then
    we can update its directory name there by default.

    We should probably also display a notice in the installer when
    installing the Intel builds on ARM. The 64-bit version is fine, but the
    32-bit version has issues, and my experience so far is that the native
    build is significantly faster than both. We can do that another time though.

    @zooba
    Copy link
    Member

    zooba commented Mar 4, 2022

    Posted half a PR for this, which is worth taking in itself, but doesn't completely address the whole change. It will install to the correct ProgramFiles folder on ARM64 now, but still with the suffix.

    If we are to take the rest of the change (use "-amd64" for AMD64 builds on ARM64 OS), it has to be ready for beta 1. There's still time, but absent a major issue from *not* doing it, we want to avoid changing on-disk layout during beta/RC.

    @zooba
    Copy link
    Member

    zooba commented Mar 4, 2022

    New changeset 8f31bf4 by Steve Dower in branch 'main':
    bpo-46744: Move Windows ARM64 installation directory to correct ProgramFiles (GH-31677)
    8f31bf4

    @conioh
    Copy link
    Mannequin Author

    conioh mannequin commented Mar 7, 2022

    Thank you very much, Steve.

    I took a look but didn't get to making the patch. I'm happy to see it was just removing the
    calculation of the PF(ARM) folder.

    For the second issue, even though I prefer it the way I said (unqualified = native), since

    (i) the default is using the value of sys.winver at the end of the path, and it's "VER"
    for amd64 Python and "VER-32" for x86 Python regardless of the OS (i.e. even on 32-bit
    Windows);
    (ii) this has been the situation long enough to be a compatibility concern as you said, and
    (iii) if someone really wants the names to be the other way around they may choose whatever
    they like during the install, and it's only the defaults that are that way

    I think you are right in keeping it out for now. I wouldn't insist on it. If there comes a
    time when more arguments in favor of this change come up or the circumstances change (e.g.
    the compatibility issue becomes less of a concern) I'm sure you'd revisit this.

    I did notice one small issue when I tried to build Python for ARM64 following your message
    from 2022-02-15.

    The help message for [Tools/msi/build.bat](https://github.com/python/cpython/blob/main/Tools/msi/build.bat) says to use the -ARM64 flag to build installers
    for ARM64 but it didn't work for me. I took me a few attempts at clean and rebuild before I
    finally looked at the script itself and saw that it actually expects -arm64 in lowercase.

    If you could change either the help string or the flag itself so they'd math it would be great.

    Uppercase is nice because it is consistent with [PCbuild/build.bat](https://github.com/python/cpython/blob/main/PCbuild/build.bat) and lowercase is nice
    because if there's come CI that uses this flag it wouldn't have to be modified too. Obviously
    you if there are such consideration better than me so I leave the decision to you.

    If there's no problem or difficulty in changing the arguments for [Tools/msi/build.bat](https://github.com/python/cpython/blob/main/Tools/msi/build.bat) it
    might be nice if building for ARM64 would be "-ARM64" and building for x86 would be "-Win32"
    to match the platform names for [PCbuild/build.bat](https://github.com/python/cpython/blob/main/PCbuild/build.bat).

    @zooba
    Copy link
    Member

    zooba commented Mar 7, 2022

    Good call on the batch file. It should be easy enough to make those options case-insensitive, and to support both forms of x86 flag. I'll leave this issue open for that if someone wants to have a go, otherwise I'll get to them at some point.

    @conioh
    Copy link
    Mannequin Author

    conioh mannequin commented Mar 16, 2022

    I opened a PR for this change (#31803). I'd appreciate your comments and/or approval.

    I didn't add anything to the NEWS.d folder since it seems to me like something completely internal to the build process and so minor not worth mentioning there.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @zooba
    Copy link
    Member

    zooba commented Aug 25, 2022

    Thanks for the PR! Apologies for the delay in merging it

    @zooba zooba closed this as completed Aug 25, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.11 only security fixes OS-windows topic-installation type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @zooba