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

Compile NVDA with the Windows 10 SDK #7568

Merged
merged 13 commits into from Oct 9, 2017

Conversation

Projects
None yet
5 participants
@michaelDCurran
Contributor

michaelDCurran commented Sep 5, 2017

Summary of the issue:

Currently NVDA is specifically compiled with Windows SDK 7.1A which was the last windows SDK version to support XP. Newer versions of Visual Studio have contained a special re-packaged Windows SDK 7.1a for this purpose. However, this SDK may lack particular features (E.g. static analysis) or optimizations, and will not take advantage of APIs only available on newer Operating Systems.

Description of how this pull request fixes the issue:

This PR upgrades to SCons 3.0, which now detects and uses Visual Studio 2017. Our sconscripts specifically check for VS 2017 and will no longer run on anything lower.
This PR also removes the use of Windows SDK 7.1a, thereby falling back to the latest SDK available on the system; This will be the latest Windows 10 SDK.
This PR also changes the _WIN32_WINNT define to be WIN7 (rather than XP), which allows the use of features available on at most Windows 7. The linker now also no longer receives /subsystem or /machine arguments, which now means binaries will default to being suitable for Windows 7 at a minimum, and CPU features such as SSE2 will be used.
There were some calls to GetVersion which is now deprecated. These were either removed (as they were only checking for Vista anyway), or changed to the now recommended versionHelper macros.

Testing performed:

  • NVDA executed on a Windows 7 SP1 machine, tested speak typed characters, read a page in Internet Explorer.

Known issues with pull request:

None

Change log entry:

No user visible change.
Changes for Developers:

  • NVDA is now compiled with Visual Studio 2017 and the Windows 10 SDK.

@michaelDCurran michaelDCurran requested a review from feerrenrut Sep 5, 2017

@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Sep 5, 2017

Collaborator

Hi,

When I try to compile this from source, Command prompt and powerShell says MIDL.EXE is not a recognized command or an operable program. Thanks.

Collaborator

josephsl commented Sep 5, 2017

Hi,

When I try to compile this from source, Command prompt and powerShell says MIDL.EXE is not a recognized command or an operable program. Thanks.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Sep 5, 2017

Contributor
Contributor

michaelDCurran commented Sep 5, 2017

@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Sep 5, 2017

Collaborator

Yep.

Note to others: do NOT (ever) uninstall whatever SDK that comes with Visual studio Community, otherwise build failures may result.

Thanks.

Collaborator

josephsl commented Sep 5, 2017

Yep.

Note to others: do NOT (ever) uninstall whatever SDK that comes with Visual studio Community, otherwise build failures may result.

Thanks.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Sep 19, 2017

Contributor

@feerrenrut: another review please. This now also upgrades to SCons 3.0 official and switches to Visual Studio 2017 for compilation. This completes the switch to more modern tools.

Contributor

michaelDCurran commented Sep 19, 2017

@feerrenrut: another review please. This now also upgrades to SCons 3.0 official and switches to Visual Studio 2017 for compilation. This completes the switch to more modern tools.

@michaelDCurran michaelDCurran requested a review from feerrenrut Sep 19, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 19, 2017

Collaborator

@michaelDCurran: Could you please review the sconscript for nvdahelper/localWin10 There is now some obsolete stuff in there which can be removed.

Collaborator

leonardder commented Sep 19, 2017

@michaelDCurran: Could you please review the sconscript for nvdahelper/localWin10 There is now some obsolete stuff in there which can be removed.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Sep 19, 2017

Collaborator

I also have the following error

link /nologo /incremental:no /WX/subsystem:windows,6.01 /release /OPT:REF /export:DllGetClassObject,private /export:DllCanUnloadNow,private /export:GetProxyDllInfo,private /manifest:embed /manifestinput:build\x86_64\ISimpleDOM.manifest /dll /out:build\x86_64\ISimpleDOM.dll /implib:build\x86_64\ISimpleDOM.lib rpcrt4.lib oleaut32.lib ole32.lib /PDB:build\x86_64\ISimpleDOM.dll.pdb /DEBUG build\x86_64\iSimpleDOMNode_i.obj build\x86_64\iSimpleDOMNode_p.obj build\x86_64\iSimpleDOMNode_data.obj
LINK : warning LNK4044: unrecognized option '/WX/subsystem:windows,6.01'; ignored
   Creating library build\x86_64\ISimpleDOM.lib and object build\x86_64\ISimpleDOM.exp
iSimpleDOMNode_data.obj : error LNK2001: unresolved external symbol iSimpleDOMNode_ProxyFileInfo
build\x86_64\ISimpleDOM.dll : fatal error LNK1120: 1 unresolved externals
scons: building terminated because of errors.
Collaborator

leonardder commented Sep 19, 2017

I also have the following error

link /nologo /incremental:no /WX/subsystem:windows,6.01 /release /OPT:REF /export:DllGetClassObject,private /export:DllCanUnloadNow,private /export:GetProxyDllInfo,private /manifest:embed /manifestinput:build\x86_64\ISimpleDOM.manifest /dll /out:build\x86_64\ISimpleDOM.dll /implib:build\x86_64\ISimpleDOM.lib rpcrt4.lib oleaut32.lib ole32.lib /PDB:build\x86_64\ISimpleDOM.dll.pdb /DEBUG build\x86_64\iSimpleDOMNode_i.obj build\x86_64\iSimpleDOMNode_p.obj build\x86_64\iSimpleDOMNode_data.obj
LINK : warning LNK4044: unrecognized option '/WX/subsystem:windows,6.01'; ignored
   Creating library build\x86_64\ISimpleDOM.lib and object build\x86_64\ISimpleDOM.exp
iSimpleDOMNode_data.obj : error LNK2001: unresolved external symbol iSimpleDOMNode_ProxyFileInfo
build\x86_64\ISimpleDOM.dll : fatal error LNK1120: 1 unresolved externals
scons: building terminated because of errors.
@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Sep 19, 2017

Collaborator
Collaborator

josephsl commented Sep 19, 2017

michaelDCurran added some commits Sep 19, 2017

iSimpleDOMNode_sconscript: don't lose existing flags when adding c_ex…
…t and /I to MIDLFLAGS as this was dropping /x64 which is needed for compiling when 64 bit.
nvdaHelper sconscripts: now we are always using the Windows 10 SDK, w…
…e can compile nvdaHelperLocalWin10 using the same infrastructure as nvdaHelperRemote and nvdaHelperLocal etc, except that rather than /mt, /ZW should be used. This removes a great deal of code from the nvdaHelperLocalWin10 sconscript.

 # Please enter the commit message for your changes. Lines starting
@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Sep 19, 2017

Contributor
Contributor

michaelDCurran commented Sep 19, 2017

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Sep 19, 2017

Contributor
Contributor

michaelDCurran commented Sep 19, 2017

michaelDCurran added a commit that referenced this pull request Oct 3, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Oct 4, 2017

Collaborator

It looks like SCons now has officially moved to github. Would it be possible to use the official repo as a submodule?
https://github.com/SConsProject/scons

Collaborator

leonardder commented Oct 4, 2017

It looks like SCons now has officially moved to github. Would it be possible to use the official repo as a submodule?
https://github.com/SConsProject/scons

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Oct 4, 2017

Contributor
Contributor

michaelDCurran commented Oct 4, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Oct 4, 2017

Collaborator

@michaelDCurran commented on 4 okt. 2017 09:37 CEST:

Err... this didn't exist when I looked a few weeks ago. Must have just
happened?

  1. Probably, yes.

But certainly, if it is official, then yes I'll switch it.

It is now mentioned on the website as their primary development platform.

Collaborator

leonardder commented Oct 4, 2017

@michaelDCurran commented on 4 okt. 2017 09:37 CEST:

Err... this didn't exist when I looked a few weeks ago. Must have just
happened?

  1. Probably, yes.

But certainly, if it is official, then yes I'll switch it.

It is now mentioned on the website as their primary development platform.

@michaelDCurran michaelDCurran merged commit 9c3beaa into master Oct 9, 2017

@nvaccessAuto nvaccessAuto removed the incubating label Oct 9, 2017

@nvaccessAuto nvaccessAuto added this to the 2017.4 milestone Oct 9, 2017

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Oct 9, 2017

Contributor

Actually, this was too early. Give it another week. master now reset to before this pr. I read the date wrong ;)

Contributor

michaelDCurran commented Oct 9, 2017

Actually, this was too early. Give it another week. master now reset to before this pr. I read the date wrong ;)

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Oct 9, 2017

Collaborator

Hmm, Github still thinks this is merged.

Collaborator

leonardder commented Oct 9, 2017

Hmm, Github still thinks this is merged.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Oct 9, 2017

Contributor
Contributor

michaelDCurran commented Oct 9, 2017

# UWP dlls can only be dynamically linked with the CRT,
# but some systems might not have this version of the CRT.
# Therefore, we must include it.
vcRedist = os.path.join(progFilesX86, r"Microsoft Visual Studio 14.0\VC\redist\onecore\x86\Microsoft.VC140.CRT")
vcRedist = os.path.join(progFilesX86, r"Microsoft Visual Studio\2017\Community\VC\Redist\MSVC\14.11.25415\onecore\x86\Microsoft.VC141.CRT")

This comment has been minimized.

@leonardder

leonardder Oct 17, 2017

Collaborator

Would it be possible to get rid of the 14.11.25415 part here by using glob and only use the most recent version available? One problem I see with the current approach is that Appveyor silently updates its Visual Studio 2017 image to a newer build of VS, which might bundle a newer version of the redistributable. Strange enough, when I updated to Visual Studio 2017.4, i lost 14.11.25415 and have now 14.11.25325.

@leonardder

leonardder Oct 17, 2017

Collaborator

Would it be possible to get rid of the 14.11.25415 part here by using glob and only use the most recent version available? One problem I see with the current approach is that Appveyor silently updates its Visual Studio 2017 image to a newer build of VS, which might bundle a newer version of the redistributable. Strange enough, when I updated to Visual Studio 2017.4, i lost 14.11.25415 and have now 14.11.25325.

michaelDCurran added a commit that referenced this pull request Oct 18, 2017

Compile NVDA with Visual Studio 2017. (#7568)
    * Switch to using SCons 3.0 from a git mirror we manage.
    * Compile NVDA with the Windows 10 SDK, replace GetVersion with versionHelper macros, and remove a pre-vista TSF check.
                       * Instruct appveyor to use its Visual Studio 2017 image
* Update readme to mention Visual Studio 2017
* iSimpleDOMNode_sconscript:  don't lose existing flags when adding c_ext and /I to MIDLFLAGS as this was dropping /x64 which is needed for compiling when 64 bit.
* nvdaHelper sconscripts: now we are always using the Windows 10 SDK, we can compile nvdaHelperLocalWin10 using the same infrastructure as nvdaHelperRemote and nvdaHelperLocal etc, except that rather than /mt, /ZW should be used. This removes a great deal of code from the nvdaHelperLocalWin10 sconscript.
* Readme.md: update VS 2017 section with extra needed dependency
* Clarify error message when VC++ 14.1 is missing.
* Ensure that all Python directories exist, eitherwise raise an error alerting the user to the fact they may need to run git submodule update --init
* nvdaHelperLocalWin10 sconscript:  search versioned directories from newest to oldest to find the vc restributables.
@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Oct 18, 2017

Contributor
Contributor

michaelDCurran commented Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment