Skip to content

Conversation

@michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Oct 30, 2022

Link to issue number:

None.

Summary of the issue:

Although NVDA can be locally built with Visual Studio 2022, Official builds on appveyor are built with Visual Studio 2019, and even local builds still require the VS 2019 build tools, as that is what the microsoft-ui-uiAutomation project requires by default.
As Visual Studio 2022 has been out for some time now, NvDA should move to this version officially. This would remove the need for developers to have multiple versions installed, and NVDA can take advantage of improvements in the latest version (E.g. faster compile time).

Description of user facing changes

None.

Description of development approach

microsoft-ui-uiAutomation sconscript: instruct msbuild to use the same platform toolset as the rest of NVDAHelper by passing in the PlatformToolset build variable created from the MSVC_VERSION SCons variable. This removes the need for the VS 2019 build tools to be installed when compiling with VS 2022.
Instruct appveyor to use its VS 2022 system image.
Update the NVDA docs to no longer mention VS 2019.

Aldo disables notepad system tests. These have been unreliable for some time and become much more flakey on this image.

Testing strategy:

Ran NVDA locally for several hours, performing normal daily tasks.

Known issues with pull request:

None known.

Change log entries:

New features
Changes
Bug fixes
For Developers

  • Building NVDA now requires Visual Studio 2022. Please refer to NVDA's readme.md for the specific list of Visuao Studio components.

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@michaelDCurran michaelDCurran requested a review from a team as a code owner October 30, 2022 08:37
@AppVeyorBot
Copy link

See test results for failed build of commit 44fbf7c9bf

@Brian1Gaff
Copy link

Brian1Gaff commented Oct 30, 2022 via email

@michaelDCurran
Copy link
Member Author

michaelDCurran commented Oct 30, 2022 via email

@LeonarddeR
Copy link
Collaborator

I'm currently on Visual Studio 2022 preview and am suffering issues with compiling liblouis. See liblouis/liblouis#1259

@LeonarddeR
Copy link
Collaborator

I added a fix for liblouis in my louisClang branch.
In clang 15, The -Wint-conversion warning diagnostic for implicit int <-> pointer conversions now defaults to an error in all C language modes. This can be suppressed with -Wno-error=int-conversion
I removed a replacement for the warning level as we ignore liblouis warnings anyway.
@michaelDCurran Feel free to pull in commit 6ba1d63 . Having said that, I"m not sure whether this works on the version of Clang bundled with the current VS 2022, that'll need testing.

@michaelDCurran
Copy link
Member Author

michaelDCurran commented Oct 31, 2022 via email

@LeonarddeR
Copy link
Collaborator

Great.

@AppVeyorBot
Copy link

See test results for failed build of commit 8a20ba1933

@AppVeyorBot
Copy link

See test results for failed build of commit c956f10fbc

@AppVeyorBot
Copy link

See test results for failed build of commit 520a2b7274

@seanbudd seanbudd changed the title Compile NvDA with Visual Studio 2022 Compile NVDA with Visual Studio 2022 Nov 2, 2022
@AppVeyorBot
Copy link

See test results for failed build of commit 4cc36bafe9

@AppVeyorBot
Copy link

See test results for failed build of commit f1e935d377

@AppVeyorBot
Copy link

See test results for failed build of commit 4b71d9ddc6

@michaelDCurran
Copy link
Member Author

The VS 2022 image on appveyor is Windows 11. As Notepad has been modernised with a richEdit control on Windows 11, this causes inconsistancy for our system tests between win 10 and 11.
In order to progress this pr further, we'll need to find a replacement for notepad that provides the same control on both win 10 and 11.
However, we should at least take the first commit from this pr as it does enable full compilation of NVDA with VS 2022 locally, without requiring the VS 2019 build tools.

@michaelDCurran michaelDCurran marked this pull request as draft November 2, 2022 10:37
@josephsl
Copy link
Contributor

josephsl commented Nov 2, 2022 via email

@josephsl
Copy link
Contributor

josephsl commented Nov 2, 2022

Hi,

How about using WordPad as the class names are the same across Windows 10 and 11? We may need to look for rich edit implementation issues, but for at least getting this PR to work, we can try it.

Thanks.

michaelDCurran added a commit that referenced this pull request Nov 3, 2022
Taken from pr #14313 which cannot yet be merged in its entirety.

Summary of the issue:
NVDA can be compiled locally with VS 2022, but the VS 2019 build tools are still required, due to the Microsoft-UI-UIAutomation project specifying the VS 2019 platform toolset by default.

Description of user facing changes
None

Description of developer facing changes
NVDA can now be fully compiled locally with Visual Studio 2022, without also requiring the 2019 build tools.

Description of development approach
• When compiling the microsoft-ui-uiAutomation project, instruct msbuild to use the same platform toolset as the rest of NVDAHelper.
@LeonarddeR
Copy link
Collaborator

The VS 2022 image on appveyor is Windows 11.

I'm slightly puzzled. According to the appveyor docs, its Windows Server 2019.

@LeonarddeR
Copy link
Collaborator

@michaelDCurran Could you please merge in master so we can get new artifacts? I'd be interested in debugging this further, but as the artifacts are gone, I have no log files to investigate.

@michaelDCurran
Copy link
Member Author

@LeonarddeR I've merged master now.

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Jun 21, 2023

@michaelDCurran Thanks for merging.
I played with Wordpad briefly, but I think it has major disadvantages, such as replacing \n by a hard carriage return and having entirely different move by word logic. Not to mention that I needed to save the test files as utf-16 because wordpad doesn't like utf-8. Ugh!

@nvaccess nvaccess deleted a comment Jun 21, 2023
@LeonarddeR
Copy link
Collaborator

It looks like the thing just builds. As I already thought, Notepad in Windows Server 2019 should still be on the Windows 10 version. There must have been another, unrelated issue.
That said, I guess it is still problematic that Windows 11 users can't run the system tests, but that might be something for a follow up?

@hwf1324
Copy link
Contributor

hwf1324 commented Jun 21, 2023

I wonder if you have noticed that in the recent VS 2022 installation, ARM64 became ARM64/ARM64CE. should changes be made to the dependencies section of the documentation to alert those using the latest VS 2022?

@cary-rowen
Copy link
Contributor

@hwf1324 wrote:

I wonder if you have noticed that in the recent VS 2022 installation, ARM64 became ARM64/ARM64CE. should changes be made to the dependencies section of the documentation to alert those using the latest VS 2022?

I also noticed this situation you mentioned, I hope this can be made clearer in the documentation.

Since NV Access has recently unified product documentation, can you open a separate PR to clarify this?

Cheers

Cary

@seanbudd seanbudd added this to the 2024.1 milestone Sep 27, 2023
@LeonarddeR
Copy link
Collaborator

Is there anything holding this back, apart from the merge conflicts?

@seanbudd seanbudd marked this pull request as ready for review November 2, 2023 23:21
@AppVeyorBot
Copy link

See test results for failed build of commit 684b52e4b3

@AppVeyorBot
Copy link

See test results for failed build of commit 5992447733

@AppVeyorBot
Copy link

See test results for failed build of commit a3a80837e0

@AppVeyorBot
Copy link

See test results for failed build of commit 5df20d9e91

@AppVeyorBot
Copy link

See test results for failed build of commit 2628ac4503

@AppVeyorBot
Copy link

See test results for failed build of commit f298913499

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.

9 participants