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

Add Referenced Projects #160

Merged
merged 3 commits into from
Jan 12, 2021
Merged

Conversation

slang25
Copy link
Contributor

@slang25 slang25 commented Jan 2, 2021

Fixes #159

This change adds to the functionality when you set addProjectReferences to true. At the moment it will follow all references where you have already added the project to the solution, but if you want to create an ad-hoc solution with a csproj as the entry point, I think you'll want this behaviour to automatically add references projects to the solution.

Just to be clear, the code change code (in GetReferencedAnalyzerProjects) is only called when addProjectReferences is true.

@slang25 slang25 marked this pull request as ready for review January 3, 2021 01:36
@@ -59,7 +59,7 @@ public void CreatesCompilationOptions()
}

[TestCase(false, 1)]
[TestCase(true, 2)]
[TestCase(true, 3)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Essentially this is the change in behavior, you don't need to add child projects now when addProjectReferences is used. If the old behavior is still desired, perhaps we should change the parameter to an enum and support both variants?

@slang25
Copy link
Contributor Author

slang25 commented Jan 8, 2021

To counteract the guilt I'd feel bumping this PR, I bought you a coffee 😆

@daveaglick
Copy link
Collaborator

Lol. Was already planning to take a look at the open Buildalyzer PRs this weekend, but the beverage is appreciated!

@slang25
Copy link
Contributor Author

slang25 commented Jan 9, 2021

😄 it's the least I could do.

I think I may not need this anymore, what I need is basically this:
https://github.com/aws/codelyzer/blob/acd3c02c4b41aa0a14393fcf0ae6f4f145212f97/src/Analysis/Codelyzer.Analysis.Build/WorkspaceBuilderHelper.cs#L74-L113

but I want it to happen out of the box. When I use this PR alone, doesn't give the right results from a clean build, because I've not built the projects in the right order.

@daveaglick
Copy link
Collaborator

Tell me more about

When I use this PR alone, doesn't give the right results from a clean build, because I've not built the projects in the right order.

Is the idea that sometimes analyzerResult.Manager.GetProject(x) won't have the project yet and so we need an extra flow where if the project hasn't already been added to the workspace, and the manager hasn't yet built it, we should go off and build it and then add it?

That seems totally doable, though I think you're right that we'd want to put that behind a flag (probably combined with the behavior in this PR - something like bool buildMissingReferences.

@daveaglick daveaglick merged commit b879288 into phmonte:main Jan 12, 2021
@daveaglick
Copy link
Collaborator

I guess the part I'm not quite understanding is:

  • GetReferencedAnalyzerProjects() now adds the project file to the manager (after this PR) if it wasn't already there
  • Then we loop through those projects (including missing ones thanks to this PR) and call ProjectAnalyzer.AddToWorkspace()
  • Inside AddToWorkspace() we call return analyzer.Build().FirstOrDefault().AddToWorkspace(workspace, addProjectReferences); which seems like it should do what you're thinking and build any projects that weren't already built.

So my disconnect is why that doesn't work as expected to build projects you're missing once this PR is merged.

@daveaglick
Copy link
Collaborator

I think I get it...is the problem that we don't pick up on transitive references? I.e. if I'm adding project A to the workspace and if references project B, I'll add and build B. But if B also references C, I'll never add and build C to the workspace.

@daveaglick
Copy link
Collaborator

I'm going to bring this discussion back to the original issue...

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.

Automatically load ProjectReferences
2 participants