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

bootloader Windows XP compatibility #2974

Merged
merged 1 commit into from Dec 7, 2017

Conversation

@tenuki
Copy link
Contributor

commented Nov 7, 2017

This PR address ticket #2876 when using MSVC compiler.

Basically this makes the linker use the option /SUBSYSTEM, with its 4 combinations:

  • /SUBSYSTEM:WINDOWS,5.01
  • /SUBSYSTEM:CONSOLE,5.01
  • /SUBSYSTEM:WINDOWS,5.02
  • /SUBSYSTEM:CONSOLE,5.02

Where 5,01 is used for 32bit arch and 5,02 for 64bit.

I ran a few manual tests which worked ok, if there's some suite, please let me know how to run it.

I'm not sure if for an automated build system, an extra setup (package having support for that platform is required to be installed for MSVC, in that case, please let me know).

@htgoebel htgoebel changed the title Fix for #2876 bootloader Windows XP compatibility Nov 15, 2017

@htgoebel
Copy link
Member

left a comment

Thanks for this pull-request.

Since this MSVC is a beast, I'm afraid of side-effects. This is why I'm picky about changing things here.

What are these options doing exactly? Why is this safe? Why can I be confident, there are not side-effects? Why is this necessary now, but has not been necessary earlier? You may want proof your statement by pointing we to some 3rd-party explanation.

I'm not sure if […] an extra setup […] is required.

This is not what I wanted to read :-\ The bootloader needs to build in the VM we are using (and in Appvaor, of course) and I'm not going to test this as long as your are "not sure" (say: have not tested this yourself).

Win XP is end of live for quite some time. So I'm not going to spend much time on this. (Sorry, but I have to be careful with my time).

Please also note that we are not including bootlaoders build by anybody other then the core developers. So please remove them when updating the pull-request.

Also the commit message has to follow the guideline.

I'm looking forward for you explanasion


set_win_link(ctx.env, True)

def set_win_link(env, win):

This comment has been minimized.

Copy link
@htgoebel

htgoebel Nov 15, 2017

Member

I'm not convinced, this is a good place for doing this. I understand you need to pass different options for consoled and windowed, but there already is windowed() to set the values for the later case. So the default should be to set the "console"-options (around line 621, where LINKFLAGS are already set) and change them in windowed() (env._get_list_value_for_modification() might be helpful).

Also this should be well documented, what *exactly it does and that this is required for Windows XP compatibility.

def _clean_win_subsystem(env):
for v in ['C','CXX']:
for plat in ['CONSOLE','NATIVE','POSIX','WINDOWS','WINDOWSCE']:
env[v+'FLAGS_'+plat] = []

This comment has been minimized.

Copy link
@htgoebel

htgoebel Nov 15, 2017

Member

Please add a comment about what this does, why this is necessary and why this will not have any side-effects.

Also the function name should be more spelling, pointing to win32 and msvc. Also for better understanding this function should be nested like windowed() is. Do we need a function at all?

This comment has been minimized.

Copy link
@htgoebel

htgoebel Nov 15, 2017

Member

Use string-formating for better readability? Anyway, please follow PEP8: spaces.

return base

def _clean_win_subsystem(env):
for v in ['C','CXX']:

This comment has been minimized.

Copy link
@htgoebel

htgoebel Nov 15, 2017

Member

Please use a more spelling variable name, e.g. var.

@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2017

What are these options doing exactly? Why is this safe? Why can I be confident, there are not side-effects? Why is this necessary now, but has not been necessary earlier? You may want proof your statement by pointing we to some 3rd-party explanation.

I'm not sure if […] an extra setup […] is required.

This is not what I wanted to read :-\ The bootloader needs to build in the VM we are using (and in Appvaor, of course) and I'm not going to test this as long as your are "not sure" (say: have not tested this > yourself).

Thanks for the answer and comments, I'll fix that when have some more time, also let me answer these:

I had two things that wasn't sure about:

First, I seemed to remember that there were necessary two changes to make MSVS compile for XP. The one I've applied (settings the subsystem version specifically for XP or XP/x64). The other part was having an RTM on builder box supporting that. And that's something I don't know about pyinstaller's build bots.

Second, I wasn't sure at all of how to deal with these build compilation system, how these build configs work and theirs clone to make another "setup", so I've tried to do my best. (That's why I've clean-up some variables in case they were reused (as indeed I was having a problem of a setting being specified twice)).

I don't know what you mean with "have not tested this yourself". Because it is clear I don't have access to your build system. And it is also clear I've made the bins on my own and tried them (and other people have tried them too, check the ticket dup of this where I pointed that), and, bad on my part, I've committed them. But, again, I wouldn't be proposing this if didn't work on my side, and the tests ran seems to check it is working (I understand that's not a final word, of course).

Thanks, I'll work on changes you pointed!!!!

@tenuki tenuki referenced this pull request Nov 16, 2017
@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2017

I'm sorry for the other PR, I'm a very basic user of git didn't know how to re-use this, and was going to close it in favor of the new one..

However here we are.. I've got the changes working, and I think, the code is what was expected to be.

Let me know whatever I can be helpful for, thanks!

@ghost

This comment has been minimized.

Copy link

commented Nov 21, 2017

@tenuki You should not modify binary files in this pull request. Please revert those changes.

@tenuki tenuki force-pushed the tenuki:update-for-2876-xp-support branch from 86e20f4 to 6d41325 Nov 21, 2017

@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2017

@xoviat I'm sorry but I don't know how to do that with git. I would checkout bins from master and commit them over in the branch. would that be ok? If no, how should I do it?
Thanks

@ghost

This comment has been minimized.

Copy link

commented Nov 21, 2017

git reset --soft HEAD~1
git checkout HEAD -- PyInstaller/bootloader/Windows-32bit/run.exe
[repeat for all binaries]
git commit -a
git push -f

@tenuki tenuki force-pushed the tenuki:update-for-2876-xp-support branch from 6d41325 to 3bb0525 Nov 21, 2017

@ghost

This comment has been minimized.

Copy link

commented Nov 21, 2017

Also, please use a more detailed commit message than simply "updated." Maybe explain what you did and why.

@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2017

@xoviat thanks, and sure, good commit message, previous one wasn't the good commit. thanks

@htgoebel

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

Thanks for the update. I still do not understand what this change does exactly.

I found https://docs.microsoft.com/en-us/cpp/build/reference/subsystem-specify-subsystem, which says: "The choice of subsystem affects the entry point symbol (or entry point function) that the linker will select." Okay, but why do we need 5.01/5.02 for Windows and why this is safe to use for Windows 10 and later, too?

Reading https://stackoverflow.com/questions/30960872/ I assume the following, this sets the minimum version of windows, this application will run on. 50.1 is the value for 32-bit XP, 5.02 for 64-bit XP.

This Stackoverflow-Answer also says Windows 8 SDK only supports development of software down to Windows Vista, for XP SDK 7 is required. So do we need to switch the SDK, too? How is this done ?

Regarding the building the bootloader: Please verify the bootloaders build in our environment. https://pyinstaller.readthedocs.io/en/stable/bootloader-building.html#vagrantfile-virtual-machines should give all information required. Basically it's just:

vagrant plugin install vagrant-reload vagrant-scp
cd bootloader
vagrant up windows10
vagrant halt windows10
vagrant destroy windows10
@ghost

This comment has been minimized.

Copy link

commented Nov 22, 2017

Python 3.5 cannot be used on Windows XP anyway. I also don't think that this PR, if merged, will do what the author wants it to do: binaries that are compatible with Windows XP will not show up on pypi.

To produce Windows XP binaries, you need to set the platform toolset to v140_xp and everything should configure correctly.

@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2017

I found https://docs.microsoft.com/en-us/cpp/build/reference/subsystem-specify-subsystem, which says: "The choice of subsystem affects the entry point symbol (or entry point function) that the linker will select." Okay, but why do we need 5.01/5.02 for Windows and why this is safe to use for Windows 10 and later, too?

It is required because since VS2012 the default minimal target for VS was setup to Vista (win ver. 6.0).
Windows10 (and others) is backward compatible and is able to execute all binaries built for previous versions.

About the entry-point, it is selected by the /SUBSYSTEM: first argument, CONSOLE or WINDOW as it was used before. One invokes main(..) while other WinMain(..) (or the respective wide or ansi versions).

Reading https://stackoverflow.com/questions/30960872/ I assume the following, this sets the minimum version of windows, this application will run on. 50.1 is the value for 32-bit XP, 5.02 for 64-bit XP.

Yes, that's what the commit says too.

This Stackoverflow-Answer also says Windows 8 SDK only supports development of software down to Windows Vista, for XP SDK 7 is required. So do we need to switch the SDK, too? How is this done ?

When I was using VS2012 I had to install by hand the SDK, however with the VS2015 I've tested it it wasn't required. However, I'm checking out the vagrant files and testing.. and will let you know.

@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2017

Python 3.5 cannot be used on Windows XP anyway. I also don't think that this PR, if merged, will do what the author wants it to do: binaries that are compatible with Windows XP will not show up on pypi.

I'm not sure if you're referring to me but, at least in my case, I'm not doing this for pypi. But anyway, XP will die and isn't supported anymore by a lot of products, and you're completely free to not support it anymore, and not to spend any time for its support.

To produce Windows XP binaries, you need to set the platform toolset to v140_xp and everything should configure correctly.

I'll check the vagrant environment.. I have had not to install anything more than VS2015.. but maybe that's not the case in vagrant's setup.. I'll check that..

@ghost

This comment has been minimized.

Copy link

commented Nov 22, 2017

Sorry, was wrong on this issue. However, you need /D \"_USING_V110_SDK71_\" in the CL invocations.

@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2017

Sorry, was wrong on this issue. However, you need /D "USING_V110_SDK71" in the CL invocations.

Are you completely sure? I've just finish to download the build box images, and tested it wasn't required.

You can see here, there's no error on build, with the subsystem specified. And after that, the dumpbin output (on the left), shows the binary effectively targets the required version.

Virtual_Box_bootloader_windows10_1511395258670_26061_23_11_2017_0.png

(I'm sorry I don't know what's the good way to share such an image if required)

Please, contact me if I can be helpful in some way. Thanks.

@ghost

This comment has been minimized.

Copy link

commented Nov 23, 2017

It's probably fine then. But please squash the two commits together and revert the unnecessary changes.

@@ -44,7 +44,6 @@ variants = {
# map waf's DEST_OS to these values.
DESTOS_TO_SYSTEM = {
'linux': 'Linux',
'freebsd': 'FreeBSD',

This comment has been minimized.

Copy link
@ghost

ghost Nov 23, 2017

In particular here.

@@ -55,7 +54,6 @@ DESTOS_TO_SYSTEM = {
# Map from platform.system() to waf's DEST_OS
SYSTEM_TO_BUILDOS = {
'Linux': 'linux',
'FreeBSD': 'freebsd',

This comment has been minimized.

Copy link
@ghost
@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2017

I'm sorry I don't know how to do that with git.

I can make a new PR from a new branch, with one commit, if you want.

Also don't know how the missing freebsd line/change leaked-in. :-(

@ghost

This comment has been minimized.

Copy link

commented Nov 24, 2017

No need to make a new PR.

git remote add upstream https://github.com/pyinstaller/pyinstaller.git
git fetch upstream
git reset --soft upstream/develop
[edit files to remove unnecessary changes]
git commit -a
git push -f

@tenuki tenuki force-pushed the tenuki:update-for-2876-xp-support branch 2 times, most recently from eeb5bec to 770384c Nov 25, 2017

When using MSVC set the subsystem to force minimal XP target OS.
Basically this makes the linker use the option /SUBSYSTEM, with its 4
combinations:

/SUBSYSTEM:WINDOWS,5.01
/SUBSYSTEM:CONSOLE,5.01
/SUBSYSTEM:WINDOWS,5.02
/SUBSYSTEM:CONSOLE,5.02

Where 5,01 is used for 32bit arch and 5,02 for 64bit this specifies XP as
a minimal targeted OS version.

https://blogs.msdn.microsoft.com/vcblog/2012/10/08/windows-xp-targeting-with-c-in-visual-studio-2012/

@tenuki tenuki force-pushed the tenuki:update-for-2876-xp-support branch from 770384c to a344c85 Nov 25, 2017

@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2017

I think it's finally done.

@xoviat thank you so much for git assistance provided, I'll try to learn from it!

@htgoebel

This comment has been minimized.

Copy link
Member

commented Nov 26, 2017

I found https://docs.microsoft.com/en-us/cpp/build/reference/subsystem-specify-subsystem, which says: "The choice of subsystem affects the entry point symbol (or entry point function) that the linker will select." Okay, but why do we need 5.01/5.02 for Windows and why this is safe to use for Windows 10 and later, too?

About the entry-point, it is selected by the /SUBSYSTEM: first argument, CONSOLE or WINDOW as it was used before. One invokes main(..) while other WinMain(..) (or the respective wide or ansi versions).

This is the CONSOLE resp. WINDOWS value. But how does the suffix effect the entry-point? Or will this just become flags in the PE file?

I'll check the vagrant environment

Results?

@tenuki

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

@htgoebel no, the version specified (5.01/5/.02) doesn't change the entry point. Documentation says subsystem (console/gui) does.

Subsystem version changes requested OS version header.

I'm sorry, I mean that was going to check, on vagrant box, if something else was required to build for XP, meaning, an special SDK (as xoviat pointed out), and that wasn't required. Looks like it was only required for some VS2012.

@htgoebel htgoebel added the bootloader label Dec 6, 2017

@htgoebel htgoebel modified the milestones: PyInstaller 3.4, Hotfix 3.3.1 Dec 7, 2017

@htgoebel htgoebel merged commit 4496bf1 into pyinstaller:develop Dec 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.