Don't require refout to produce marker #2180
Conversation
|
Yes, where does it need to go/what approvals do we need to get? Thanks! |
|
Assuming we take #2182 to 15.3, this should be fine--I changed the target branch for you. I'll drive with the team. You can patch your VS's copy of this file with this change to unblock testing. |
|
@rainersigwald I just pushed another update. I realized as I was working on the project system changes that we should create the marker if the project produces a ref assembly or if it consumes a ref assembly. In the case above, if ClassLibrary1 doesn't produce a marker, then if only its implementation dll changes ClassLibrary2 won't know that it needs to rebuild to copy the new dll and touch its marker. |
| references on this build, subsequent builds of projects that depend on it must | ||
| not be considered up to date, so touch this marker file that is considered an | ||
| input to projects that reference this one. --> | ||
| <Touch Files="$(CopyUpToDateMarker)" | ||
| AlwaysCreate="true" | ||
| Condition="'$(ProduceReferenceAssembly)' == 'true' and '@(ReferencesCopiedInThisBuild)' != ''" /> | ||
| Condition="'$(ProduceReferenceAssembly)' == 'true' or '@(ReferencesCopiedInThisBuild)' != ''" /> |
There was a problem hiding this comment.
I don't think this is ok--won't it always invalidate everything downstream of any project that produces a reference assembly?
There was a problem hiding this comment.
Isn't that correct, though? In the situation where
ClassLibrary1 -> ClassLibrary2 -> ConsoleApp1
If I build everything and then just change the implementation of ClassLibrary1, if we don't touch the up to date marker, then the project system up to date check will think that ClassLibrary2 is up to date since ClassLibrary1's ref assembly will be older than ClassLibrary2.dll. So it won't build ClassLibrary2, so ClassLibrary1.dll won't get copied and ClassLibrary2's marker won't be touched. So ConsoleApp1 won't be built and will run with the old ClassLibrary1.
Touching ClassLibrary1's marker won't cause ClassLibrary2.dll to be rebuilt (since MSBuild will see it as up to date since it's comparing the ref assembly), it will just cause ClassLibrary1.dll to be copied and the marker to be touched. Which seems correct.
There was a problem hiding this comment.
In VS the ClassLibrary1 marker wouldn't be updated on a noop build because MSBuild wouldn't be invoked. On the command line the markers would be updated for every project on every build, which is unnecessary I/O, but nothing should look at them so it's probably not the end of the world.
The design we were working with dealt with this scenario by adding the implementation assemblies to the list of FUTD inputs. Can you elaborate on why you prefer this design?
There was a problem hiding this comment.
Oh, ok. The design process of this feature has kinda been a big game of Telephone, so I came away with a different understanding of how this situation would be dealt with but reading the design again, I see what I missed. OK, I'll roll back to the original fix.
|
I just pushed another commit because the Touch tasked was wrapped in a target that only runs when there are copy local references. |
|
@dotnet-bot test Windows_NT Build for CoreCLR please Odd, I don't recognize failures in |
|
@panopticoncentral Do you still need this change if you add the (impl of ref) to (impl of this project) comparison? Won't that cover your original scenario as well? |
|
No, I don't think so. Let me walk through it, though, in case I've got it wrong. Given: ClassLibrary1 -> ClassLibrary2 -> ConsoleApp1 Where ClassLibrary2 references ClassLibrary1 and ConsoleApp1 references only ClassLibrary2. And both ClassLibrary1 and ClassLibrary2 output ref assemblies. So we do a full build, touch the implementation of ClassLibrary1, and then build.
The problem in this situation is that now if you do another build, ConsoleApp1 will fail the up to date check and copy ref\ClassLibrary2.dll again, because ClassLibrary2.marker will always be newer than ConsoleApp1.dll. If ConsoleApp1 had a marker, though, then we would just compare the markers and know it was up to date. Or am I missing something? |
There was a problem hiding this comment.
You're right, that timeline was just what I needed. To go back to dotnet/project-system#2254 this was the whole reason for the new (yellow) comparison; I failed to account for the (non-ref-assembly-producing) -> (updated-only-by-copying-ref) case.
@AndyGerlicher this should go in for preview 3.
|
There may an additional problem, and I wanted to check with you. In FindReferenceAssembliesForReferences, it looks like the task creates an UpToDateCheckInput item for the marker of a ref assembly reference. I think this a problem because my understanding of UpToDateCheckInput (which I'm also working on implementing) is that those items are supposed to be checked against all of the outputs. Whereas markers are only supposed to be compared to markers. So it looks like we shouldn't create an UpToDateCheckInput item and up-to-date checkers are just going to have to special case the marker. Do I have that right, or am I misunderstanding things? |
|
@panopticoncentral That is a correct understanding of the initial implementation, but I believe I already fixed it in #2167 here :) In addition, I don't think it mattered because |
|
Awesome! Correct, it doesn't matter for the old system, but in the new system we're gathering from design-time builds as well, so it would matter there. |
|
Oh, good to know . . . and better behavior! |
|
Per @AndyGerlicher's request, I'm going to take this to a feature branch and run RPS tests on it (since it involves extra I/O for every project). |
|
RPS looks good. I resolved merge conflicts, this can go in after CI passes. |
|
@dotnet-bot test OSX Build for CoreCLR please |
PR dotnet#2180 caused a performance-test scenario internal to Microsoft to start failing. The root cause was that the projects in the test customized $(IntermediateOutputPath) _very_ late, after importing CSharp.targets (and transitively Common.targets). That meant that the path stored in $(CopyOutOfDateMarker) was no longer under $(IntermediateOutputPath), and that the directory wasn't created before the new code attempted to touch the marker file. Ideally, projects would set IntermediateOutputPath _before_ importing common.targets, but that hasn't been strictly required in all cases before so requiring it now is an unacceptable regression. The late customization was handled for other intermediate outputs by defining their paths with items (expanded after all properties are expanded) rather than properties (expanded in order). There are other properties defined by using $(IntermediateOutputPath), $(_GenerateBindingRedirectsIntermediateAppConfig) and $(WinMDExpOutputPdb). This PR doesn't change those since they weren't recently regressed.
PR #2180 caused a performance-test scenario internal to Microsoft to start failing. The root cause was that the projects in the test customized $(IntermediateOutputPath) _very_ late, after importing CSharp.targets (and transitively Common.targets). That meant that the path stored in $(CopyOutOfDateMarker) was no longer under $(IntermediateOutputPath), and that the directory wasn't created before the new code attempted to touch the marker file. Ideally, projects would set IntermediateOutputPath _before_ importing common.targets, but that hasn't been strictly required in all cases before so requiring it now is an unacceptable regression. The late customization was handled for other intermediate outputs by defining their paths with items (expanded after all properties are expanded) rather than properties (expanded in order). There are other properties defined by using $(IntermediateOutputPath), $(_GenerateBindingRedirectsIntermediateAppConfig) and $(WinMDExpOutputPdb). This PR doesn't change those since they weren't recently regressed.
The behavior of MSBuild is at odds with the design discussed by @jcouv in dotnet/project-system#2254. In the design, any project that references a ref assembly will create an up to date marker in it's intermediate output path. Currently only projects that produces ref assemblies and reference ref assemblies create the up to date marker. This isn't sufficient because if you have:
ClassLibrary1 -> ClassLibrary2 -> ConsoleApp1 (where CL1 and CL2 produce ref assemblies)
Then ClassLibrary2 will produce a marker but ConsoleApp1 won't. So there's nothing to compare ClassLibrary2's marker against.
@rainersigwald it looks like just removing the check for producing a ref assembly should be sufficient, what do you think?