Skip to content

[xaprepare] Fix dotnet-install script size check #8311

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

Merged
merged 1 commit into from
Aug 30, 2023
Merged

[xaprepare] Fix dotnet-install script size check #8311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions build-tools/xaprepare/xaprepare/Application/Utilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,10 +488,7 @@ public static HttpClient CreateHttpClient ()
try {
using (HttpClient httpClient = CreateHttpClient ()) {
httpClient.Timeout = WebRequestTimeout;
var req = new HttpRequestMessage (HttpMethod.Head, url);
req.Headers.ConnectionClose = true;

HttpResponseMessage resp = await httpClient.SendAsync (req, HttpCompletionOption.ResponseHeadersRead).ConfigureAwait (true);
HttpResponseMessage resp = await httpClient.GetAsync (url, HttpCompletionOption.ResponseHeadersRead).ConfigureAwait (true);
if (!resp.IsSuccessStatusCode || !resp.Content.Headers.ContentLength.HasValue)
return (false, 0, resp.StatusCode);

Expand Down
14 changes: 8 additions & 6 deletions build-tools/xaprepare/xaprepare/Steps/Step_InstallDotNetPreview.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ protected override async Task<bool> Execute (Context context)
dotnetPath = dotnetPath.TrimEnd (new char [] { Path.DirectorySeparatorChar });

if (!await InstallDotNetAsync (context, dotnetPath, BuildToolVersion)) {
Log.ErrorLine ($"Installation of dotnet SDK {BuildToolVersion} failed.");
Log.ErrorLine ($"Installation of dotnet SDK '{BuildToolVersion}' failed.");
return false;
}

Expand All @@ -47,7 +47,7 @@ protected override async Task<bool> Execute (Context context)
ProcessRunner.QuoteArgument ($"-bl:{logPath}"),
};
if (!Utilities.RunCommand (Configurables.Paths.DotNetPreviewTool, restoreArgs)) {
Log.ErrorLine ($"dotnet restore {packageDownloadProj} failed.");
Log.ErrorLine ($"Failed to restore runtime packs using '{packageDownloadProj}'.");
return false;
}

Expand All @@ -74,18 +74,19 @@ async Task<bool> DownloadDotNetInstallScript (Context context, string dotnetScri
string tempDotnetScriptPath = dotnetScriptPath + "-tmp";
Utilities.DeleteFile (tempDotnetScriptPath);

Log.StatusLine ("Downloading dotnet-install...");
Log.StatusLine ("Downloading dotnet-install script...");

(bool success, ulong size, HttpStatusCode status) = await Utilities.GetDownloadSizeWithStatus (dotnetScriptUrl);
if (!success) {
string message;
if (status == HttpStatusCode.NotFound) {
message = "dotnet-install URL not found";
message = $"dotnet-install URL '{dotnetScriptUrl}' not found.";
} else {
message = $"Failed to obtain dotnet-install size. HTTP status code: {status} ({(int)status})";
message = $"Failed to obtain dotnet-install script size from URL '{dotnetScriptUrl}'. HTTP status code: {status} ({(int)status})";
}

return ReportAndCheckCached (message, quietOnError: true);
if (ReportAndCheckCached (message, quietOnError: true))
return true;
Comment on lines -88 to +89
Copy link
Member

@jonathanpeppers jonathanpeppers Aug 30, 2023

Choose a reason for hiding this comment

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

I deleted the quietOnError parameter entirely and just made it log every time.

I don't think you'd see message above in the build logs without doing something like:

image

Copy link
Member Author

@pjcollins pjcollins Aug 30, 2023

Choose a reason for hiding this comment

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

It seems like we may want to skip the cache file error logging initially, as long as we actually proceed to the download step which we weren't doing before. It could be confusing to ~always have that error message in our CI runs?

Copy link
Member

@jonathanpeppers jonathanpeppers Aug 30, 2023

Choose a reason for hiding this comment

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

The way it is currently, it looks like we'd never see this log message on error:

Failed to obtain dotnet-install script size from URL '{dotnetScriptUrl}'. HTTP status code:

Copy link
Member Author

@pjcollins pjcollins Aug 30, 2023

Choose a reason for hiding this comment

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

Or we could simplify this to just warn in both places, as an error about failing to download the script will happen later either way...

Copy link
Member Author

@pjcollins pjcollins Aug 30, 2023

Choose a reason for hiding this comment

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

The way it is currently, it looks like we'd never see this log message on error:

Failed to obtain dotnet-install script size from URL '{dotnetScriptUrl}'. HTTP status code:

Ah right, while this shouldn't be fatal, we still wouldn't know that the size check failed. Let me update the warning/error messaging again.

}

DownloadStatus downloadStatus = Utilities.SetupDownloadStatus (context, size, context.InteractiveSession);
Expand Down Expand Up @@ -192,6 +193,7 @@ async Task<bool> InstallDotNetAsync (Context context, string dotnetPath, string
string scriptFileName = Path.GetFileName (dotnetScriptUrl.LocalPath);
string cachedDotnetScriptPath = Path.Combine (cacheDir, scriptFileName);
if (!await DownloadDotNetInstallScript (context, cachedDotnetScriptPath, dotnetScriptUrl)) {
Log.ErrorLine ($"Failed to download dotnet-install script.");
return false;
}

Expand Down