Fix ilasm managed resource lookup on Linux. #28104
Conversation
|
This change has a corresponding PR on the mainline repo: dotnet/runtime#42735 |
MarkpageBxl
force-pushed
the
ilasm-managed-resources-path-fix-3.1
branch
2 times, most recently
from
e189533 to
97e6c8c
Compare
ILAsm did not properly parse paths on *nix systems, which notably broke the inclusion of managed resources in an assembly. This commit enables ILAsm to support both types of directory separators by relying on the DIRECTORY_SEPARATOR_CHAR_A macro where relevant.
We rely on the FEATURE_PAL macro to determine whether we are targeting a *nix platform.
MarkpageBxl
force-pushed
the
ilasm-managed-resources-path-fix-3.1
branch
from
97e6c8c to
5beca66
Compare
| wcscpy_s(wzFileName,2048,pwzInputFiles[j]); | ||
| pwz = wcsrchr(wzFileName,'\\'); | ||
| pwz = wcsrchr(wzFileName,DIRECTORY_SEPARATOR_CHAR_A); | ||
| #ifndef FEATURE_PAL |
There was a problem hiding this comment.
#ifdef TARGET_UNIX is a preferred way now, can you please change it here and at the two other places?
There was a problem hiding this comment.
Hi Jan, it looks like the TARGET_UNIX macro doesn't exist on the 3.1 branch, only on the mainline. FEATURE_PAL seemed to be the equivalent test on the older 3.1 codebase.
There was a problem hiding this comment.
Ah, I am sorry, I have not noticed this is the 3.1 port and not the original 5.0 change.
|
@jeffschwMSFT do you think this change would meet the bar to get accepted into 3.1? |
|
It seems that a workaround could be to copy the managed resources into the working directory of the ilasm. @mlindstr please correct me if I am wrong. |
|
@janvorli That is correct, or otherwise making sure that ilasm is called with the expected working directory. @jeffschwMSFT The workaround outlined by Jan would do the trick, but it would force us to hack a different behavior for the Windows and Linux platforms in our products. Our legacy language compilers target .NET Core, and we intend to only track LTS releases for maintenance purposes. We believe this fix is minor and low risk, and it would greatly help us to have it land to enable our compiler stack to work properly on Linux without having to hack a solution until 6.0 LTS is released. |
|
@jeffschwMSFT is this critical for November servicing? |
jeffschwMSFT
added
area-CodeGen
Servicing-consider
|
@wtgodbe we were closing on a few details internally. We plan on considering this issue in an upcoming servicing release |
leecow
added
Servicing-approved
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
* Fix ilasm path parsing on *nix. ILAsm did not properly parse paths on *nix systems, which notably broke the inclusion of managed resources in an assembly. This commit enables ILAsm to support both types of directory separators by relying on the DIRECTORY_SEPARATOR_CHAR_A macro where relevant. * Only consider colon as a special path character on Windows. We rely on the FEATURE_PAL macro to determine whether we are targeting a *nix platform.
ILAsm did not properly find managed resources if they were not in the
working directory of ILAsm itself. While there was a provision for
Windows-based systems using backslashes as directory separators, there
was no such provision for *nix-based systems using forward slashes.
This commit enables ILAsm to lookup both types of directory separators.