-
Notifications
You must be signed in to change notification settings - Fork 63
Add a basic project and E2E test for the bootstrapper #386
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
Conversation
this is all very hacky / wip atm
I'm not sure why we have logic to check if it's not a PR run or an internal run if the pipeline is for PRs only. Maybe this won't work, and I don't understand the system that well, but it seems like it may have been copy pasted and a result of de-merging two pipelines into separate ones. There is also a large amount of yaml which is duplicated and can be simplified/shared.
This reverts commit 5cf6eec.
I will be away but you are good to merge it if it is approved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, its an empty file for when we add strings to this project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be here?
No, you shouldn't commit a .Designer file. It is generated on-build. You only need the .resx file.
<TargetFramework>net8.0</TargetFramework> | ||
<RunAOTCompilation>true</RunAOTCompilation> | ||
<PublishTrimmed>true</PublishTrimmed> | ||
<IsPackable>false</IsPackable> | ||
<PublishSingleFile>true</PublishSingleFile> | ||
<PublishAot>true</PublishAot> | ||
<WasmStripILAfterAOT>true</WasmStripILAfterAOT> | ||
<RollForward>LatestMajor</RollForward> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should review which of these properties should be set. I don't think RollForward
applies to a self-contained app, and I don't think WasmStripILAfterAOT
applies to non-WASM projects. Some of the other AOT / publish properties may be redundant, I think the main one you need to specify is PublishAOT
.
private readonly static string artifactsDirectory = Path.GetFullPath(Path.Combine(Directory.GetCurrentDirectory(), "..", "..", "..", "..", "..", "artifacts")); | ||
private readonly static string executablePath = Path.Combine(artifactsDirectory, "bin", "dotnet-bootstrapper", TestUtilities.GetConfiguration(), TestUtilities.GetTargetFramework(), TestUtilities.GetRuntimeIdentifier(), | ||
RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "dotnet-bootstrapper.exe" : "dotnet-bootstrapper"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to be a bit more resilient to changes in the folder layout. Probably a good strategy is to find the repo root by looking for a .git
folder and then use a relative path from there.
See TestContext.GetRepoRoot
in dotnet/sdk for a sample of how to do that. There's also a bunch more code in there for finding different folders, we probably don't need most of that complexity for this repo.
Resolves #347
MSBuildBinLogQuery
can be removed in a different PR.