Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

gkhanna79
Copy link
Member

They are generated for Windows, Linux (Centos, Debian, Ubuntu) and OSX.

SOS is placed next to CoreCLR.

@weshaggard @ericstj PTAL. Also, the following are outstanding:

  1. Determine how to insert reference to the APISets nupkg
  2. Unify this package with Razzle build so that we have a single nuget package generation

@gkhanna79
Copy link
Member Author

@gkhanna79
Copy link
Member Author

@dotnet-bot test this please

CPUName=$(uname -p)
# Some Linux platforms report unknown for platform, but the arch for machine.
if [ $CPUName = "unknown" ]; then
if [ $CPUName == "unknown" ]; then

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shell portability issue. This will fail on NetBSD. == is extension in test(1).

@krytarowski
Copy link

I forgot that build.sh enforces gnu bash. Do with my notes whatever you like. Thanks for your patch.

Hopefully there will be added NetBSD platform soon too.

@krytarowski
Copy link

..but if you like, converting it to /bin/sh and reducing dependency shouldn't be too difficult.

build.sh Outdated
if [ "$(cat /etc/*-release | grep -cim1 ubuntu)" -eq 1 ]; then
export __DistroName=ubuntu
elif [ "$(cat /etc/*-release | grep -cim1 centos)" -eq 1 ]; then
export __DistroName=centos
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @ellismg is changing this to rhel.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ellismg Let me know if rhel is the final naming for centos scenario and I will do the needful.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please change __DistroName to rhel. It should be fine to have both centos and rhel builds produce packages with the rhel.7 RID for now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point, we are going to need to extend these checks to bring in the version of the distro so we don't produce packages with an Ubuntu 14.04 RID when building on Ubuntu 15.10. I wouldn't let that block progress here, but we should file an issue so we know to follow up. I think @ericstj has a similar issue in CoreFX around building the "wrong" packages on Linux because today the build doesn't know about which Linux distro it is targeting and so we just produce all the packages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is fine. We should have this function/capability be present in buildtools package (or akin). For me, I picked up the logic from https://github.com/dotnet/cli/blob/a7f438f85d5a68c8f4c61bb422c39bbf3c1e1822/scripts/common/_rid.sh.

Needless to say, there shouldnt be multiple copies for such a function.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should unify this. I oepend dotnet/buildtools#437 to track.

@davidfowl
Copy link
Member

Why aren't mscorlib and coreclr co-located in the package itself?

@ericstj
Copy link
Member

ericstj commented Feb 5, 2016

mscorlib needs to satisfy guardrails so it must be in lib. Ref side is design time facade in portable compatibility pack (for mscorlib-based PCLs).

@davidfowl
Copy link
Member

I think we need it in both places then. It's impossible to use CoreCLR from a package because mscorlib needs to be next to coreclr.dll. We're hitting this right now with the CLI and the only workaround would be to xcopy those 2 dlls into the output path or edit the package cache in place (eek!)

@ericstj
Copy link
Member

ericstj commented Feb 5, 2016

I see, how about both in lib? Lib+ native would bin clash. Alternatively we could have the il in lib and ni in native.

@davidfowl
Copy link
Member

I see, how about both in lib? Lib+ native would bin clash. Alternatively we could have the il in lib and ni in native.

That could work, but now thinking about it, package size might me an issue? Also we'll end up with both the ni and non ni in the output folder. Bin clashing seems better here.

Another question to @gkhanna79 @schellap @jkotas would be to understand why coreclr and mscorlib need to be side by side in the same folder? Maybe the code can be changed so that it looks in the usual places (like the TPA list) for assemblies...

@gkhanna79
Copy link
Member Author

Alternatively we could have the il in lib and ni in native.

@ericstj @davidfowl We dont need IL version of mscorlib since the NI can satisfy that requirement.

mscorlib needs to satisfy guardrails so it must be in lib.

@ericstj Can you please elaborate on this?

@jkotas
Copy link
Member

jkotas commented Feb 5, 2016

Why do we need both mscorlib.ni.dll and mscorlib.dll? mscorlib.ni.dll should be enough. If the .ni.dll suffix is a problem, we can rename it mscorlib.ni.dll to mscorlib.dll.

@davidfowl
Copy link
Member

mscorlib needs to satisfy guardrails so it must be in lib. Ref side is design time facade in portable compatibility pack (for mscorlib-based PCLs).

Can't this be in another package? Why would that be in the runtime package...

@ellismg
Copy link

ellismg commented Feb 5, 2016

I have a vested interest in the raw mscorlib.dll being available from packages somehow. The code-coverage runs we do in CoreFX will not instrument code from mscorlib.ni.dll, last I checked, so being able to restore a version of the run-time that didn't use native images would be great. It may also be helpful in bootstrapping scenarios (e.g. do we know that mscorlib.ni.dll compiled for Linux would work on FreeBSD/OSX/Some Other Linux Distro?)

@@ -0,0 +1,248 @@
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is out of date with what we have in CoreFX for the RID graph, but if you do what @ericstj says above, you can delete this file.

@ericstj
Copy link
Member

ericstj commented Feb 5, 2016

@jkotas ni.dll isn't a problem, I was just suggesting something that would permit an entry in both lib and native without a binclash.

@gkhanna79 gaurdrails is the feature in nuget that caluculates if a package is compatible with a given framework/RID pair. It looks at the compile entries in the compile graph and the runtime entries in the runtime graph and makes sure that every compile assembly has a matching runtime assembly. If not it means the package is not compatible with targeted runtime/framework. https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Commands/CompatilibityChecker.cs

Can't this be in another package? Why would that be in the runtime package...

The design-time facade is in another package. On net-native (where we don't have mscorlib) there is a corresponding runtime facade. On coreclr we can't have a runtime facade since it has a real mscorlib. The right thing to do here is have the real mscorlib satisfy the gaurdrails check for the design-time facade because that accurately models what actually happens at runtime: a v1 PCL with an mscorlib reference will bind directly to the real mscorlib when running on coreclr. We can't have any other package with this mscorlib because it would either a) clash with this one or b) not clash but be unused IL.

I did a quick check to see if native would satisfy the compat check and it does not. We could move the entire runtime to the lib folder which would work on windows, but not on unix since lib is filtered to just dll.

<Version>1.0.1</Version>
<RuntimeOSPath>$(OutputRootPath)CentOs.7.1.1503</RuntimeOSPath>
<SkipPackageFileCheck>true</SkipPackageFileCheck>
<PackageTargetRuntime>centos.7.1-$(PackagePlatform)</PackageTargetRuntime>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you change this stuff to rhel, please use rhel.7 as the prefix in the RID. See dotnet/corefx@f71d339 for the changes I made for RHEL support in CoreFX.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@davidfowl
Copy link
Member

We could have mscorlib.dll IL in a seperate package that's only brought in directly, or in the case of the PCL compatibility check to satisfy gaurdrails. That's likely better anyhow since I think the mscorlib IL pivot isn't as granular as the runtime (distro + arch). Thoughts?

That sounds much better.

@ericstj
Copy link
Member

ericstj commented Feb 6, 2016

Yeah, I'm inclined to do that. It means we'd only en up downloading the IL in the case of someone using the PCL v1 compat package (a legacy package) or directly referencing it.

@gkhanna79
Copy link
Member Author

@ellismg @ericstj Incorporated PR feedback. For now, I am packaging both mscorlib.dll (in lib) and mscorlib.ni.dll (next to CoreCLR).

I would like to keep this change moving unblocked, so we should figure out how to remove the IL version of mscorlib and get nuget railguard verification still working.

@gkhanna79 gkhanna79 force-pushed the FixNugetPkg branch 2 times, most recently from 865b61b to a8fbc6c Compare February 6, 2016 21:25
@gkhanna79
Copy link
Member Author

@mmitche Since we now build mscorlib.dll on the respective OS, as part of this change, I have updated the CI groovy script to no longer build non-Windows mscorlib on Windows. PTAL and let me know if anything else is missing/needs to be done.

@gkhanna79
Copy link
Member Author

why coreclr and mscorlib need to be side by side in the same folder? Maybe the code can be changed so that it looks in the usual places (like the TPA list) for assemblies

Theoretically, CoreCLR and mscorlib could be in different folders and made to work. However, the semantic to keep in mind is that, logically, CoreCLR and mscorlib.dll are a single artifact - they just happen to live in managed and native implementations. Mscorlib is the managed part of the runtime though, due to historical reasons, has had non-runtime specific FX code as well.

To maintain the above semantic, it is important that they are packaged together and deployed together, which has a good side-effect of them being next to each other and also results in Mscorlib's path on TPA to be derived from CoreCLR's path.

@gkhanna79
Copy link
Member Author

I see that CentOS CI invoked the build as " + ./build.sh skipmscorlib verbose checked x64" from a custom shell script. @mmitche Is this CI specific shell script or generated in the repo? I need to have it no longer pass skipmscorlib argument for non-Windows platforms.

@gkhanna79
Copy link
Member Author

@mmitche The Windows build failed due to the following:

d:\j\workspace\dotnet_coreclr\debug_windows_nt_bld_prtest>build.cmd debug x64 linuxmscorlib

This is inspite of my changes in netci.groovy. Is there any other place I need to make change to?

@ellismg
Copy link

ellismg commented Feb 6, 2016

The groovy changes in a PR don't impact that PR. You should do @dotnet-bot test ci please to ask Jenkins to generate the new job definitions, inspect them for correctness and then merge the PR. The changes will be picked up for future jobs.

@ellismg
Copy link

ellismg commented Feb 6, 2016

(Note that because of how the groovy stuff works, it's best to pull the changes into a separate PR and stage your changes so that you can update the CI definitions after updating the product to ensure you don't break the world)

@jkotas
Copy link
Member

jkotas commented Feb 7, 2016

if anything else is missing/needs to be done.

Once this goes through, we should also fix corefx CI to start using mscorlib native image on Unix (right now, the script is deleting it - https://github.com/dotnet/corefx/blob/master/run-test.sh#L169 - it was workaround for Windows native image getting picked up on Unix by accident).

@gkhanna79
Copy link
Member Author

@piotrpMSFT @Sridhar-MS What is the latest version of CLI that works on CentOS? Does restore operation work on CentOS?

build.cmd Outdated
if /i "%1" == "clean" (set __CleanBuild=1&shift&goto Arg_Loop)

REM CI_TODO: Remove this once CI stops building mscorlib for non-Windows OS on Windows.
REM Need to keep FreeBSD until .NET CLI Exists for it.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really like to keep the ability to cross compile mscorlib for other OS's on Windows. We support this in CoreFX for the rest of the managed code and it's very useful for both bootstrapping and local development.

@gkhanna79 gkhanna79 force-pushed the FixNugetPkg branch 2 times, most recently from cc85cb9 to 3190154 Compare February 7, 2016 18:25
@gkhanna79
Copy link
Member Author

@dotnet-bot test Ubuntu x64 Checked Build and Test

gkhanna79 added a commit that referenced this pull request Feb 8, 2016
 Generate Microsoft.NETCore.Runtime.CoreCLR nuget packages
@gkhanna79 gkhanna79 merged commit d53cbfb into dotnet:master Feb 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants