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

Fix vswhere for Build Tools #1753

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

dkaszews
Copy link
Contributor

@dkaszews dkaszews commented Aug 10, 2024

According to microsoft/vswhere#22, vswhere.exe only finds full Visual Studio IDE instances. To find a naked Build Tools instance, one needs to explicitly add -products 'Microsoft.VisualStudio.Product.BuildTools' to the call, but that won't find IDE instances in turn.

The PR adds the second form as a fallback to support users who only have Build Tools installed without the full IDE.

> ./install.py --skip-build --verbose
Calling C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -latest -property installationVersion
vswhere returned nothing usable: ""
Retrying vswhere for Build Tools
Calling C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -latest -property installationVersion
 -products Microsoft.VisualStudio.Product.BuildTools
vswhere -latest returned version 17.10.35122.118

This change is Reviewable

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! I've left two small comments.

Also, the diff is not that bad on Reviewable!

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @dkaszews)


build.py line 150 at r1 (raw file):

def FindLatestMSVC( quiet, preview=False ):
  ACCEPTABLE_VERSIONS = [ 17, 16, 15 ]

This does not seem to be needed here.


build.py line 162 at r1 (raw file):

      vswhere_args.append( '-prerelease' )

    if msvc := UseVsWhere( quiet, vswhere_args ):

:= operator is python 3.9+.
We should hold back just a little longer. Python 3.8 goes EOL in October.

Yes, our CI uses python 3.9, but that's because new ubuntu does not have python 3.8 and I didn't want to drag in pyenv to compile an old version.is_accessible

Copy link
Contributor Author

@dkaszews dkaszews left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)


build.py line 150 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

This does not seem to be needed here.

It is used below for the registry loop. I moved it to file scope to remove duplication.


build.py line 162 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

:= operator is python 3.9+.
We should hold back just a little longer. Python 3.8 goes EOL in October.

Yes, our CI uses python 3.9, but that's because new ubuntu does not have python 3.8 and I didn't want to drag in pyenv to compile an old version.is_accessible

Walrus was introduced in 3.8: https://docs.python.org/3/whatsnew/3.8.html#assignment-expressions

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (waiting on @dkaszews)


build.py line 150 at r1 (raw file):

Previously, dkaszews (Dominik Kaszewski) wrote…

It is used below for the registry loop. I moved it to file scope to remove duplication.

That was outside the diff. Yeah, let's move it to the file scope, with a better name for context.

I'm just not seeing the second push in the diff.


build.py line 162 at r1 (raw file):

Previously, dkaszews (Dominik Kaszewski) wrote…

Walrus was introduced in 3.8: https://docs.python.org/3/whatsnew/3.8.html#assignment-expressions

So it is. Nevermind.

@dkaszews dkaszews force-pushed the fix-vswhere-build-tools branch from 3db0bfa to 4856081 Compare August 10, 2024 12:17
Copy link
Contributor Author

@dkaszews dkaszews left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)


build.py line 150 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

That was outside the diff. Yeah, let's move it to the file scope, with a better name for context.

I'm just not seeing the second push in the diff.

Pushed now, was having some connection issues

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dkaszews)

@dkaszews dkaszews force-pushed the fix-vswhere-build-tools branch from 4856081 to 31b80ea Compare August 10, 2024 13:41
@bstaletic
Copy link
Collaborator

Turns out i did not merge the CI pull request when I thought I had.
Now that #1746 has been merged, please rebase again.

@dkaszews dkaszews force-pushed the fix-vswhere-build-tools branch from 31b80ea to 6f1f4f8 Compare August 10, 2024 16:24
@dkaszews
Copy link
Contributor Author

Rebased again

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dkaszews)

Copy link

codecov bot commented Aug 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.87%. Comparing base (74a5e71) to head (6f1f4f8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1753      +/-   ##
==========================================
+ Coverage   95.49%   95.87%   +0.38%     
==========================================
  Files          52       84      +32     
  Lines        6947     8437    +1490     
  Branches        0      163     +163     
==========================================
+ Hits         6634     8089    +1455     
+ Misses        313      298      -15     
- Partials        0       50      +50     

@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Aug 12, 2024
Copy link
Contributor

mergify bot commented Aug 12, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit 18af82a into ycm-core:master Aug 12, 2024
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants