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

Fix mono aot build #3122

Merged
merged 2 commits into from
Jul 12, 2023
Merged

Fix mono aot build #3122

merged 2 commits into from
Jul 12, 2023

Conversation

radical
Copy link
Member

@radical radical commented Jul 12, 2023 •
edited
Loading

Issue: dotnet/BenchmarkDotNet#2311

  • Latest BDN copies the PackageReferences from
    MicroBenchmarks.csproj to the generated project.
    • but it does not copy the version property, thus breaking the build.
    • Move the version property to a Common.props, so it can be used by
      the main MicroBenchmarks.csproj, and the autogenerated project.

@radical
Fix mono aot build
2421075 
Issue: dotnet/BenchmarkDotNet#2311

- Use the extension props file to set `SelfContained=true` for the
  autogenerated project. This allows the project to have all the
  dependencies in the output folder, as required for AOT.

- Latest BDN copies the `PackageReferences` from
  `MicroBenchmarks.csproj` to the generated project.
  - but it does not copy the version property, thus breaking the build.
  - Move the version property to a Common.props, so it can be used by
    the main MicroBenchmarks.csproj, and the autogenerated project.
@radical
Copy link
Member Author

radical commented Jul 12, 2023

This depends on dotnet/BenchmarkDotNet#2367 .

@radical radical mentioned this pull request Jul 12, 2023
Closed
kotlarmilos
kotlarmilos previously approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

A minor suggestion, otherwise LGTM!

kotlarmilos
kotlarmilos approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

LGTM!

LoopedBard3
LoopedBard3 approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@LoopedBard3 LoopedBard3 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, and the failures are the same as on main so good to merge. @radical do you have permission to merge? If not, I can go ahead and merge this.

@radical
Copy link
Member Author

radical commented Jul 12, 2023

We do need the bdn update though.

@radical
Copy link
Member Author

radical commented Jul 12, 2023

As for the failures, I think it might need a fix like dotnet/runtime@2e873dd . Essentially set DOTNET_ROOT. I'm testing that hypothesis in dotnet/runtime#88755 .

@radical
Copy link
Member Author

radical commented Jul 12, 2023

@LoopedBard3 I only see a nightly nuget pushed - https://github.com/dotnet/BenchmarkDotNet/actions/runs/5532878163/jobs/10095576828

@LoopedBard3
Copy link
Member

LoopedBard3 commented Jul 12, 2023 •
edited
Loading

@LoopedBard3 I only see a nightly nuget pushed - https://github.com/dotnet/BenchmarkDotNet/actions/runs/5532878163/jobs/10095576828

Do you know the earliest version that we need? @cincuranet is working on getting a BDN update PR in: #3120. We will make sure this merges along with the BDN update today.

@radical
Copy link
Member Author

radical commented Jul 12, 2023

I think updating to the latest bdn will also run into dotnet/BenchmarkDotNet#2346 .

@radical
Copy link
Member Author

radical commented Jul 12, 2023 •
edited
Loading

The failures on windows are same as dotnet/runtime#88779 . I'm fixing it for wasm with dotnet/runtime@35477aa . I think a similar fix is needed here too - to set DOTNET_ROOT. And the reason for that is dotnet/roslyn#68918 changed
to build the dotnet path using DOTNET_ROOT instead of depending PATH.

@cincuranet cincuranet mentioned this pull request Jul 12, 2023
Closed
@LoopedBard3
Copy link
Member

LoopedBard3 commented Jul 12, 2023 •
edited
Loading

@radical It does seem that the BDN update is causing us to hit dotnet/BenchmarkDotNet#2346. As we wait for that fix, do we want to get this PR merged so we are closer to being in a good state once the fix is ready?

For the DOTNET_ROOT, our MicroBenchmark builds already set the DOTNET_ROOT explicitly on the running machine. We have an issue to fix in the affected areas, #3136.

@radical
Copy link
Member Author

radical commented Jul 12, 2023

@radical It does seem that the BDN update is causing us to hit dotnet/BenchmarkDotNet#2346. As we wait for that fix, do we want to get this PR merged so we are closer to being in a good state once the fix is ready?

Do you mean merge this(#3122), and then continue to wait for BDN update via #3120?
Yeah, we can merge this. The "fix" won't take effect yet, but no need to keep this one open till the bdn update.

@LoopedBard3
Copy link
Member

@radical It does seem that the BDN update is causing us to hit dotnet/BenchmarkDotNet#2346. As we wait for that fix, do we want to get this PR merged so we are closer to being in a good state once the fix is ready?

Do you mean merge this(#3122), and then continue to wait for BDN update via #3120? Yeah, we can merge this. The "fix" won't take effect yet, but no need to keep this one open till the bdn update.

Pretty much. Merge this #3122 with #3120 coming shortly while we wait for the fix to dotnet/BenchmarkDotNet#2346.

Let me know if you don't have merge permissions and I will merge this in.

@radical radical enabled auto-merge (squash) July 12, 2023 21:52
@radical
Copy link
Member Author

radical commented Jul 12, 2023

ah, looks like I can't merge on Red.Required statuses must pass before merging

@radical radical disabled auto-merge July 12, 2023 21:54
@LoopedBard3 LoopedBard3 merged commit b8ef22b into dotnet:main Jul 12, 2023
@radical radical deleted the fix-mono-aot branch July 12, 2023 22:03
@adamsitnik
Copy link
Member

Latest BDN copies the PackageReferences from
MicroBenchmarks.csproj to the generated project.

It was a bad idea and just got reverted: dotnet/BenchmarkDotNet#2365

IMO once #3120 gets merged we should revert this PR too, as it won't be needed anymore and adds some non-trivial complexity to our MSBuild files that are already quite complex (cc @cincuranet )

@radical kudos for finding a quick fix!

@cincuranet
Copy link
Contributor

I'll take care of the revert.

@cincuranet cincuranet mentioned this pull request Jul 18, 2023
Merged
cincuranet added a commit that referenced this pull request Jul 18, 2023
@cincuranet
Reverts #3122 (b8ef22b). (#3143)
157f1db 
This is no longer needed #3122 (comment).
@kotlarmilos kotlarmilos mentioned this pull request Jul 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kotlarmilos kotlarmilos kotlarmilos approved these changes

@LoopedBard3 LoopedBard3 LoopedBard3 approved these changes

@adamsitnik adamsitnik Awaiting requested review from adamsitnik

@cincuranet cincuranet Awaiting requested review from cincuranet

@lewing lewing Awaiting requested review from lewing

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

Assignees
No one assigned
Labels
arch-wasm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@radical @LoopedBard3 @adamsitnik @cincuranet @kotlarmilos