-
Notifications
You must be signed in to change notification settings - Fork 2.7k
First step to create nuget packages for ARM32/Linux #8421
Conversation
cc @gkhanna79 |
@@ -52,9 +52,15 @@ | |||
<ProjectReference Include="alpine\3.4.3\Microsoft.NETCore.ILAsm.pkgproj"> | |||
<Platform>amd64</Platform> | |||
</ProjectReference> | |||
<ProjectReference Include="ubuntu\14.04\Microsoft.NETCore.ILAsm.pkgproj"> |
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.
Please keep these next to the other 16.04 (or 14.04) entries for consistency.
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.
Same for other such changes in this PR.
</PropertyGroup> | ||
<ItemGroup> | ||
<NativeSplittableBinary Include="$(BinDir)libcoreclr.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libcoreclrtraceptprovider.so" /> | ||
<NativeSplittableBinary Condition="'$(PackagePlatform)' != 'arm'" Include="$(BinDir)libcoreclrtraceptprovider.so" /> |
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.
Why do we need this conditional (are we not building it for Arm)?
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.
Yes. This file is not build for ARM.
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.
Why is the traceprovider not built for Arm?
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.
Still waiting on this.
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 found why is it not built for ARM.
FEATURE_EVENT_TRACE
is not set for ARM build. (src/pal/CMakeLists.txt)
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 am building on the Raspberry PI 3 Raspian.
So, when the build is properly enabled (there was one more place where the feature was set for PAL, I was setting it in clrdefinitions.cmake only before), it fails in /usr/include/urcu/uatomic/generic.h
, which is a header that belongs to the lttng. I can see it is a known issue that was reported and fixed in lttng more than a year ago (http://comments.gmane.org/gmane.linux.kernel.tracing.lttng.devel/10012). So maybe there are too old lttng libraries on my Raspbian or something. But I have just installed the liblttng-ust-dev
package avaiable in Raspbian. And the version and header seems to be the same as the one on my AMD64 ubuntu, so I wonder if the problem is in clang 3.7 being more sensitive.
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 just made the two changes and completed a build successfully on Linux Mint - my changes are in https://github.com/gkhanna79/coreclr/tree/ArmLTTNG. If these look good, then @hseok-oh can you please incorporate them as libcoreclrtraceptprovider.so
is getting built?
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.
Successfully build libcoreclrtraceptprovider.so by @gkhanna79's patch with my private rootfs (based on ubuntu-mate 16.04). But it fails with default rootfs (ubuntu 14.04) for ARM cross build, and fails in /usr/include/urcu/uatomic/generic.h
like @janvorli.
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.
#3879 (comment)
#3879 (comment)
IMHO, it's better excluding libcoreclrtraceptprovider.so
for ARM and fix it later.
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.
@hseok-oh Please file an issue for building this for Arm to ensure we dont miss it.
With that said, I think the rest of the PR LGTM to merge.
<NativeSplittableBinary Include="$(BinDir)libdbgshim.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libmscordaccore.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libmscordbi.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libsos.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libsosplugin.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)System.Globalization.Native.so" /> | ||
<ArchitectureSpecificNativeFile Include="$(BinDir)sosdocsunix.txt" /> | ||
<ArchitectureSpecificNativeFile Include="$(BinDir)mscorlib.ni.dll" /> | ||
<ArchitectureSpecificNativeFile Include="$(BinDir)System.Private.CoreLib.ni.dll" /> | ||
<ArchitectureSpecificNativeFile Condition="'$(PackagePlatform)'!='arm'" Include="$(BinDir)mscorlib.ni.dll" /> |
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 presume you are doing this because cross-compilation done on a non-Arm machine (e.g. x64) does not build a x86 (or x64) hosted crossgen that can generate Arm code,right? If it, i think it will really help to bring up the x86 Linux build as that will help build a x86-hosted crossgen that generate arm32 BI images and thus, alleviating the need for these.
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.
Exactly. We also think implementing x86-host/arm-target crossgen as future work.
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.
Thank you @hseok-oh. Can you please file an issue for "Building cross-targeting components" for Linux Arm32 bringup?
@@ -69,6 +69,19 @@ initDistroRid() | |||
fi | |||
} | |||
|
|||
initTargetDistroRid() |
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.
Instead of this function, why can we not fix https://github.com/dotnet/coreclr/pull/8421/files#diff-0b83f9dedf40d7356e5ca147a077acb4R67 to use $__BuildArch
instead of $__HostArch
?
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.
$__DistroRid
is used in https://github.com/hseok-oh/coreclr/blob/5145193d4970eaf9a008014a7b02307611bd96ef/build.sh#L237 before nuget packaging, and this function need host environment information to use msbuild on .net core.
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.
Given the above, I would suggest that you rename the current $__DistroRid to $__HostDistroRid and your proposed $__CrossDistroRid as $__DistroRid. This will maintain the existing uses and convey the correct meaning via $__DistroRid and also eliminate your changes in dir.props.
@@ -128,9 +128,11 @@ | |||
<TargetsWindows Condition="'$(BuildOS)' == 'Windows_NT'">true</TargetsWindows> | |||
|
|||
<TargetsUnix Condition="'$(TargetsFreeBSD)' == 'true' or '$(TargetsLinux)' == 'true' or '$(TargetsNetBSD)' == 'true' or '$(TargetsOSX)' == 'true'">true</TargetsUnix> | |||
<TargetsLinuxCross Condition="'$(TargetsLinux)' == 'true' and '$(CROSSCOMPILE)' != ''">true</TargetsLinuxCross> |
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 dont think we need cross-compilation identification like this. We perform cross-compilation on Windows as well without requiring this.
See my comment above - if we can use a single function that defines the DistroRid correctly, then your change here is not required.
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.
If the environment variable '$__DistroRid' is not used in other place, we can overwrite the value of $__DistroRidCross
to $__DistroRid
in build.sh. Then we don't need TargetLinuxCross
.
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.
See my comment above on how to address it.
Also, please add the RID definition athttps://github.com/dotnet/corefx/blob/master/pkg/Microsoft.NETCore.Platforms/runtime.json before using it here for package generation. |
@hseok-oh, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
@hseok-oh, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
@gkhanna79 PTAL |
</PropertyGroup> | ||
<ItemGroup> | ||
<NativeSplittableBinary Include="$(BinDir)libcoreclr.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libcoreclrtraceptprovider.so" /> | ||
<NativeSplittableBinary Condition="'($PackagePlatform)' == 'arm'" Include="$(BinDir)libcoreclrtraceptprovider.so" /> |
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 should be "!=" and not "==".
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.
And likewise for other such comparisons.
<NativeSplittableBinary Include="$(BinDir)libdbgshim.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libmscordaccore.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libmscordbi.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libsos.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)libsosplugin.so" /> | ||
<NativeSplittableBinary Include="$(BinDir)System.Globalization.Native.so" /> | ||
<ArchitectureSpecificNativeFile Include="$(BinDir)sosdocsunix.txt" /> | ||
<ArchitectureSpecificNativeFile Include="$(BinDir)mscorlib.ni.dll" /> | ||
<ArchitectureSpecificNativeFile Include="$(BinDir)System.Private.CoreLib.ni.dll" /> | ||
<ArchitectureSpecificNativeFile Condition="'($PackagePlatform)' == 'arm'" Include="$(BinDir)mscorlib.ni.dll" /> |
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.
Same as above.
<ArchitectureSpecificNativeFile Include="$(BinDir)mscorlib.ni.dll" /> | ||
<ArchitectureSpecificNativeFile Include="$(BinDir)System.Private.CoreLib.ni.dll" /> | ||
<ArchitectureSpecificNativeFile Condition="'($PackagePlatform)' == 'arm'" Include="$(BinDir)mscorlib.ni.dll" /> | ||
<ArchitectureSpecificNativeFile Condition="'($PackagePlatform)' == 'arm'" Include="$(BinDir)System.Private.CoreLib.ni.dll" /> |
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.
Same as above.
@hseok-oh Are we good to merge this change? |
@gkhanna79 merge this PR, please. Thanks. |
…_nupkg First step to create nuget packages for ARM32/Linux Commit migrated from dotnet/coreclr@e7d1919
Modify src/.nuget to create nuget package files for ARM32/Linux (cross build).
It excludes
*.ni.dll
generated by crossgen.generate for