Skip to content

Clean up dependencies projects#25609

Merged
tmat merged 6 commits intodotnet:dev15.7.xfrom
jasonmalinowski:clean-up-dependencies-projects
Mar 20, 2018
Merged

Clean up dependencies projects#25609
tmat merged 6 commits intodotnet:dev15.7.xfrom
jasonmalinowski:clean-up-dependencies-projects

Conversation

@jasonmalinowski
Copy link
Member

@jasonmalinowski jasonmalinowski commented Mar 19, 2018

Infrastructure only change.

This cleans up our dependencies projects which are used to determine the list of facades we put into PortableFacades. Previously, we had two projects, DevDivPackagesRoslyn (which would state the things we depended on) and DevDivPackagesDebugger, which is what the Debugger depends on that we're responsible for handling their dependencies. A few things had gotten screwed up in this process:

  • DevDivPackagesRoslyn would have to be kept in sync with VisualStudioSetup.Dependencies (which is essentially the F5 equivalent of PortableFacades, so should list the same thing)
  • You could add things to CompilerExtension and it wouldn't end up anywhere.
  • The projects didn't have dependencies to each other, so bumping one version in one place could result in "fun" version conflicts that NuGet would have resolved just fine if it had a proper reference.

Copy link
Member

Choose a reason for hiding this comment

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

Is this a functional or style consistency change? If it's the former then I may update BuildBoss to catch it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Style consistency.

Copy link
Member

Choose a reason for hiding this comment

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

Why are you removing DevdiVPackagesDebugger? Had that in this solution to make the inner loop here easy:

  • Restore DevDivInsertionFiles.sln
  • F5

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR changes your loop to restore Roslyn.sln, F5. That's not changing either way, so no reason to have it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

(and yes the restore is more expensive there. But now it means compiler dependencies are actually being considered, so the next time somebody from your team adds a dependency there's a chance the right thing will happen, instead of nothing happening)

Copy link
Member

Choose a reason for hiding this comment

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

DevDivInsertionFiles.vbproj is not in Roslyn.sln. It would need to be added there to keep the loop you subscribe. I'm fine with doing that but probably should delete DevDivInsertionFiles.sln at the same time. It's only purpose was to support that specific developer loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only purpose was to support that specific developer loop.

Well, when DevDivInsertionFiles.sln was created, Roslyn wasn't even using NuGet -- we were just checking in references into our TFVC repo... 😄

The restore inputs of DevDivInsertionFiles.vb isn't needed as an input in anything anyways -- F5 would probably still do it. I'm happy to delete the solution file and move the project in if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

The restore inputs of DevDivInsertionFiles.vb isn't needed as an input in anything anyways

Sure but the restore of what used to be DevDivInsertionFiles.sln was a nice inner dev loop and a functional requirement in our old setup. Given the deletion of the project references the SLN file can't be restored + F5'd and the project itself doesn't need to be restored hence the SLN is pointless now.

I'm happy to delete the solution file and move the project in if you'd like.

I'd prefer that. It was my eventual plan anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the functional requirement was undocumented, and perhaps only known by you. 😉

Will do the deletion, but going to do that in a separate PR. @tmat wants this in to facilitate the System.Collections.Immutable cleanup (as he'll hit the same things I did without this), and I don't want to delay this one much further than necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the functional requirement was undocumented, and perhaps only known by you.

And the test infrastructure that I wrote to make sure it worked 😄

Will do the deletion, but going to do that in a separate PR.

Okay

This contained a list of packages that we depended on and needed
facades deployed into Visual Studio. We already have
VisualStudioSetup.Dependencies that already (should have...) contained
all the facades and more. We can just consume that project instead
of having yet another duplication of facade lists.
…t include

This ensures that if the compiler adds a new facade but we don't update
VisualStudioSetup.Dependencies (either intentionally or by accident)
we still will include the right facades.
@jasonmalinowski jasonmalinowski force-pushed the clean-up-dependencies-projects branch from a6655d6 to ed6497f Compare March 20, 2018 00:24
By doing this, the VisualStudioSetup.Dependencies.csproj will now
figure out the highest version of our own dependencies and the debugger
dependencies, and create a single package that we can deploy that
contains all of them.
@jasonmalinowski jasonmalinowski force-pushed the clean-up-dependencies-projects branch from ed6497f to 9b20d50 Compare March 20, 2018 00:39
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@jaredpar
Copy link
Member

Going to need to delete this line:

https://github.com/dotnet/roslyn/blob/master/src/Setup/DevDivPackages/Debugger/DevDivPackagesDebugger.csproj#L7

Looks like BuildBoss wasn't running on DevDivInsertionFiles.sln hence items ilke this got missed.

@jasonmalinowski
Copy link
Member Author

@tmat will continue to monitor this but I have no further changes. If it's green, feel free to merge before I do.

@tmat tmat merged commit fd87d3d into dotnet:dev15.7.x Mar 20, 2018
@jasonmalinowski jasonmalinowski deleted the clean-up-dependencies-projects branch March 20, 2018 02:24
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.

3 participants