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

lib: ignore VS instances that cause COMExceptions #2018

Closed
wants to merge 2 commits into from

Conversation

amcasey
Copy link
Contributor

@amcasey amcasey commented Jan 8, 2020

Checklist
  • npm install && npm test passes
    Some tests are failing, but they also fail without my change. I assume the CI system will catch actual failures.
  • tests are included
    I see that find-visualstudio has pretty good test coverage, but I didn't see an obvious way to simulate this COM failure
  • documentation is changed or added
    Are there user-facing docs for find-visualstudio? I'd be happy to add a blurb about ignoring certain instances.
  • commit message follows commit guidelines
Description of change

I have quite a few instances of VS installed and it looks like Find-VisualStudio.cs enumerates all of them, even when find-visualstudio.js already knows which one it wants (from the environment variable in the developer command prompt). One of them (from 15.7.2, if that's interesting) causes a COMException on the ISetupInstance2.GetPackages call. Ignoring such packages seems harmless and unblocks the rest of the run.

Note: an alternative approach would be to print a JSON blob with an empty packages list and let the caller reject it with an explicit error message.

I have quite a few instances of VS installed and it looks like
Find-VisualStudio.cs enumerates all of them, even when
find-visualstudio.js already knows which one it wants (from the
environment variable in the developer command prompt).  One of them
(from 15.7.2, if that's interesting) causes a COMException on the
ISetupInstance2.GetPackages call.  Ignoring such packages seems harmless
and unblocks the rest of the run.
@rvagg rvagg requested a review from joaocgreis January 8, 2020 23:49
@joaocgreis
Copy link
Member

@amcasey thanks for opening this PR! Ideally we would print an error message mentioning the COM issue, but I'll happily take this as is.

One thing though, I fear this might make any other problem in InstanceJson extremely confusing to debug. Can you catch the COMException explicitly?

@amcasey
Copy link
Contributor Author

amcasey commented Jan 9, 2020

@joaocgreis Works for me. 😄

@joaocgreis
Copy link
Member

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/174/ ✔️

The Travis failures are unrelated.

I'll land this early next week (we need to wait at least 48h).

joaocgreis pushed a commit that referenced this pull request Jan 15, 2020
I have quite a few instances of VS installed and it looks like
Find-VisualStudio.cs enumerates all of them, even when
find-visualstudio.js already knows which one it wants (from the
environment variable in the developer command prompt).  One of them
(from 15.7.2, if that's interesting) causes a COMException on the
ISetupInstance2.GetPackages call.  Ignoring such packages seems
harmless and unblocks the rest of the run.

PR-URL: #2018
Reviewed-By: João Reis <[email protected]>
@joaocgreis
Copy link
Member

Landed in 1f7e1e9

Thanks @amcasey

@joaocgreis joaocgreis closed this Jan 15, 2020
@amcasey amcasey deleted the IgnoreBadInstances branch January 16, 2020 00:37
rvagg pushed a commit that referenced this pull request Feb 3, 2020
I have quite a few instances of VS installed and it looks like
Find-VisualStudio.cs enumerates all of them, even when
find-visualstudio.js already knows which one it wants (from the
environment variable in the developer command prompt).  One of them
(from 15.7.2, if that's interesting) causes a COMException on the
ISetupInstance2.GetPackages call.  Ignoring such packages seems
harmless and unblocks the rest of the run.

PR-URL: #2018
Reviewed-By: João Reis <[email protected]>
@rvagg rvagg mentioned this pull request May 26, 2020
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.

2 participants