Adding special target that will get a resource file and pass it to ilasm for S.R.CS.Unsafe so that it has the right version headers#31782
Conversation
| </Exec> | ||
| <Error Condition="'$(_IldasmCommandExitCode)' != '0'" Text="ILDasm failed while running command: "$(_IldasmCommand)"" /> | ||
|
|
||
| <!-- Calling cvtres.exe which should be on the path in order to transform the resource file to an obj --> |
There was a problem hiding this comment.
Where is cvtres going to come from?
There was a problem hiding this comment.
Good question... Why do we even need it? Shouldn't ILDasm be emitting a format that roundtrips through ILAsm?
There was a problem hiding this comment.
We have a requirement right now saying that you have to be running on a developer command prompt in order to build corefx. That means that cvtres will be on the PATH. I know that this means we would add a dependency to VS (when we are trying to remove as much of them as possible) but we will only add this for Windows build and I will log an issue to ilasm so that they can provide another way of passing in version information without having to transform the .res file.
There was a problem hiding this comment.
Why do we even need it? Shouldn't ILDasm be emitting a format that roundtrips through ILAsm?
Talked to @jkotas about this offline, basically, ILDasm doesn't emit the format that ILAsm would be able to understand for now so we can submit a feature request for this.
ericstj
left a comment
ericstj
left a comment
There was a problem hiding this comment.
Did you look into driving
off a header value? Might require adding the hex representation of ascii version string to the version header.| <_IldasmCommand>$(_IldasmCommand) $(ContractOutputPath)/$(MSBuildProjectName).dll</_IldasmCommand> | ||
| <_IldasmCommand>$(_IldasmCommand) /OUT=$(IntermediateOutputPath)Disassembly/$(MSBuildProjectName).il</_IldasmCommand> | ||
|
|
||
| <_CvtResCommand>cvtres.exe</_CvtResCommand> |
There was a problem hiding this comment.
You can consider checking for this in the framework directory ($(SystemRoot)\Microsoft.NET\Framework\v4.0.30319\cvtres.exe) and falling back to assume on the path and warning if it is absent. That way you can support a case where someone ran outside a developer cmd prompt (even though its technically a requirement).
| <Reference Include="System.Runtime" /> | ||
| </ItemGroup> | ||
|
|
||
| <Target Name="AddResourcesFileToIlasm" BeforeTargets="CoreCompile" Condition="'$(OS)'=='Windows_NT'"> |
There was a problem hiding this comment.
Were you able to test what ILDasm did on linux? I'm curious if it does any better or this scenario (resource roundtripping) is completely broken.
There was a problem hiding this comment.
Actually I tried running it on Linux and ildasm runs succesfully, except that it only proudces the .il file, it won't produce the .res as I believe it is expected.
| </ItemGroup> | ||
|
|
||
| <Target Name="AddResourcesFileToIlasm" BeforeTargets="CoreCompile" Condition="'$(OS)'=='Windows_NT'"> | ||
| <MakeDir Directories="$(IntermediateOutputPath)Disassembly" /> |
There was a problem hiding this comment.
rather than a directory i'd just give it a semi-unique name (to preserve path chars). Something like $(MSBuildProjectName).ref.il.
| <Reference Include="System.Runtime" /> | ||
| </ItemGroup> | ||
|
|
||
| <Target Name="AddResourcesFileToIlasm" BeforeTargets="CoreCompile" Condition="'$(OS)'=='Windows_NT'"> |
There was a problem hiding this comment.
Add inputs / outputs so that you don't break incremental.
|
|
||
| <!-- Getting the res file by disassemblying the contract assembly --> | ||
| <Exec Command="$(_IldasmCommand)" ConsoleToMSBuild="true" StandardOutputImportance="Low"> | ||
| <Output TaskParameter="ExitCode" PropertyName="_IldasmCommandExitCode" /> |
There was a problem hiding this comment.
Add file-writes so that clean works correctly.
| BeforeTargets="CoreCompile" | ||
| Condition="'$(OS)'=='Windows_NT'" | ||
| Inputs="$(ContractOutputPath)/$(MSBuildProjectName).dll" | ||
| Outputs="$(IntermediateOutputPath)$(MSBuildProjectName).ref.obj"> |
There was a problem hiding this comment.
nit, I'd call this res.obj or ref.res.obj to indicate its a resource object. Add a comment about why its conditioned for only windows.
…asm for S.R.CS.Unsafe so that it has the right version headers
|
Merging now since OSX leg seems to have gone stale. |
…asm for S.R.CS.Unsafe so that it has the right version headers (dotnet#31782) * Adding special target that will get a resource file and pass it to ilasm for S.R.CS.Unsafe so that it has the right version headers * PR Feedback * Updating buildtools and fixing obj name
…asm for S.R.CS.Unsafe so that it has the right version headers (#31806) * Adding special target that will get a resource file and pass it to ilasm for S.R.CS.Unsafe so that it has the right version headers (#31782) * Adding special target that will get a resource file and pass it to ilasm for S.R.CS.Unsafe so that it has the right version headers * PR Feedback * Updating buildtools and fixing obj name * Rev-up buildtools version to get required Fixes
…asm for S.R.CS.Unsafe so that it has the right version headers (dotnet/corefx#31782) * Adding special target that will get a resource file and pass it to ilasm for S.R.CS.Unsafe so that it has the right version headers * PR Feedback * Updating buildtools and fixing obj name Commit migrated from dotnet/corefx@e6506db
4 participants
Fixes: https://github.com/dotnet/corefx/issues/24012
The following PR needs to get merged and ingested in order for this to work: dotnet/buildtools#2123
Adding a target that will get a resource file with the file information headers, then it will transform the .res file into an obj (which is the format that ilasm takes) and then passes it down to the ilasm call so that System.Runtime.CompilerServices.Unsafe has the right version headers in them.
cc: @jkotas