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

Fixes issues with the build script in detecting dump files to upload and analyze #174

Merged
merged 5 commits into from
Aug 17, 2023

Conversation

michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented Aug 14, 2023

#170 failed on main here: https://github.com/CommunityToolkit/Windows/actions/runs/5828716207/job/15806953245

image

But the dump detection, upload, and auto-analysis didn't run.

I missed that the plugin (dorny/paths-filter) I picked doesn't work on untracked files, only git files. So using GitHub Action output to search for a dump file and use that for analysis.

Will see if this build fails at least once for a test... (the rare time you want something to fail)...

Fixes CommunityToolkit/Tooling-Windows-Submodule#7 in c18dc07

@michael-hawker
Copy link
Member Author

Was using linux command, but the default Windows shell is PowerShell, so adjusted command, should be good now.

Also VS Code kept modifying the config file due to the new dotnet environment they shipped, so made change here so it stops modifying it locally all the time.

@michael-hawker
Copy link
Member Author

Ok, didn't realize PowerShell would throw an error and stop things inline like that, added SilentlyContinue to the command, so now should be blank as expected if no files found.

@michael-hawker
Copy link
Member Author

Added always flag for detecting the dump file.

@michael-hawker
Copy link
Member Author

Still not doing what I expect with the detection... outputting some tests

Removed the vs code config stuff, something happened with an infinite loop for me locally suddenly with the new C# extension (conflict between need for msbuild as winappsdk vs. it trying to use dotnet or something...) We'll have to look into that later for #101

@michael-hawker michael-hawker force-pushed the fix-dump-detect branch 3 times, most recently from f842422 to 764a75b Compare August 15, 2023 22:45
@michael-hawker
Copy link
Member Author

Think I've got the build stuff sorted now with the auto-dump analysis (though doesn't seem as helpful in these test scenarios without more commands), though adding /Blame to the test runner does provide nicer output of the crash.

Tried ignoring a few tests that pop-up in the logs, but really think this is a memory constriction type issue.

@michael-hawker
Copy link
Member Author

Rebased on top of #173 in order to get preview feed packages so I can check NuGet icon whilst I'm doing builds.

@michael-hawker
Copy link
Member Author

@Arlodotexe do you want us to merge this into your .NET 7 branch as it should be more stable with the few disabled tests hopefully (or at least better report ones that are still problematic)?

@Arlodotexe
Copy link
Member

If the CI keeps failing on #117, I will do that. Otherwise I plan on closing #117 and then closing this.

@michael-hawker
Copy link
Member Author

Hmm, filename change for casing didn't make a difference. The iconUrl is showing up in the manifest though, so it's something specific to do with PackageIcon... we're doing everything according to the docs, so not sure what's wrong here and why that isn't showing up in the package metadata... Going to try updating the NuGet Pack lib and see what happens. Think SourceLink is changing too, but we can investigate that later too, maybe that's a .NET 8 thing...

…and analyze

dorny/paths-filter only works on detecting changes to git files not untracked files
Add Blame to test runner and upload of sequence file
… all examples show lowercase?

Also, add PackageIconUrl back as acceptable to have both for backwards compat
… it's just random based on number of tests run...)

Need to comment out as Ignore ignored... see CommunityToolkit/Tooling-Windows-Submodule#121
Related to investigation, see info in actions/runner-images#7937
See if this helps with `PackageIcon`
@michael-hawker
Copy link
Member Author

Alright, rebased back on top of main so we can merge ahead of #173 if needed.

@michael-hawker michael-hawker enabled auto-merge (rebase) August 17, 2023 00:09
@niels9001 niels9001 self-requested a review August 17, 2023 06:25
@niels9001
Copy link
Collaborator

@michael-hawker Nit: Nuget recommends a 128x128px icon vs. the 256 we use now? But I'll PR and update that so this can go through!

@michael-hawker michael-hawker merged commit c1e2f66 into main Aug 17, 2023
7 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fix-dump-detect branch August 17, 2023 06:26
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.

Fix NuGet pack
3 participants