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.

Sign up for GitHub

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

Jump to bottom

Improve wix.targets compatibility with Microsoft.Common.targets #60

Merged
merged 4 commits into from Dec 30, 2021
Merged

Improve wix.targets compatibility with Microsoft.Common.targets #60

merged 4 commits into from Dec 30, 2021

Conversation

robmen
Copy link
Member

@robmen robmen commented Dec 10, 2021 •
edited

@robmen robmen force-pushed the robmen/wix-targets branch 5 times, most recently from fe1e433 to ec3749a Compare December 14, 2021 23:59
@robmen robmen force-pushed the robmen/wix-targets branch 2 times, most recently from 8cfe93e to 1cb5419 Compare December 21, 2021 06:21
rseanhall
rseanhall reviewed Dec 22, 2021
View reviewed changes
src/api/api.cmd Outdated
dotnet test -c %_C% --no-build burn\test\WixToolsetTest.Mba.Core\WixToolsetTest.Mba.Core.csproj || exit /b
dotnet test -c %_C% --no-build wix\api_wix.sln || exit /b
dotnet test -c %_C% --no-build --nologo -v m burn\test\WixToolsetTest.Mba.Core\WixToolsetTest.Mba.Core.csproj || exit /b
dotnet test -c %_C% --no-build --nologo -v m wix\api_wix.sln || exit /b
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you already put this in some places earlier, but why do we want minimal verbosity? If something goes wrong, I don't want to have to run another build to find out why.

Copy link
Member

Choose a reason for hiding this comment

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

Error messages still show up. -v m just hides the spew from compiler invocations and such.

In a dream world, there's a single MSBuild invocation for the whole world so you could easily specify additional switches (e.g., /bl).

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'm still planning to add /bl before I'm done with this PR. I find those are the most enjoyable way to diagnose build failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am definitely not looking forward to downloading binary logs to find out why the build failed on the CI server.

Copy link
Member

Choose a reason for hiding this comment

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

They're smaller than diag-level text logs -- I want binlogs for Burn!

@robmen robmen force-pushed the robmen/wix-targets branch 4 times, most recently from 8712b67 to 080919f Compare December 28, 2021 16:55
@rseanhall
Copy link
Contributor

I don't know why @barnson still added the references to Microsoft.NETFramework.ReferenceAssemblies in #54. I had purposely not included them in my VS2022 PR, and removed it from Directory.Packages.props.pp, because of these NU1009 errors. The only purpose I can see for including them is for machines that don't have the .NET Framework targeting pack.

@barnson
Copy link
Member

barnson commented Dec 28, 2021

Because the GH build failed without them: https://github.com/wixtoolset/wix4/runs/4639436215?check_suite_focus=true

@robmen
Copy link
Member Author

robmen commented Dec 28, 2021

Yeah. I can see the appeal of not needing to install targeting packs (especially the old ones) but my patience with Traversal targets ignoring their DisableImplicitFrameworkReferences setting is rapidly wearing thin.

@rseanhall
Copy link
Contributor

Because the GH build failed without them: https://github.com/wixtoolset/wix4/runs/4639436215?check_suite_focus=true

Huh? The build failed because you tried to add Microsoft.NETFramework.ReferenceAssemblies back to Directory.Packages.props.pp: 5d169aa

@robmen robmen force-pushed the robmen/wix-targets branch 2 times, most recently from 1717d09 to 01aff3f Compare December 29, 2021 19:37
@rseanhall
Copy link
Contributor

I expected this to fail the build:

D:\a\wix4\wix4\src\test\burn\WixToolset.WixBA\WixToolset.WixBA.csproj : error NU1009: The packages Microsoft.NETFramework.ReferenceAssemblies are implicitly referenced. You do not typically need to reference them from your project or in your central package versions management file. For more information, see https://aka.ms/sdkimplicitrefs [D:\a\wix4\wix4\src\test\burn\BurnE2ETests.sln]

@robmen
Copy link
Member Author

robmen commented Dec 29, 2021

Whoa! So did I. It failed the last "build improvements" PR. I will force revert this one real fast.

@robmen
Copy link
Member Author

robmen commented Dec 29, 2021

Oh, thank goodness. It didn't merge. I still need to investigate the root remaining failure in #77.

@robmen
Significant rewrite of wix.targets to improve compatibility
3ace529 
This is still a work in progress but the wix.targets are now much
more compatible with other project systems. The focus has been on
removing customizations to leverage MS.Common.targets.
@robmen
Simplify and normalize ext projects
8dd4d5f 
Now that wix.targets is more compatible with MS.Common.targets
the extension projects can be simplified. Also made their project
files more consistent with each other.
@robmen
Integration tests for wixproj Sdk-style builds
4833d5e
@robmen robmen marked this pull request as ready for review December 30, 2021 20:10
@robmen robmen enabled auto-merge (rebase) December 30, 2021 20:10
@robmen robmen merged commit 07599b2 into develop Dec 30, 2021
@robmen robmen deleted the robmen/wix-targets branch December 30, 2021 20:51
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@barnson barnson barnson left review comments

@rseanhall rseanhall rseanhall left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@robmen @rseanhall @barnson