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

[PERF] Finish fixing PerfBDNApp, continuation of #89057 #89135

Merged
merged 7 commits into from
Jul 20, 2023

Conversation

LoopedBard3
Copy link
Member

@LoopedBard3 LoopedBard3 commented Jul 18, 2023

Continuation of #89057 as it didn't completely fix the issue. Slightly reorders the build steps in the yml and grabs the BDNVersion used in the performance repo in order to inject the version into the maui PerfBDNApp build csproj. This is necessary because the Versions.props file from the performance repo is not available when building the mobile app.

Test with successful build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2224698&view=results

…l out the BDNVersion value from the performance Versions.props.
@ghost
Copy link

ghost commented Jul 18, 2023

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Continuation of https://github.com/dotnet/runtime/pull/89057/files as it didn't completely fix the issue. Slightly reorders the build steps in the yml and grabs the BDNVersion used in the performance repo in order to inject the version into the maui PerfBDNApp build csproj. This is necessary because the Versions.props file from the performance repo is not available when building the mobile app.

Test run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2224698&view=results

Author: LoopedBard3
Assignees: LoopedBard3
Labels:

area-Infrastructure

Milestone: -

Copy link
Contributor

@cincuranet cincuranet left a comment

Choose a reason for hiding this comment

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

Few small notes. Otherwise LGTM.

- pwsh: |
$BenchmarkDotNetVersionCapture = Get-Content .\performance\eng\Versions.props | Select-String -Pattern '<BenchmarkDotNetVersion>(.*)</BenchmarkDotNetVersion>'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use (.+?) for "tighter" match.

- pwsh: |
$BenchmarkDotNetVersionCapture = Get-Content .\performance\eng\Versions.props | Select-String -Pattern '<BenchmarkDotNetVersion>(.*)</BenchmarkDotNetVersion>'
$BenchmarkDotNetVersion = $BenchmarkDotNetVersionCapture.Matches.Groups[1].Value
Copy link
Contributor

Choose a reason for hiding this comment

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

Check here also that the match was successful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a check for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please take a quick look at this match check an let me know if you have any thoughts on better ways to do the check. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion here. $BenchmarkDotNetVersionCapture.Matches[0].Success or ...Matches.Groups[1].Success might be other option.

…heck for the BDN version capture to make sure we find it, exiting with a 1 if not.
…tion-operator operand invalid as non-greedy matches appear to not be supported by all sed.
This reverts commit 23ed8d3.
@LoopedBard3
Copy link
Member Author

- pwsh: |
$BenchmarkDotNetVersionCapture = Get-Content .\performance\eng\Versions.props | Select-String -Pattern '<BenchmarkDotNetVersion>(.*)</BenchmarkDotNetVersion>'
$BenchmarkDotNetVersion = $BenchmarkDotNetVersionCapture.Matches.Groups[1].Value
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion here. $BenchmarkDotNetVersionCapture.Matches[0].Success or ...Matches.Groups[1].Success might be other option.

@LoopedBard3 LoopedBard3 merged commit 2ec8a68 into dotnet:main Jul 20, 2023
@LoopedBard3 LoopedBard3 deleted the PerfUpdateMobileBDNFlowPt2 branch July 20, 2023 16:12
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants