-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Add profile generators for Visual Studio #7774
Add profile generators for Visual Studio #7774
Conversation
New misspellings found, please review:
To accept these changes, run the following commands
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
|
Though, off the top of my head, this took a good bit of time on launch to be able to populate these entries. I'm worried that if we enable these for every user by default, we'll get a bunch of complaints about how long this takes to load. I suppose these could be added to |
Eh I think the profiles should be minimal. I already don't want the Azure by default and now we want to add Visual Studio, which we all know has huge performance issues. Maybe we could do more perf optimization on this end but most of it needs to come from VS. IMO this is better as an extension (same with Azure). If we want to add this now, it definitely should be disabled by default. I think it should also check if VS is even installed prior to attempting to create these profiles. Maybe it is and I can't tell (My C++ experience is very limited). Maybe add some more comments to clarify everything a bit more. |
Ideally, yes, this and the azure cloud shell would both be extensions. This looks like it's the right way to detect VS versions. I bet that there's not a faster way of doing it either, we're probably at the mercy of the perf of Microsoft.VisualStudio.Setup.Configuration code itself. Maybe we'll want to do this as a proto-extension like @PankajBhojwani is working on over in #7584/#7632. In this case, we'd have VS ship code that would synthesize these profiles themselves, and then leave them for the Terminal to detect at a later time. If this is the path we want to pursue, we'd want to make sure to work with the VS team directly to get them to incorporate this code into VS on their end. IMO, (after reviewing the code closer of course), I'm okay with this going in for now as a disabled-by-default dynamic source, and then moving it to an extension (when those are officially supported). |
This is what I was thinking. Maybe you can internally file a perf bug for them while engaging in discussions as serving as a proto-extension. I think that would be a fantastic idea. |
I'm happy to add more comments, especially around the VS Setup Configuration code.
|
Bullet 1: Probably could get some minor optimization by preventing the generators from running in first place. Since this is to be disabled by default, you could add something that ties to the default setting and attempt to run if a user explicitly chooses to enable the profile source. Bullet 2: You should be able to write something up that consolidates both in one go instead of iterating over it twice. This project does tend towards code re-use where possible. Beyond that, not sure if there's anything within the TIL libraries or deps such as FMT that could help but might be worth a look. |
Frankly, I don't think those are going to be of that much help. From what I remember, when we added The proto-extension solution isn't a bad solution here, because we'll be able to pass the "enumerate VS installs" cost on to VS, who will be able to leave the profile(s) behind after it runs it's startup. /cc @PankajBhojwani for discussion of proto extensions |
With the proto extensions approach, it wouldn't even need to be VS itself. A third party powershell script or small command line tool could do it, and be developed in the open using the same ISetupConfiguration approach used here. A distinct tool would also have the same advantage of being able to generate profiles for any supported VS version. All that said, I have no strong objection to the generators being default disabled. As an avid VS user, the sooner I can stop trying to manually craft the complex complex command-line for Developer PowerShell, the better. (There's a lot of quote escaping!). How about I take a crack at caching the VsSetupInstances and making the generators off by default? |
This one should feel a bit snappier |
oh my gosh I know exactly what I did wrong. This PR was the first time I built & ran the Terminal since coming back from paternity leave. We didn't have the jumplist added when I left. So I mistakenly attributed the longer startup time to this PR, when in actuality, the slowdown was probably primarily from the jumplist. That's my bad 🤦♂️ |
New misspellings found, please review:
To accept these changes, run the following commands
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
|
I saw JumpList on the hot path when I did some profiling. Especially CShellLink::SetPath |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The caching is implemented, comments added. What are my next steps for review? |
Probably nothing more at this point - just wait till we get some time to review in depth 😜 I'd want to make sure the @DHowett gets a chance to sign off on this one as well, but he's got a full week of meetings unfortunately, so it might be a little while before we can get you two reviews @msftbot make sure @DHowett signs off on this |
New misspellings found, please review:
To accept these changes, run the following commands
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
|
If we go the proto-extensions route we incur a cost on start-up time by needing to query for the json files - if that cost is much smaller than querying for VS installations then that might be the better way to go (I am not sure how I do want to say that this does seem like quite a perfect use case for proto extensions though, and if we want to encourage the use of that instead of adding more dynamic profile generators to our codebase then that is something we should talk about too. As said above, would definitely want @DHowett's opinion on this. |
Hello @zadjii-msft! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks correct to me.
To be honest I'm not entirely happy about the verbosity of the code. I believe it would've been easier to understand if it wasn't split into two sub-classes.
Additionally I see that Ah I didn't realize that you store it in a static member. I'm sorry for that. Although that's not quite thread-safe. 😅 (I'm mentioning this because I've been thinking about running generators concurrently.)VsSetupConfiguration::QueryInstances()
is cached (because it's a slow call I assume?), but due to it being two sub-classes the function is still called twice during launch, defeating the purpose of the cache.
I'd like to merge it regardless of my opinion though, as it can still be improved upon later on.
Thanks so much for doing this! I pushed Microsoft.VisualStudio.Setup.Configuration.Native 2.3.2262 to TerminalDependencies! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this. Well done! Thank you.
We're in the home stretch for this nearly one-year-old PR. Good lord. Thanks for bearing with us.
NuGet.Config
Outdated
<configuration> | ||
<packageSources> | ||
<clear /> | ||
<!-- Dependencies that we can turn on to force override for testing purposes before uploading. --> | ||
<!--<add key="Static Package Dependencies" value="dep\packages" />--> | ||
<add key="TerminalDependencies" value="https://pkgs.dev.azure.com/ms/terminal/_packaging/TerminalDependencies/nuget/v3/index.json" /> | ||
<!-- !!! THIS NEEDS TO BE REMOVED BEFORE MERGE. The Microsoft.VisualStudio.Setup.Configuration.Native package needs to be added to the upstream source !!! --> | ||
<add key="nuget.org" value="https://api.nuget.org/v3/index.json"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During conflict resolution, you can finally back this one on out 😄
/// <summary> | ||
/// Takes a relative path under a Visual Studio installation and returns the absolute path. | ||
/// </summary> | ||
std::wstring VsSetupConfiguration::ResolvePath(ComPtrSetupInstance pInst, std::wstring_view relativePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit about these -- since the ComPtr argument is taken by copy, each of these causes an AddRef/Release (presumably, unless the compiler is smart?) on its argument for every call. It may not be necessary if the caller is holding an owning reference.
I believe the idiomatic way to take non-owning parameters is via raw pointer arguments (ala xxxSetupInstance*
).
A less-idiomatic way, which requires the holder to have a wil::com_ptr
themselves, is to slap a const &
on it. 😁
|
||
unsigned long long minVersion{ 0 }; | ||
unsigned long long maxVersion{ 0 }; | ||
THROW_IF_FAILED(helper->ParseVersionRange(range.data(), &minVersion, &maxVersion)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW: In our specific case, the string views we are passing are null-terminated. That's a side effect of them being compiled-in; there's no general guarantee that a string_view is null-terminated. This will work fine, but we'll want to be careful if we ever allow people to specify their own version ranges!
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Thanks so much for this! |
Sorry to bring back closed PR to life, but is there a way to setup it to run x64 prompt by default? I still have my custom profile which calls |
@kasper93 That's a good point! VS features 6 items in the start menu: |
Let's file a new issue at this point to represent adding preferences to the profile generator like which architecture is prioritized. Thanks! |
So, the way I found to do it is to edit the profile generated by this generator and add |
## Summary of the Pull Request Similar to `vswhere -latest`, show only the latest Visual Studio command prompts / developer PowerShell. This was tested by deleting the local package state and testing against fresh state with both VS2019 and VS2022 Preview installed, and indeed VS2022 Preview (both cmd and powershell) show. The other profiles were generated but hidden by default. ## References Modification of PR #7774 ## PR Checklist * [x] Closes #11307 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments The sort algorithm is the same basic algorithm I used in https://github.com/microsoft/vswhere. It sorts first by installation version with a secondary sort based on the install date in case the installation versions are the same. ## Validation Steps Performed With both VS2019 and VS2022 Preview installed, I made sure the initial state was expected, and tried different combinations of hiding and unhiding generated entries, and restarted Terminal to make sure my settings "stuck".
🎉 Handy links: |
Doc updated in response to some discussion in [#11326] and [#7774]. In those PRs, it became clear that there needs to be a simple way of collecting up a whole group of profiles automatically for sorting in these menus. Although discussion centered on how hard it would be for extensions to provide that customization themselves, the `match` statement was added as a way to allow the user to easily filter those profiles themselves. This was something we had originally considered as a "future consideration", but ultimately deemed it to be out of scope for the initial spec review. References: * #1571 * #11326 * #7774
The PowerShell equivalent was added in the initial pull request, #7774.
This commit adds dynamic profile generators for Visual Studio Developer
Command Prompt (VS2017+) and Visual Studio Developer PowerShell
(VS2019.2+)
Tested manually by deploying locally. My local environment has four
instances of VS installed, one VS2017 and multiple channels of VS2019.
We're wrapping the COM Visual Studio Setup Configuration API to query
for VS instances and retrieve the relevant properties. Two different
namespaces are used so the end-user can turn off one or the other. For
instance, end user may prefer to always use Developer PowerShell.
Validation Steps Performed
Closes #3821