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

Reimplementation of in memory profile support #180

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mitchcapper
Copy link
Contributor

#151 and #147

Not ready for committing yet. There is a problem with older vs versions (even 17.10 for example) where it throws an error. I am thinking we need an older version of the Microsoft.VisualStudio.ProjectSystem.Managed package. I used a 17.11 version so maybe that somehow only works in versions of VS newer than that.

Note this eliminates the dependency on the beta feed. It uses what I mentioned in dotnet/project-system#7010 of putting the nuget package together myself. nuget also ensures it will never go stale un-avail again.

I will try the oldest version of the package that has the in memory support see if that resolves this issue.

Note this code is not taken directly from the old branch I manually re-merged the differences in to make sure it had all the changes from master. The changes with InitConfigHandlerForSupportedProjects I am not sure if should be part of this PR but they were added originally and not yet in master so I did leave them there.

There is some chance we might be able to accomplish this with a reflection only solution. This is a bit tricky as we need to implement an interface rather than just call a function or two, but is not impossible (I believe).

I plan to post in 7010 above as well after some more testing.

@mitchcapper
Copy link
Contributor Author

Closes #172

@mitchcapper
Copy link
Contributor Author

Added dynamic support. Note I plan to still get an older package for the solution as that is preferable. The dynamic reflection looks complex but it is a one time cost for the most part as everything is cached/ compiled expressions after that.

@mitchcapper
Copy link
Contributor Author

OK this is pretty stable now. I downgraded the managed package to the oldest current version (17.7.37.99). We could go older if desired but id need a package of the nupkg file to repackage.

I disabled dynamic/reflection compiling by default as well now that it works are older VS2022 instances.

@Irame
Copy link
Collaborator

Irame commented Sep 19, 2024

Hi thanks for the work you put in and sorry for my lack of responses.

The main problem I have right now is, that I need some more feedback on v3.0.3.1. I had some major problems on v3.0 and some people suggested I roll it back. So that what I did, but now I don't have enough feedback to go live with these fixes.

Maybe you can find some people to test the new version, that would be great!

@mitchcapper
Copy link
Contributor Author

What problems happened exactly? Was this mostly around the VS Managed package?

I use this plugin pretty heavily and the only bug I am aware of I finally spent the time to track down. It was quite annoying, but I didn't realize the conditions it would happen until generating a test case for the VS team and eventually figuring out the issue. I also wasn't aware it was only with IPersistOption as I only have really used SCLA with it.

See dotnet/project-system#9536
Essentially with persist mode enabled in this plugin if you close a solution with SCLA as the launch profile and re-open it, then re-trigger SCLA to create the non persistent profile it will show in the debug launcher as active, but it won't be. This would mean you could enable command line args in the extension and they would seem to have no effect. If you changed off of it as the startup project and back onto it that would fix it.

I have kind of a work around in that bug report but still would need to figure out if we are supposed to be the default profile. The other option would be to somewhat randomize the names to avoid it saving out as the last active profile.

Both hacky so we can hold off to see what the VS team says but, I am guessing even if they fixed it, we will need to add a work around for all the old vs versions that support IPersistOption but wont have the fix. The only exception being if it is fixed in the nuget package somehow.

@Irame
Copy link
Collaborator

Irame commented Sep 24, 2024

Sorry I failed to provide the necessary context.
The Problems I was talking about had nothing to do with this feature (because it wasn't included in the 3.0 release). I had some problems with dependencies needed for the switch to a dependency injection model. Some of them occurred on specific scenarios (like not installing specific workloads) which I never even thought of testing. (#175, #177)

But I've answered every issue with a link to a new version and nobody did respond with feedback if it fixes their issues. Im now at a point where I would give it a shoot again and release it under 3.2 and see how many complaints I get.

One other thing, did you take a look at my shoot at integrating your old solution onto the new master dev/cps_inmemory_profile?

@mitchcapper
Copy link
Contributor Author

Yeah thats a pain, those two bugs sound like they could have been running an older VS. Clearly not requiring specific version should avoid the possible error but the other option is reflection and generated code based discovery and implementation. That was my code here enabled with the define of DYNAMIC_VSProjectManaged not that i'm for that.

One other thing, did you take a look at my shoot at integrating your old solution onto the new master dev/cps_inmemory_profile?

Yes sorry to be clear I rebased onto master and used that code for reference. With that as the basis for this PR hopefully should merge pretty cleanly.

I would like to see the VS teams response on dotnet/project-system#9536 before you release in memory profiles otherwise we should include a work around for the bug as it is frustrating otherwise.

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