-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Use workload sets as per user request #37681
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
Changes from all commits
cdba4ac
3a54940
65ea81a
02253d6
63647b2
347d0c9
39f0ec7
52af35d
2625649
198e397
2a4a9f9
b28ca38
0aca30e
4d1b0fc
65e6c73
197b7f9
324d5e0
d44a33d
60a387f
402ff34
5796ee5
4b8ab17
2e0d6e5
0e4c5f1
3a60e0c
d47e404
ea939c7
21a33a5
925239d
1c69272
cb06526
f21016d
4155948
6324121
8c8ce54
5bbcb0d
5c09c2c
b9f79ac
e38c25a
d0e0bcf
9a34b4b
11c0d44
c7ec231
94255c9
5944045
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |
| using System.Text.Json; | ||
| using System.Text.Json.Serialization; | ||
|
|
||
| #pragma warning disable CS8632 | ||
|
|
||
| namespace Microsoft.DotNet.Workloads.Workload | ||
| { | ||
| internal class InstallStateContents | ||
|
|
@@ -12,17 +14,21 @@ internal class InstallStateContents | |
| public bool? UseWorkloadSets { get; set; } | ||
|
|
||
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
| public Dictionary<string, string> Manifests { get; set; } | ||
| public Dictionary<string, string>? Manifests { get; set; } | ||
|
|
||
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
| public string? WorkloadVersion { get; set; } | ||
|
|
||
| private static readonly JsonSerializerOptions s_options = new() | ||
| { | ||
| PropertyNamingPolicy = JsonNamingPolicy.CamelCase, | ||
| WriteIndented = true, | ||
| AllowTrailingCommas = true, | ||
| }; | ||
|
|
||
| public static InstallStateContents FromString(string contents) | ||
| { | ||
| return JsonSerializer.Deserialize<InstallStateContents>(contents, s_options); | ||
| return JsonSerializer.Deserialize<InstallStateContents>(contents, s_options) ?? new InstallStateContents(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for this change? Will the JsonSerializer ever return null from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would if the string is null or something like that. That should never happen in our case, but it was making nullable unhappy, and this was an easy way to make that error go away. |
||
| } | ||
|
|
||
| public static InstallStateContents FromPath(string path) | ||
|
|
@@ -35,4 +41,6 @@ public override string ToString() | |
| return JsonSerializer.Serialize<InstallStateContents>(this, s_options); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #pragma warning restore CS8632 | ||
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.
Suggestion: I think this would feel safer if it also took a "root" path where it wouldn't delete above that. There are a few places in FileBasedInstaller (mainly in the garbage collection I think) where this could simplify the code.
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.
Can you clarify what error case you're trying to avoid? The two possible cases I can think of are:
The second could happen, though wrapping it in a try/catch would presumably be just as good. (I think this is very unlikely unless the directory with the file to delete is already protected.) The first shouldn't happen.
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 under the dotnet/metadata directory there might be directories that we don't want to delete even if they are empty. It's not a big deal though.