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

Only include DesignTimeFacadeAssemblies which are not in ReferencePath #1520

Merged
merged 2 commits into from
Jan 10, 2017

Conversation

cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Jan 7, 2017

Resolves #1345

@cdmihai
Copy link
Contributor Author

cdmihai commented Jan 7, 2017

@cdmihai
Copy link
Contributor Author

cdmihai commented Jan 8, 2017

@dotnet-bot test Windows_NT Build for Desktop please

@ericstj
Copy link
Member

ericstj commented Jan 9, 2017

I was taking another look at this the other day, and while I think this is the smallest tactical fix I noticed something that might be better.

ImplicitlyExpandDesignTimeFacades always adds all facades and causes conflicts, but I noticed that RAR is already getting the path to the facades via $(TargetFrameworkDirectory). I then see that RAR actually selects the subset of facades that are actually needed and outputs them via ReferenceDependencyPaths. What surprised me is that we didn't pass this item to CSC, but other compilers did accept this item. @davkean / @weshaggard do you remember why that is the case?

@weshaggard
Copy link
Member

@weshaggard do you remember why that is the case?

No I don't recall why that is the case. I didn't even know RAR was getting the path to the facades at all.

@cdmihai
Copy link
Contributor Author

cdmihai commented Jan 9, 2017

If that's the case, then it looks like ImplicitlyExpandDesignTimeFacades would become obsolete if only ReferenceDependencyPaths flowed into the the compiler. Right?

@ericstj
Copy link
Member

ericstj commented Jan 9, 2017

@cdmihai yeah, I think so. I just don't know why that wasn't done to begin with. Maybe @davkean or someone with more history in this codebase could chime in.

@davkean
Copy link
Member

davkean commented Jan 9, 2017

Yeah I don't history on this - I wasn't aware of it.

@cdmihai
Copy link
Contributor Author

cdmihai commented Jan 9, 2017

Including some of the committers from the new home of the csc target, maybe they know more.

@tannergooding, @agocke, @AArnott, do you know more about @ericstj's proposal of deleting the ImplicitlyExpandDesignTimeFacades target and changing the CSC task invocation to include the output of RAR, ReferenceDependencyPaths, which supposedly contains the target framework facade dlls? If not, I'll start experimenting with it and see what breaks. The risky part of the change is unknown consequences, since a large population of things flow through RAR and CSC :)

@agocke
Copy link
Member

agocke commented Jan 10, 2017

@cdmihai That code is before my time, so unfortunately my advice is just try it and see :)

@cdmihai
Copy link
Contributor Author

cdmihai commented Jan 10, 2017

@dotnet-bot test Windows_NT Build for Desktop please

1 similar comment
@AndyGerlicher
Copy link
Contributor

@dotnet-bot test Windows_NT Build for Desktop please

@AArnott
Copy link
Contributor

AArnott commented Jan 10, 2017

I'm afraid I'm not this deeply familiar with resolving references. @crmann1 may know.

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.

9 participants