building: ensure that onefile builds on Windows have manifest embedded #5625
Conversation
Test building with different --icon options, as that is what seems to be triggering inconsistent behavior at the moment.
PyInstaller/building/api.py
Outdated
| if is_win and (self.icon != "NONE" or self.versrsrc or self.resources | ||
| or self.uac_admin or self.uac_uiaccess or not is_64bits): | ||
| or self.uac_admin or self.uac_uiaccess or not is_64bits | ||
| or (self.manifest and not self.exclude_binaries)): |
There was a problem hiding this comment.
I'd rather like some comments in here explaining why simply if is_win: is not what we want. Is it that there's no point adding a manifest unless at least one of those conditions is true or that it breaks it if we unconditionally add the manifest ?
There was a problem hiding this comment.
Yeah, that's a fair point. I think going with just is_win should be enough (and I strongly suspect that this part of code simply gradually evolved to be that way).
The optional stuff (self.icon, self.versrsrc and self.resources) are under their corresponding subsequent checks anyway.
The (self.manifest and not self.exclude_binaries) was added to fix this issue (and its subsequent check has an additional is_win check that's redundant).
self.uac_admin or self.uac_uiaccess are probably there to ensure that we add the manifest if either of those options is enabled (as they're written in the manifest).
Not sure about not is_64bits part, but I suspect that nowdays the self.icon != "NONE" accounts for most cases, so that doesn't influence much, anyway.
There was a problem hiding this comment.
Is it that there's no point adding a manifest unless at least one of those conditions is true or that it breaks it if we unconditionally add the manifest ?
We probably always need the manifest. If nothing else, we use it to try opting-in for long paths; and as discovered by #5624, some other things may depend on it as well.
The only time when we don't need to embed it in the executable is if we're doing onedir build, since it's written to a separate file. (Which I think is why we have that not self.exclude_binaries check).
There was a problem hiding this comment.
The onefile builds on Windows may not have manifest embedded if icon was explicitly disabled via `--icon NONE`. This in turn causes subtle issues, such as pyinstaller#5624. Remove the convoluted check for entering the resource-modification codepath, as we always need to reach the part that embeds the manifest in the onefile executable, and the rest of resource-modification parts are already guarded by their corresponding checks. This should simplify the code and make it less prone to errors.
|
Let's see what the CI thinks about the simplified version, then... |
|
Strange. AppVeyor appears to have dosed off. Might need to give it an encouraging |
bwoodsend
added
the
merge-on-ci-pass
|
@Legorooj Are you able to merge this? Every time I press the confirm rebase and merge button it doesn't do it. |
|
Merged 👍 |
... regardless of the icon setting.
The
onefilebuilds on Windows may not have manifest embedded if icon was explicitly disabled via--icon NONE. This in turn causes subtle issues, such as #5624.Fixes #5624.
Side note: with 3c3228d, the behavior of this issue became inverted; before it (i.e., PyInstaller 4.1 and older), the manifest would not be embedded if icon was not provided (i.e.,
self.iconwas falsy), i.e., by default. So this may also explain some weird behavior of Windowsonefilebuilds made with PyInstaller 4.1 and older (which mysteriously went away if --icon was specified).