Fix ResolveProjectReferences MSBuild target. #64
Conversation
src/WixToolset.Sdk/tools/wix.targets
Outdated
| @@ -351,7 +350,7 @@ | |||
| Projects="@(_MSBuildProjectReferenceExistent)" | |||
| Targets="%(_MSBuildProjectReferenceExistent.Targets);GetTargetPath" | |||
| Properties="%(_MSBuildProjectReferenceExistent.SetConfiguration);%(_MSBuildProjectReferenceExistent.SetPlatform)" | |||
| Condition="('$(BuildingSolutionFile)' == 'true' or '$(BuildingInsideVisualStudio)' == 'true' or '$(BuildProjectReferences)' != 'true') and '@(_MSBuildProjectReferenceExistent)' != '' " | |||
| Condition="('$(BuildingInsideVisualStudio)' == 'true' or '$(BuildProjectReferences)' != 'true') and '@(_MSBuildProjectReferenceExistent)' != '' " | |||
There was a problem hiding this comment.
Thanks for looking at this. v3 created this since we didn't use Common.targets, but in v4 we are using it. This target probably needs to match the canonical version at https://github.com/dotnet/msbuild/blob/83cd7d4e36b71d5b2cefd02cb9a5a58d27dd6a75/src/Tasks/Microsoft.Common.CurrentVersion.targets#L1914. It would be nice if we could extend that one instead of overwriting it with our own.
There was a problem hiding this comment.
@rseanhall I totally agree. Do you think it'll be sufficient to run standard ResolveProjectReferences and then filter out Wix library references into WixLibProjects?
There was a problem hiding this comment.
I was probably going to defer to others to handle this PR since I have no knowledge of what the customization is supposed to be doing.
There was a problem hiding this comment.
I have a very large work item post 4.0-preview.0 to review our .target files from top to bottom. There are many little problems that sum up to a larger feeling that there is a large re-work required. At this time, I'm inclined to leave this line here as a throwback to WiX v3 that should be accounted (and every possibly tossed).
There was a problem hiding this comment.
If we're not going to fix this properly in 4.0-preview.0, then if someone does the work to put our customization inside the latest ResolveProjectReferences from Common.targets then I think we should take it.
There was a problem hiding this comment.
Wixlib stuff is generation of Wixlib references. And it's still being done in the ResolveWixLibraryReferences target which is called after the vanilla ResolveProjectReferences.
There was a problem hiding this comment.
@robmen @barnson @rseanhall Do you have ancient Wix source? I'd like to investigate the evolution of wix.targets before v3.8.
There was a problem hiding this comment.
There was a problem hiding this comment.
OK. So there are two commits which distinguish ResolveProjectReferences from the vanilla version (apart from the wixlib filtering).
The biggest one is wixtoolset/codeplex@9781a18.
Here the comment from @robmen says Add support to build only .wixlib project references.
Correct me if I'm wrong, but apparently this is not used anywhere. I haven't seen any place in the code, neither present nor past where BuildProjectReferences is set to wixlib. That was almost exactly 10 years ago.
The next one is wixtoolset/codeplex@975c902.
Apparently, this a workaround for the bug described here: https://devblogs.microsoft.com/visualstudio/incorrect-solution-build-ordering-when-using-msbuild-exe/
However, I need to confirm it.
Other then that, it's only the mentioned logic for populating WixLibProjects which I moved to ResolveWixLibraryReferences.
There was a problem hiding this comment.
@rseanhall Reimplemented according to your suggestion. wixtoolset/codeplex@975c902 was necessary, so had to keep the old structure.
|
I wonder if this is the root cause behind wixtoolset/issues#6290. |
drolevar
force-pushed
the
fix_msbuild_targets
branch
2 times, most recently
from
df5d501 to
52dcf94
Compare
By the way, it seems to fix this issue. At least if I remove the workaround target, it seems to build and all the tests complete successfully. |
drolevar
force-pushed
the
fix_msbuild_targets
branch
from
52dcf94 to
c70e122
Compare
When building a solution from command line with MSBuild later than 4.0 it doesn't build the complete project tree like it used to do before.
So GetTargetPath might be called before the project is built, which in certain situations leads to an error.
I have discovered this issue when I was doing a command line build of the solution, where WiX projects were referencing Dotfuscator-enabled C# projects. And in this case ResolveProjectReferences target from wix.targets was calling GetTargetPath on such Dotfuscator-enabled projects, which was triggering a dotfuscation before the project was built. This failed of course.