Fix an error when using /MANIFEST:NO with VS backend

[matyalatte]

Closes #13041

#12472 disabled the EmbedManifest node in .vcxproj.
But it conflicts with the /MANIFEST:NO option, or the GenerateManifest node should be disabled.

This PR make meson set false to GenerateManifest when target.link_args disables the manifest creation with /MANIFEST:NO.

[bruchar1]

MSVC command line options are case sensitive. I don't think you need to transform to upper.

[matyalatte]

Thanks for the review but I think it's necessary.
I tried /ManIfesT:NO and got the same error as #13041.
Also, I can see /ManifestFile in linker options that VS adds automatically, though the document says /MANIFESTFILE.
Seems that some options are case-insensitive.

[bruchar1]

You are right. Options for the compiler are case-sensitive, but options for the link are not. That's weird.

I don't really like the fact that you do 2 list comprehensions, but I understand why you do it. Maybe should the first line be a generative expression instead?

[bruchar1]

Also, the test for emptyness should use the bool operator, not the len function.

Suggested change

	upper_args = [arg.upper() for arg in target.link_args]
	manifest_args = [arg for arg in upper_args if arg == '/MANIFEST' or arg.startswith('/MANIFEST:')]
	return len(manifest_args) == 0 or manifest_args[-1] != '/MANIFEST:NO'
	upper_args = (arg.upper() for arg in target.link_args)
	manifest_args = [arg for arg in upper_args if arg == '/MANIFEST' or arg.startswith('/MANIFEST:')]
	return not manifest_args or manifest_args[-1] != '/MANIFEST:NO'

[dcbaker]

does /MANIFEST have to be the last argument, or could there be another argument after that?

[matyalatte]

/MANIFEST, /MANIFEST:EMBED, and /MANIFEST:EMBED,ID=xxx can enable the manifest creation.
I got the same error as #12472 when ignoring /MANIFEST:EMBED in the get_gen_manifest function.

[dcbaker]

My concern is that if your link args are:

['/somearg', '/MANIFEST:NO', '/someother-arg']

that this will fail. And (assumine that link.exe is sane):

['/somearg', '/MANIFEST:NO', '/MANIFEST', '/someother-arg']

makes this more complicated. I think we really need to do something like:

for x in reversed(manifest_args):
    if x == '/MANIFEST:NO':
        break
    if x.startswith('/MANIFEST'):
        return False
return True

So that we look at the last occurance of the /MANIFEST argument, and properly handle it being not the last argument in the link_args

[bruchar1]

He already filtered non-MANIFEST args in the list comprehension, so I think the proposed code is ok. But I thought about proposing a similar explicit for loop instead of list comprehensions. I think it would be more efficient, because it can stop at the first occurence found.

[dcbaker]

You're right, I missed that somehow. derp.

Yeah, I think I'd prefer a single loop instead of the two comprehensions as well.

[matyalatte]

So, should it be like this?

# Returns if a target generates a manifest or not.
def get_gen_manifest(target):
    if isinstance(target, build.BuildTarget):
        for arg in reversed(target.link_args):
            arg = arg.upper()
            if arg == '/MANIFEST:NO':
                return False
            if arg == '/MANIFEST' or arg.startswith('/MANIFEST:'):
                break
    return True

if arg.startswith('/MANIFEST'): doesn't work because there are other options that start with /MANIFEST.

[matyalatte]

I noticed my code ignores global and project args.
I'd prefer to revert #12472 rather than check linker options...
meson 1.3.2 already supports custom manifests because /MANIFEST:NO can disable the manifest embedding.
As mentioned in #13041, you will get warnings when disabling some manifest options but you can just ignore them and it's the simplest way to fix the issue.

[matyalatte]

Replaced the list comprehensions with a for loop.
and fixed a bug that it ignored some linker options e.g. global link arguments.

[bruchar1]

@dcbaker I think this one is ready

[eli-schwartz]

Please remember in future that PRs like this should NOT be merged until the fixup commit is squashed into the original commit.