-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Adding basic support for an ilproj sdk #19066
Conversation
<_OSArchitecture>$([System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture)</_OSArchitecture> | ||
|
||
<MicrosoftNetCoreIlasmPackageRuntimeId Condition="'$(MicrosoftNetCoreIlasmPackageRuntimeId)' == ''">$(_OSPlatform)-$(_OSArchitecture.ToLower())</MicrosoftNetCoreIlasmPackageRuntimeId> | ||
<MicrosoftNetCoreIlasmPackageVersion Condition="'$(MicrosoftNetCoreIlasmPackageVersion)' == ''">2.0.8</MicrosoftNetCoreIlasmPackageVersion> |
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.
Long term, we should either come up with a mechanism to update this to have the current build version, or we should package ILAsm in this SDK.
I am leaning towards the latter, but that requires https://github.com/dotnet/coreclr/issues/15059 to be resolved first.
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.
In the mean-time you should just use the version in the repo
Line 15 in fc04b40
<PackageVersion Condition="'$(PackageVersion)' == ''">3.0.0</PackageVersion> |
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.
Fixed.
</ItemGroup> | ||
|
||
<PropertyGroup> | ||
<_IlasmDir>$(ToolsDir)\ilasm</_IlasmDir> |
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.
There is some cleanup to be done here, but doing a "clean import" followed by separate "refactoring/cleanup" was unanimously favored
src/.nuget/packages.builds
Outdated
@@ -23,6 +23,7 @@ | |||
<ItemGroup> | |||
<Project Include="Microsoft.NETCore.ILAsm\Microsoft.NETCore.ILAsm.builds" /> | |||
<Project Include="Microsoft.NETCore.ILDAsm\Microsoft.NETCore.ILDAsm.builds" /> | |||
<Project Include="Microsoft.NET.Sdk.IL\Microsoft.NET.Sdk.IL.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.
Is there anything special required to ensure the nupkg is published?
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.
No but you should scope this to only happen on one OS leg similar logic to when we build the identity package
coreclr/src/.nuget/dir.traversal.targets
Line 15 in fc04b40
<BuildIdentityPackage Condition="'$(BuildingAnOfficialBuildLeg)' == 'true' AND ('$(OS)' != 'Windows_NT' OR '$(BuildArch)' != 'x64')">false</BuildIdentityPackage> |
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.
Added Condition="'$(BuildIdentityPackage)' == 'true'"
Are these files supposed to be under |
<MicrosoftNetCoreIlasmPackageName>runtime.$(MicrosoftNetCoreIlasmPackageRuntimeId).microsoft.netcore.ilasm</MicrosoftNetCoreIlasmPackageName> | ||
<MicrosoftNetCoreRuntimeCoreClrPackageName>runtime.$(MicrosoftNetCoreIlasmPackageRuntimeId).microsoft.netcore.runtime.coreclr</MicrosoftNetCoreRuntimeCoreClrPackageName> | ||
|
||
<ToolsDir Condition="'$(ToolsDir)' == ''">$([System.IO.Path]::Combine($([System.IO.Path]::GetTempPath()), $([System.IO.Path]::GetRandomFileName())))</ToolsDir> |
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 know other places use this ToolsDir property so I'd suggest you define some private property to use here. It is fine to use ToolsDir if it is set but I think setting it might cause some conflicts.
No they should be under a subdir. |
FYI @RussKeldorph |
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
|
||
<PropertyGroup> | ||
<MSBuildAllProjects>$(MSBuildThisFileFullPath);$(MSBuildAllProjects)</MSBuildAllProjects> |
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 file doesn't seem necessary. Can we remove it until we have stuff to put in it?
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 comment didn't appear to be addressed.
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.
Ah, missed this. Will remove for the time being.
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.
Fixed.
<_OSPlatform Condition="$([MSBuild]::IsOSPlatform('osx'))">osx</_OSPlatform> | ||
<_OSArchitecture>$([System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture)</_OSArchitecture> | ||
|
||
<MicrosoftNetCoreIlasmPackageRuntimeId Condition="'$(MicrosoftNetCoreIlasmPackageRuntimeId)' == ''">$(_OSPlatform)-$(_OSArchitecture.ToLower())</MicrosoftNetCoreIlasmPackageRuntimeId> |
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.
What about linux-musl
? We support that RID as well.
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.
Do we have an existing API to get linux-musl
as the OSPlatform?
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.
Looking at the current runtime.json for ilasm
, the same question applies to rhel.6
and tizen.4.0.0
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 believe IsOSPlatform('linux') == true
is still correct here. We are still on linux
, we just happen to be using a different libc
. (And thus, requiring different native assets)
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.
It appears in corefx, the RID is normally passed in from the outside of the build:
https://github.com/dotnet/corefx/search?q=linux-musl&unscoped_q=linux-musl
The only place it is actually detected is:
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.
Unfortunately I suspect it is not going to be sufficient. I suspect as soon as you try to move to using this package in corefx we won't even get past CI because the linux-musl leg will fail.
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.
we won't even get past CI because the linux-musl leg will fail.
For this, (on alpine, tizen, rhel6) I was indicating that we would continue passing it in from outside the build, as we are today.
An alternative would be to have the .targets file duplicate this logic: https://github.com/dotnet/corefx/blob/a53adfc55bcb028d36a8647e45c66f7be5374f92/init-tools.sh#L94
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.
Ah I missed that connection. In that case I think we can live with it for now so long as we can actually consume it I think we should be good.
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've tested locally and everything is working correctly when explicitly passing in the RID.
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.
Some tools use ldd --version
to figure out the libc flavor. Based on that, there could be an MSBuild target to detect glibc vs. musl-libc, for example:
Inspired by https://github.com/justjake/encabulator/blob/34b587b/unison/linux_env.go
<?xml version="1.0" encoding="utf-8"?>
<Project>
<Target Name="DetectLibcFlavor" Condition="!$([MSBuild]::IsOSPlatform('windows'))">
<Exec Command="ldd --version" ConsoleToMSBuild="true" ContinueOnError="true" StandardOutputImportance="low" StandardErrorImportance="low">
<Output TaskParameter="ConsoleOutput" PropertyName="LddOutput" />
</Exec>
<Message Importance="high" Text="using glibc"
Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch($(LddOutput), `glibc|GNU libc|EGLIBC`)) " />
<Message Importance="high" Text="using musl-libc"
Condition=" $([System.Text.RegularExpressions.Regex]::IsMatch(`$(LddOutput)`, `musl`)) " />
</Target>
</Project>
Tested with dotnet msbuild DetectLibcFlavor.proj
.
Sample ldd --version
output on Alpine:
musl libc (x86_64)
Version 1.1.18
Dynamic Program Loader
Usage: ldd [options] [--] pathname
and on Ubuntu:
ldd (Ubuntu EGLIBC 2.15-0ubuntu10.7) 2.15
Copyright (C) 2012 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Written by Roland McGrath and Ulrich Drepper.
(would be super helpful if MSBuild provides information about libc via some intrincis method next to [MSBuild]::IsOSPlatform
and friends, which we have today)
I believe all feedback, given so far, has now been addressed. There is some follow up work for some general cleanup
There is some follow up work after https://github.com/dotnet/coreclr/issues/15059 is resolved to remove the secondary nuget reference and corresponding copy logic. There is some follow up work after https://github.com/dotnet/corefx/issues/31002 is resolved to remove the RID construction logic. Does anyone have anything further needed here, or am I good to merge? |
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'm fine with moving forward with this.
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
|
||
<PropertyGroup> | ||
<MSBuildAllProjects>$(MSBuildThisFileFullPath);$(MSBuildAllProjects)</MSBuildAllProjects> |
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 comment didn't appear to be addressed.
Looks like the build failures are occurring across multiple PRs. |
@chsienki those issues look related to your testing changes are you looking into them? |
I have a PR here #19130 Trying to figure out why the OSX corefx tests are failing still, and if its related |
Rebased now that #19130 has been merged. |
@weshaggard, are the perf lab tests doing something different that prevents them from recognizing MSBuild ItemMetadata exposed as XML attributes (rather than nested tags)? |
I think the perf lab runs are done on VS2015, so presumably an earlier version of MSBuild. For instance I had to add the xmlns= to the project element in the target files, or those runs wouldn't see them as valid MSBuild files. |
OSX has timed-out 3 times now. It has built successfully locally (on my MacBook Pro), so I am merging. |
@tannergooding do you know how you plan to consume this package in coreclr and corefx yet? |
@weshaggard, as soon as a package gets published (will check in the morning) I will add the PR to CoreFX for its consumption (I've already tested most of it locally). |
CoreFX side is here: dotnet/corefx#31512 |
This is a port of dotnet/arcade#317, which we determined should be in CoreCLR next to the ILAsm package, so that it can easily be used/consumed from source-build (and related efforts).
FYI. @tmat, @jaredpar, @rainersigwald, @eerhardt, @weshaggard, @alexperovich, @MichalStrehovsky (all the people who reviewed/commented on the previous PR).