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

windows: cross compilation fails due to headers with uppercase #78398

Closed
erikjanss mannequin opened this issue Jul 24, 2018 · 20 comments
Closed

windows: cross compilation fails due to headers with uppercase #78398

erikjanss mannequin opened this issue Jul 24, 2018 · 20 comments
Labels
3.8 only security fixes build The build process and cross-build OS-windows

Comments

@erikjanss
Copy link
Mannequin

erikjanss mannequin commented Jul 24, 2018

BPO 34217
Nosy @pfmoore, @tjguk, @methane, @zware, @eryksun, @zooba, @moreati, @miss-islington, @erikjanss
PRs
  • bpo-34217: use lowercase header for windows includes #8453
  • bpo-34217: Fix include "Windows.h" case for cross-compiling #8472
  • [3.7] bpo-34217: Use lowercase header for Windows (GH-8453) #8560
  • [3.7] bpo-34217: Use lowercase for windows headers (GH-8472) #8782
  • Files
  • windows_headers.py: python3 script to detect uppercases in windows headers
  • 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 2018-09-06.21:31:24.173>
    created_at = <Date 2018-07-24.23:42:09.210>
    labels = ['build', '3.8', 'OS-windows']
    title = 'windows: cross compilation fails due to headers with uppercase'
    updated_at = <Date 2018-09-06.21:31:24.173>
    user = 'https://github.com/erikjanss'

    bugs.python.org fields:

    activity = <Date 2018-09-06.21:31:24.173>
    actor = 'erikjanss'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-09-06.21:31:24.173>
    closer = 'erikjanss'
    components = ['Windows', 'Cross-Build']
    creation = <Date 2018-07-24.23:42:09.210>
    creator = 'erikjanss'
    dependencies = []
    files = ['47751']
    hgrepos = []
    issue_num = 34217
    keywords = ['patch']
    message_count = 20.0
    messages = ['322333', '322335', '322655', '323582', '323586', '323594', '323595', '323596', '323597', '323602', '323603', '323619', '323621', '323622', '323625', '323629', '323693', '323701', '323713', '323781']
    nosy_count = 9.0
    nosy_names = ['paul.moore', 'tim.golden', 'methane', 'zach.ware', 'eryksun', 'steve.dower', 'Alex.Willmer', 'miss-islington', 'erikjanss']
    pr_nums = ['8453', '8472', '8560', '8782']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'compile error'
    url = 'https://bugs.python.org/issue34217'
    versions = ['Python 3.8']

    @erikjanss
    Copy link
    Mannequin Author

    erikjanss mannequin commented Jul 24, 2018

    When cross compiling python on a linux host with a case sensitive file system for a windows target. The compilation will fail due to header
    files which contain uppercases.

    @erikjanss erikjanss mannequin added 3.8 only security fixes OS-windows build The build process and cross-build labels Jul 24, 2018
    @methane
    Copy link
    Member

    methane commented Jul 25, 2018

    New changeset 6cf8255 by INADA Naoki (erikjanss) in branch 'master':
    bpo-34217: Use lowercase header for Windows (GH-8453)
    6cf8255

    @methane methane closed this as completed Jul 25, 2018
    @miss-islington
    Copy link
    Contributor

    New changeset 6c89a9a by Miss Islington (bot) in branch '3.7':
    bpo-34217: Use lowercase header for Windows (GH-8453)
    6c89a9a

    @erikjanss
    Copy link
    Mannequin Author

    erikjanss mannequin commented Aug 15, 2018

    Attached a small helper script to detect camelcase windows headers,
    2 more occurences found :

    Modules/socketmodule.c versionhelpers.h 306 #include <VersionHelpers.h>
    Tools/msi/bundle/bootstrap/pch.h uxtheme.h 18 #include <Uxtheme.h>

    @erikjanss erikjanss mannequin reopened this Aug 15, 2018
    @zooba
    Copy link
    Member

    zooba commented Aug 15, 2018

    Are those bugs? On Windows, they are spelled with that casing (and while NTFS is case-insensitive, it *is* case-preserving).

    Whatever package you have used for cross-compiling may have renamed them (similarly Shlwapi.h). In which case, perhaps you could request that the package you are using preserve the case of the files it is cloning, rather than changing it?

    @methane
    Copy link
    Member

    methane commented Aug 16, 2018

    New changeset e6a4755 by INADA Naoki (Erik Janssens) in branch 'master':
    bpo-34217: Use lowercase for windows headers (GH-8472)
    e6a4755

    @erikjanss
    Copy link
    Mannequin Author

    erikjanss mannequin commented Aug 16, 2018

    @inada.naoki : thank you !

    @steve.dower : I would not consider those bugs per se, but they are
    inconsistencies, for example in the master branch :

    number of "Windows.h" includes : 2
    number of "windows.h" includes : 55

    whatever might be the 'right' solution, it's always an advantage to
    have a consistent codebase.

    @methane
    Copy link
    Member

    methane commented Aug 16, 2018

    New windows will support case sensitive filesystem too.
    Stop relying to case insensitive filesystem makes sense to me.

    @miss-islington
    Copy link
    Contributor

    New changeset bf8e9d1 by Miss Islington (bot) in branch '3.7':
    bpo-34217: Use lowercase for windows headers (GH-8472)
    bf8e9d1

    @zooba
    Copy link
    Member

    zooba commented Aug 16, 2018

    Stop relying to case insensitive filesystem makes sense to me.

    If this is the case, then we need to normalise the casing to match the actual files (which are mixed-case, not lowercase). You just broke this with your PR :)

    So which is it? Do we match the casing of the real files? Or do we change to match someone's unofficial versions of the files?

    @zooba
    Copy link
    Member

    zooba commented Aug 16, 2018

    whatever might be the 'right' solution, it's always an advantage to
    have a consistent codebase.

    Not when you are consistently wrong, it's not.

    Let's figure out the right answer here before making many changes to the codebase. First, can you tell me where you got your lowercased named files from? Because it wasn't the official SDK...

    @eryksun
    Copy link
    Contributor

    eryksun commented Aug 16, 2018

    New windows will support case sensitive filesystem too.

    NTFS in Windows 10 supports a flag to mark a directory as case sensitive [*] via NtSetInformationFile: FileCaseSensitiveInformation, which can be set from the command line via fsutil.exe. In recent builds this flag was made inheritable by child directories, so it doesn't have to be set individually on every directory in a tree. This feature was added for inter-operation with (WSL) Linux programs. It's not intended for system directories such as "Windows Kits".

    can you tell me where you got your lowercased named files from?

    IIRC, MinGW lower-cases the filenames in its modified version of the SDK.

    ---

    [*] Prior to Windows XP, case-sensitive file access was possible via FILE_FLAG_POSIX_SEMANTICS (CreateFile) and FIND_FIRST_EX_CASE_SENSITIVE (FindFirstFileEx). Windows XP (the first consumer version of NT) broke those flags by adding a (default-enabled) global flag in the kernel object manager that makes all device and file access case-insensitive -- regardless of the system call's OBJ_CASE_INSENSITIVE flag. Now in Windows 10 they're bringing back selective case sensitivity in NTFS, but this time using a per-directory flag that overrides the API. It seems they don't care that this breaks the existing OBJ_CASE_INSENSITIVE flag in the NT API. IMO, this could have been handled more cleanly by combining the per-directory flag with one or two new API flags.

    @erikjanss
    Copy link
    Mannequin Author

    erikjanss mannequin commented Aug 16, 2018

    The lowercase headers are indeed part of the mingw-w64 SDK, which is the most convenient option to cross compile on a linux host for a windows target.

    @zooba
    Copy link
    Member

    zooba commented Aug 16, 2018

    Have you posted to their mailing list to ask why they use different casing from the official SDK and whether they would consider supporting both (via symlink, presumably)?

    I think it would be strange for CPython to add a style rule for a non-supported toolset. The supported tooling will default to using the correct case for the header files.

    @erikjanss
    Copy link
    Mannequin Author

    erikjanss mannequin commented Aug 16, 2018

    As I understand it, if mingw-w64 would change to camelcase headers, it would break backwards compatibility, since a lot of windows source code uses file names in includes that differ from those on this, which works in MSVC, so it goes unnoticed. There is no good solution.

    Even if camelcase headers would be available, the CPython source code would break in much more places.

    I agree that it would be strange to have a style rule for a specific toolset, however mixing different styles is even stranger.

    The current situation is that 100% of the windows includes are lowercase and allow cross compilation. I would tend to see that as an advantage.

    If a more comprehensive solution would be available, that would be ideal of course.

    @zooba
    Copy link
    Member

    zooba commented Aug 16, 2018

    The current situation is that 100% of the windows includes are lowercase and allow cross compilation.

    Please be precise with what you are saying - the "current situation" is only so because you proposed changes and got them merged without proper discussion, and "allow cross compilation" with only one alternate toolset. Pretending this is the status quo is dishonest, so please don't insult me like that.

    if mingw-w64 would change to camelcase headers, it would break backwards compatibility

    I'm not recommending camel case - I'm recommending matching the casing used for each file in the official SDK (which I agree is a weird mix, but there are certainly compatibility reasons involved in keeping them this way). And yes, it would be a compatibility break for the clones of the SDK, but to a large extent that is their fault for choosing different casing in the first place, so I'm only a little bit sympathetic to that argument.

    What I'm mostly opposed to is the very weak bug report and the random selection of changes that came with it. Failing to compile on an unsupported toolset is not an immediate "must-fix" issue, and it really ought to go through the affected platform experts so they can confirm acceptable impact on developers who are using the supported tools. There also needs to be this central point so that, assuming we decide to keep them this way, the next person who comes in and complains that the casing doesn't match the actual files is given the correct explanation - otherwise we'll keep switching them back and forth forever.

    You're also likely to face a maintenance burden for this change, since there is currently no rule or validation that will prevent people from adding properly cased includes in the future (as I hinted, most IDEs on Windows will autocorrect filename casing for headers). If you want one, propose a change to PEP-7 on python-dev, and if it's approved then you can add a build step to validate. Otherwise, you'll have to track changes that are made and fix them as necessary. Without an explicit rule, our default position is "whatever the native target platform/tools prefer".

    I hope that explains the position a bit better, and why I push back against changes with insufficient justification being provided. At this point, I'm not going to revert these particular changes, as the cases where they will affect native developers are startlingly few these days, but I'm also explicitly saying that this does not mean open-season for all the fixes required for mingw to be happy. Each one will be taken on its merits (primarily compatibility and maintainability), at least until someone has committed to fully support the toolset.

    @methane
    Copy link
    Member

    methane commented Aug 18, 2018

    I googled and checked what MSDN uses before I merged these PRs.
    For example: https://msdn.microsoft.com/ja-jp/library/bb394814.aspx

    @erikjanss
    Copy link
    Mannequin Author

    erikjanss mannequin commented Aug 18, 2018

    I'll try to be more precise :

    • I did an (imperfect) search : before these changes 98% of the windows headers were included in the .c files in lowercase

    • These changes would bring it to 100%

    • The advantage of these changes are consistency and the ability to cross compile those .c files on linux for a windows target, using the mingw-w64 sdk, where the sdk is on a case sensitive filesystem, as is the default when installing this sdk in most linux distributions.

    • I did not consider, nor did I test other toolchains than mingw-w64/mingw-w64-sdk on linux and msvc/windows-sdk on windows.

    I'm not enough of an expert on the different tools to come up with a comprehensive solution for this issue, but am willing to validate such solution for the use case of cross compiling on linux.

    @zooba
    Copy link
    Member

    zooba commented Aug 18, 2018

    As someone who has written MSDN samples, I'll spoil the secret and let you know that there's no consistency there, so you can find examples to prove either direction ;)

    I wasn't expecting expertise or research. I was only expecting a description of the change that clearly specifies who benefits and who loses, and leaves a chance for the affected experts to confirm impact before being merged. There can be a surprising amount of subtlety around cross-platform issues, which is why we have designated people who are responsible for tracking them.

    @methane
    Copy link
    Member

    methane commented Aug 20, 2018

    I'm sorry about that. I won't merge PR like that next time.

    @erikjanss erikjanss mannequin closed this as completed Sep 6, 2018
    @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.8 only security fixes build The build process and cross-build OS-windows
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants