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

No longer append clang directories to path in SCons environment when building liblouis and update readme about Visual Studio #12728

Merged
merged 5 commits into from
Aug 13, 2021

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Aug 11, 2021

Link to issue number:

None

Summary of the issue:

In the past, we had to explicitly dig up the dirs where clang was installed inside Visual Studio. It looks like since then, Microsoft added LLVM to the path of the Visual Studio tools command prompt, which constructor batch file is used by SCons.

Description of how this pull request fixes the issue:

No longer add Clang to the path. Note that we still check its installation.

I also updated the readme according to the new design of the Visual Studio installer, thereby changing the following:

  1. As a minimum, version 16.11 is recommended which is the most recent version of Visual Studio 2019. This is not yet available on appveyor at the time of writing, but version 16.10 will only be supported for three months or so. I assume Appveyor will switch to 16.11 asap.
  2. The installer has a new design with list/tree view instead of a bunch of groupings you have to tab through.
  3. I appended a note about the build tools for those who don't require the IDE. This works like a charm and saves some unnecessary space on ones hard drive.

Testing strategy:

Tested that NVDA builds with Visual Studio build tools 16.11

Known issues with pull request:

None known

Change log entries:

Changes for developers:

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual testing.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers

@LeonarddeR LeonarddeR requested a review from a team as a code owner August 11, 2021 08:42
seanbudd
seanbudd previously approved these changes Aug 12, 2021
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will need to add a changelog entry in the "for developers" section mentioning this, as it has been 2 years since the minimum VS version has been changed. Also, a warning to the NVDA developer mailing list that they need to update VS to be able to build NVDA.

It looks like since then, Microsoft added LLVM to the path of the Visual Studio tools command prompt, which constructor batch file is used by SCons.

Do we know which version of VS introduced this change? It looks like 16.10 has this change as AppVeyor built NVDA fine. If you aren't sure of this that's okay, VS 16.11 and 16.10 both work fine as a new minimum requirement.

readme.md Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member

seanbudd commented Aug 12, 2021

Suggested changelog entry:

- Building NVDA now requires Visual Studio 2019 16.10 or later.
To build NVDA successfully, update Visual Studio to keep in sync with the [current AppVeyor build version https://www.appveyor.com/docs/windows-images-software/#visual-studio-2019]. (#12728)

@seanbudd seanbudd dismissed their stale review August 12, 2021 03:01

Changes to the README TBD

readme.md Outdated Show resolved Hide resolved
LeonarddeR and others added 2 commits August 12, 2021 08:02
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LeonarddeR, I'm going to remind the nvda developer email list to update VS after merging.

@seanbudd seanbudd merged commit 07f7bf1 into nvaccess:master Aug 13, 2021
@nvaccessAuto nvaccessAuto added this to the 2021.3 milestone Aug 13, 2021
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.

None yet

3 participants