-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Infer taskhost needed Fixes #6357 #7308
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
Replaces LoadedType for cases when we don't actually have a LoadedType. Loading the type requires loading (and locking) the assembly.
When they request a task host.
|
Note that this is based on #7132. |
I would like to pursue the automatic fix because it will smooth the transition, but we could have an intermediate state with a better error. |
|
There's some concern over the increase in size for the new assemblies - how much of an increase are we talking per new supported combination? If it's small on the order of 5MB per, then that seems like a win. Otherwise we might need to look at the impacts to SDK size overall and prioritize combinations. @rainersigwald do you have any insight here? |
|
@baronfel for this PR there is no size impact; it applies to .NET Framework MSBuild in VS/Build Tools scenarios which all always have 32- and 64-bit Size impact is in #711 and would be on the order of 15MB/runtime environment (based on the recursive size of |
|
I found the same folder and compressed it with Window's built in compression utilities to simulate what the situation would be from the SDK download perspective - it zips to 5.4 MB, which for 2 additional sets of assemblies (x64/x86) is 10.8 MB, and the current SDK for Windows is 188MB shipped, for a total increase of 5.75% increase. That seems acceptable, but I wonder - in the future could MSBuild for the target SDK platform be included in the SDK download, and the various cross-compat assemblies be shipped as an optional workloads? |
src/Shared/TypeLoader.cs
Outdated
|
|
||
| private bool Required64Bit(ProcessorArchitecture arch) | ||
| { | ||
| return arch == ProcessorArchitecture.IA64 || arch == ProcessorArchitecture.Amd64; |
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 is probably missing ARM64?
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.
For now yes, but it's still a draft 🙂
Fixes #6357
Context
If the user makes a 32-bit task and doesn't specify that in the using task and also doesn't explicitly request a task host, they get a confusing message about not being able to load some file or assembly when building with 64-bit MSBuild. Same is true for the reverse situation.
Changes Made
Currently, it seems to correct identify MSBuild's bitness and that of a task and scrapes the information appropriately. It makes a task host that also infers that it should not load the assembly fully, which causes an issue.
Testing
Notes
Before I investigate the issue I mentioned in Changes Made, I thought I should check the scope of this issue. Thoughts on just sending a better error message when I see that (smaller, easier change) versus pushing forward on automatically "fixing" it for the user?