Skip to content

Fix Visual studio release build #3761

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
Merged
merged 5 commits into from
Mar 4, 2021
Merged

Fix Visual studio release build #3761

merged 5 commits into from
Mar 4, 2021

Conversation

Rosuavio
Copy link
Contributor

Fixes local release build

Building on release mode locally does not work. This fixes it.

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

WARNING

This removes a TODO maybe we want to reintroduce the functionally before merging?

@Rosuavio Rosuavio added bug 🐛 An unexpected issue that highlights incorrect behavior build 🔥 labels Feb 17, 2021
@ghost
Copy link

ghost commented Feb 17, 2021

Thanks RosarioPulella for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Kyaa-dost February 17, 2021 17:35
@Rosuavio Rosuavio changed the title Fix local release build Fix Visual studio release build Feb 17, 2021
michael-hawker
michael-hawker reviewed Feb 25, 2021
View reviewed changes
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ViewportBehavior/ViewportBehaviorPage.xaml.cs
@@ -67,28 +65,13 @@ private void ClearLogsButton_Click(object sender, RoutedEventArgs e)
_logs.Clear();
}

private async void EffectElementHost_EnteredViewport(object sender, EventArgs e)
{
AddLog("Entered viewport");
Copy link
Member

Choose a reason for hiding this comment

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

This log thing shows up in the example, reason this was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It it was not doing anything other than logging, and the compiler was complaining about the function being async with no await. I can just bring it back for now and remove the async. Then when the animation gets reintroduced it would be simpler.

@michael-hawker
Copy link
Member

I'm also seeing a lot of weird StyleCop issues flagged (which shouldn't be as we don't use this. prefix in our guidelines/rules):

image

(in SampleController.xaml.cs)

Is this just my local environment messed up somehow?

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Feb 26, 2021 •
edited
Loading

Is this just my local environment messed up somehow?

I was trying to get it building again for now.

marcelwgn
marcelwgn approved these changes Feb 26, 2021
View reviewed changes
Copy link
Contributor

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@michael-hawker
Copy link
Member

@RosarioPulella what build configuration are you trying?

For Any CPU I'm not seeing it run the app automatically when I click run in VS, and if I try and deploy manually I get this:

1>------ Skipped Build: Project: Microsoft.Toolkit.Uwp.Samples.BackgroundTasks, Configuration: Release Any CPU ------
1>Project not selected to build for this solution configuration 
2>------ Build started: Project: Microsoft.Toolkit.Uwp.SampleApp, Configuration: Release x86 ------
2>H:\code\WindowsCommunityToolkit\Microsoft.Toolkit.Uwp.SampleApp\Microsoft.Toolkit.Uwp.SampleApp.csproj : XamlCompiler error WMC1006: Cannot resolve Assembly or Windows Metadata file 'H:\code\WindowsCommunityToolkit\Microsoft.Toolkit.Uwp.Samples.BackgroundTasks\bin\Release\Microsoft.Toolkit.Uwp.Samples.BackgroundTasks.winmd'
2>CSC : error CS0006: Metadata file 'H:\code\WindowsCommunityToolkit\Microsoft.Toolkit.Uwp.Samples.BackgroundTasks\bin\Release\Microsoft.Toolkit.Uwp.Samples.BackgroundTasks.winmd' could not be found
2>C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\XamlCompiler\Microsoft.Windows.UI.Xaml.Common.targets(486,5): error MSB4181: The "CompileXaml" task returned false but did not log an error.
========== Build: 0 succeeded, 1 failed, 17 up-to-date, 1 skipped ==========
========== Deploy: 0 succeeded, 0 failed, 0 skipped ==========

> Build started at 1:42 PM and took 29.940 seconds

I was able to build for x64 though. 🙂

@marcelwgn
Copy link
Contributor

@michael-hawker Is Any CPU supposed to work in release anyway? I though it would compile into platform dependent binaries, so Any CPU wouldn't work in there right? Any CPU also isn't an option when creating a blank UWP project so I was surprised that was even an option in VS.

@michael-hawker
Copy link
Member

@chingucoding we have an Any CPU for Debug, it falls back to x86 where required. I guess they may have removed it from the most recent template, but we still have it as part of our configuration.

It's most likely a missing checkbox in the build configuration dialog of something that's not checked to build.

@marcelwgn
Copy link
Contributor

Ah I see, thanks for the explanation @michael-hawker!

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Mar 2, 2021

Yeah I was not trying to fix building Release|Any CPU with this PR. Just to clarify, do we want to remove Release|Any CPU as an option or make it work?

@michael-hawker michael-hawker added this to the 7.0 milestone Mar 2, 2021
@michael-hawker
Copy link
Member

Let's merge this now, and we'll see if I encounter an issue when packaging the app and can forward-fix there. Thanks @RosarioPulella!

@michael-hawker michael-hawker merged commit daa8869 into CommunityToolkit:master Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelwgn marcelwgn marcelwgn approved these changes

@michael-hawker michael-hawker michael-hawker approved these changes

@azchohfi azchohfi Awaiting requested review from azchohfi

@Kyaa-dost Kyaa-dost Awaiting requested review from Kyaa-dost

Assignees
No one assigned
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior build 🔥
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Rosuavio @michael-hawker @marcelwgn