-
Notifications
You must be signed in to change notification settings - Fork 136
[release/5.0] Print and store versions at end of every build #2157
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
[release/5.0] Print and store versions at end of every build #2157
Conversation
33489da
to
da5096b
Compare
ce573d4
to
cacf32a
Compare
CI is finally green 🥳 |
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.
One question about dotnet-sdk.tar.gz vs dotnet-sdk-internal.tar.gz. Other than that this looks great, thanks a lot!
This makes it easier to visually confirm what was built: Build Info: SDK Version: 5.0.202 ASP.NET Core Version: 5.0.5 Runtime Version: 5.0.5 And also stores these values in named version files for later use. Fixes: dotnet#600
cacf32a
to
45cd8d5
Compare
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.
Thanks for working on this @omajid. I left a few comments.
AspNetCoreVersion = ExecuteDotNetCommand(SdkRootDirectory, | ||
pathToDotNet, | ||
new List<string> { "--list-runtimes" }) | ||
.Where(line => line.Contains("Microsoft.AspNetCore.App")) |
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.
This could be simplified by utilizing a different First overload and the ElementAt linq method.
.First(line => line.Contains("Microsoft.AspNetCore.App")).Split(" ").ElementAt(1)
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.
Thanks for that suggestion. When you say "simplified", is that in terms of performance? If so, do you have any suggestions on resources (book, etc) I should take a look at to improve my understanding of performance impact of LINQ queries? Thanks in advance!
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 wouldn't expect any performance difference with the suggestion I made. I don't have any suggestions on resources. I have a personal perf harness I use to measure differences for scenarios like this.
.First() | ||
.Split(" ") | ||
.First(); | ||
AspNetCoreVersion = ExecuteDotNetCommand(SdkRootDirectory, |
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.
Consider storing the result of --list-runtimes
instead of invoking it multiple times.
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.
Thanks, fixed!
tools-local/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/ParseDotNetVersions.cs
Outdated
Show resolved
Hide resolved
|
||
<MakeDir Directories="$(ExtractedPath)" /> | ||
|
||
<Exec Command="tar xf @(FinalSdkTarball) -C $(ExtractedPath)" /> |
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.
In 6.0 we've been trying to reduce the disk footprint of the tarball builds. It feels like the smoke-tests should reuse this location in CI. An alternative option is to grab the .versions files out of the tarball. Could these be utilized as is with a possible rename to satisfy the requirements? If not does it make sense to just grab the versions number out of them?
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.
That's a great idea! It should be possible to re-use the directory for smoke-tests. I am testing it now out now.
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.
Thanks again for the idea. I have implemented it. Only caveat that I can think of is that now the tests will always assume that the already-extracted-sdk is pristine; if any test modifies it, the modification will persist to future test runs.
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.
Agreed, but on the other hand, I don't think this is something our tests should be doing either.
Co-authored-by: Michael Simons <[email protected]>
This was suggested by Michael Simons in code review. - Simplify the LINQ expression. - Invoke `dotnet --list-runtimes` only once and keep the results.
c0783e6
to
5d64a7b
Compare
Modify smoke-test.sh to use the same extracted cli directory as the one we use to parse the SDK/Runtime/ASP version. The smoke-test.sh script offers to interactively delete $SCRIPT_ROOT/smoke-testing if it exists. That interactive prompt breaks CI. So we can't use the smoke-testing dir. Instead, point smoke-test.sh to a different directory (outside of $SCRIPT_ROOT/smoke-testing) so it can reuse that.
5d64a7b
to
aa23c54
Compare
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.
Thanks for the PR Omair!
This makes it easier to visually confirm what was built:
And also stores these values in named version files for later use.
The build output on the console looks like this:
And the files look like this:
Fixes: #600