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

vs2010backend: fix incompatibility with custom manifests #12472

Merged
merged 1 commit into from Nov 20, 2023

Conversation

LBPHacker
Copy link
Contributor

Given this relatively simple setup

The code attempts to create a file named zß水🍌 with standard C++ APIs (which use the ANSI variants of Win32 APIs), then checks whether it succeeded by using the Wide variant of a Win32 API. There is sufficient variation in the filename to not be covered by a single codepage, so unless the ANSI variants operate in the UTF-8 codepage, the check will fail. In more recent versions of Windows 10, this can be requested in the application manifest, which this setup also attempts, and, using the VS backend and current release versions of Meson, fails to do.

  • embed-manifest-test.cpp
    #include <Windows.h>
    #include <string>
    #include <fstream>
    
    static std::wstring WinWiden(const std::string &source)
    {
            int buffer_size = MultiByteToWideChar(CP_UTF8, 0, source.c_str(), source.size(), nullptr, 0);
            if (!buffer_size)
            {
                    return L"";
            }
            std::wstring output(buffer_size, 0);
            if (!MultiByteToWideChar(CP_UTF8, 0, source.c_str(), source.size(), &output[0], buffer_size))
            {
                    return L"";
            }
            return output;
    }
    
    int main()
    {
            std::string path = "\x7a\xc3\x9f\xe6\xb0\xb4\xf0\x9f\x8d\x8c"; // zß水🍌 in utf-8
            {
                    std::ofstream f(path);
            }
            if (GetFileAttributesA(path.c_str()) == INVALID_FILE_ATTRIBUTES)
            {
                    return 2;
            }
            if (GetFileAttributesW(WinWiden(path).c_str()) == INVALID_FILE_ATTRIBUTES)
            {
                    return 1;
            }
            return 0;
    }
  • embed-manifest-test.rc
    #include <ntdef.h>
    #include <winuser.h>
    
    CREATEPROCESS_MANIFEST_RESOURCE_ID RT_MANIFEST "winutf8.xml"
    
  • meson.build
    project('embed-manifest-test', 'cpp', default_options: [ 'cpp_std=c++17' ])
    
    executable(
            'embed-manifest-test',
            sources: [
                    import('windows').compile_resources(
                            'embed-manifest-test.rc',
                            depend_files: 'winutf8.xml',
                    ),
                    'embed-manifest-test.cpp',
            ],
    )
  • winutf8.xml
    <assembly manifestVersion="1.0" xmlns="urn:schemas-microsoft-com:asm.v1">
            <assemblyIdentity name="." version="6.0.0.0"/>
            <application>
                    <windowsSettings>
                            <activeCodePage xmlns="http://schemas.microsoft.com/SMI/2019/WindowsSettings">UTF-8</activeCodePage>
                    </windowsSettings>
            </application>
    </assembly>

and the following setup and compile commands

meson --backend=vs setup build
cd build && meson compile

the linking step fails with the following errors:

CVTRES : fatal error CVT1100: duplicate resource.  type:MANIFEST, name:1, language:0x0409
LINK : fatal error LNK1123: failure during conversion to COFF: file invalid or corrupt

due to the reason explained in the commit message:

EmbedManifest seems to default to true, which creates a default manifest based on other parameters (likewise defaults) and makes it impossible to supply your own with CREATEPROCESS_MANIFEST_RESOURCE_ID.

This has been tested with fairly vanilla installs of VS2017 and VS2022; results may vary depending on toolset, I'm not entirely sure, but I don't think the default for this setting would change. Again, as per the commit message:

There is value to being able to do this [supply your own manifest] and no value to the default one, so this should be disabled.

So this PR disables it.

@eli-schwartz
Copy link
Member

Could you reflow the commit message to 80 characters wide or thereabouts? :)

@LBPHacker
Copy link
Contributor Author

Oh, sure, hold on.

EmbedManifest seems to default to true, which creates a default manifest based
on other parameters (likewise defaults) and makes it impossible to supply your
own with CREATEPROCESS_MANIFEST_RESOURCE_ID. There is value to being able to do
this and no value to the default one, so this should be disabled.
@jpakkane jpakkane merged commit dfd8cfb into mesonbuild:master Nov 20, 2023
34 checks passed
@LBPHacker LBPHacker deleted the fix-vs2010-embed-manifest branch November 20, 2023 18:58
LBPHacker added a commit to LBPHacker/The-Powder-Toy that referenced this pull request Dec 3, 2023
This also requires patching CREATEPROCESS_MANIFEST_RESOURCE_ID out of powder-res.template.rc because Meson generates a VS config that instructs VS to add its own and this conflicts with ours. TODO: Undo this hack once mesonbuild/meson#12472 makes it into a release.
LBPHacker added a commit to LBPHacker/The-Powder-Toy that referenced this pull request Dec 3, 2023
This also requires patching CREATEPROCESS_MANIFEST_RESOURCE_ID out of powder-res.template.rc because Meson generates a VS config that instructs VS to add its own and this conflicts with ours. TODO: Undo this hack once mesonbuild/meson#12472 makes it into a release.

Also start using meson compile in place of ninja in some cases because it integrates better.
LBPHacker added a commit to LBPHacker/The-Powder-Toy that referenced this pull request Dec 3, 2023
This also requires patching CREATEPROCESS_MANIFEST_RESOURCE_ID out of powder-res.template.rc because Meson generates a VS config that instructs VS to add its own and this conflicts with ours. TODO: Undo this hack once mesonbuild/meson#12472 makes it into a release.

Also start using meson compile in place of ninja in some cases because it integrates better.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants