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

Conversation

gkhanna79
Copy link
Member

Support consuming custom build of ILCompiler.SDK

@gkhanna79
Copy link
Member Author

@piotrpMSFT @brthor @Sridhar-MS @mmitche Are you aware of this failure?

14:04:07 *** Downloading DNX 1.0.0-rc1-update1 ***
14:04:08 file #1: bad zipfile offset (local header sig): 0
14:04:08 file #2: bad zipfile offset (local header sig): 34
14:04:08 file #3: bad zipfile offset (local header sig): 409
14:04:08 file #4: bad zipfile offset (local header sig): 23381
14:04:08 file #5: bad zipfile offset (local header sig): 37704
14:04:08 file #6: bad zipfile offset (local header sig): 37742
14:04:08 file #7: bad zipfile offset (local header sig): 3188352
14:04:08 file #8: bad zipfile offset (local header sig): 3550117
14:04:08 Build step 'Execute shell' marked build as failure

@gkhanna79
Copy link
Member Author

@dotnet-bot test OSX Release Build

1 similar comment
@gkhanna79
Copy link
Member Author

@dotnet-bot test OSX Release Build

@gkhanna79
Copy link
Member Author

Release build now failed with the following:

14:52:54 *** Building stage1 dotnet using downloaded stage0 ... ***
14:52:54 failed to initialize CoreCLR, HRESULT: 0x80004005
14:52:54 Build step 'Execute shell' marked build as failure

@gkhanna79
Copy link
Member Author

@brthor @schellap PTAL. This adds support of --ilcsdkpath to dotnet compile --native.

CC @jkotas

@schellap
Copy link

@gkhanna79, why is --ilcsdkpath available to the customer? We should not make CoreRT build system have these custom hooks and ilcpath should contain what CLI expects by using dotnet publish. We are currently using the custom (just) built ILC SDK in the project.json of stage0 and stage1 to achieve that.

This is how it should work.

dotnet compiler --native
dotnet compile --native --build-arch arm64

To enable cross compilation, we need to change our package layout (eventually with content folders) but for now, lay them out like native/x64/bootstrapper.lib and native/x64_arm64/bootstrapper.lib etc. When publish is issued with the latest CLI (with @brthor 's changes to native folders in publish) in the CoreRT repo, we should end up with the right folder structure.

Then we should push these packages with minor bumped up and make corresponding CLI changes to pick the new package layout based on the build arch.

@jkotas
Copy link
Member

jkotas commented Dec 20, 2015

--ilcpath and --ilcsdkpath are not mainstream switches. They are primarily for CoreRT testing purposes (e.g. mixing release build of the compiler with debug build of the sdk libraries, and vice versa) and for advanced use cases (e.g. cross-compilation - until dotnet has built-in support for it).

@schellap You are right that we can live without these switches (both of them) and always copy everything into the right fixed folder structure, but it is not very convenient to do so.

This comment was marked as spam.

This comment was marked as spam.

@gkhanna79
Copy link
Member Author

We should not make CoreRT build system have these custom hooks and ilcpath should contain what CLI expects by using dotnet publish

CoreRT build system is not updated/modified to consume them. Infact, the default semantic is to always have ILCPath and ILCSdkPath to be the same. However, an explicit switch enables the customization at CLI level. If we make ILC.exe handle --ilcsdkpath:

  1. The logic to specify references from ILCompiler.SDK (e.g. S.P.C.dll and more in future) would be duplicated between CLI and ILC implementations

  2. ILC will be expected to do the job of linking (native components of SDK) which it currently does not do and expects the CLI command to actually do (which it does).

This switch is not just for cross-compilation but also for same architecture compilation when there is need to target a custom build of ILCompiler.SDK.

@schellap
Copy link

If we make ILC.exe handle --ilcsdkpath:

I am not saying ILC should handle the SDK path. I'm saying instead of adding an option to CLI put the right stuff in place where CLI looks using dotnet publish (similar to what CLI does). This would be in x64 folder for normal compilation and x64_arm64 for cross compilation. We already have support to get these folders dropped by publish based on recent work by @brthor for publish making this change only necessary for convenience.

I am fine if it is for convenience or temporary reasons. My concern was that if we tested (i.e., updated our build/test) with this extra option, we are not testing how we ship and how CLI gets things in the right place using dotnet publish. Doing something different internally might mask any problems around the contracts between shipping ILC and CLI.

This comment was marked as spam.

@gkhanna79
Copy link
Member Author

put the right stuff in place where CLI looks using dotnet publish (similar to what CLI does).

This is not intuitive or straightforward when you want to use the ILC that comes with CLI and want to use a SDK that you just built.

We already have support to get these folders dropped by publish based on recent work by @brthor for publish making this change only necessary for convenience.

Once the above change is enabled, --ilcsdkpath will, behind the scenese, simply point to it. It can serve both default and custom scenarios.

My concern was that if we tested (i.e., updated our build/test) with this extra option

We are not explicitly specifying a value for this - it is taking the value of ILCPath (and will take path to the subfolders when they light up). Effectively, we are (and will be) testing what we are shipping.

@schellap
Copy link

This is not intuitive or straightforward when you want to use the ILC that comes with CLI and want to use a SDK that you just built.

I agree with this one being difficult right now and I see that we might want to be able to test in this mode for servicing scenarios.

The changes LGTM.

gkhanna79 added a commit that referenced this pull request Dec 20, 2015
@gkhanna79 gkhanna79 merged commit 2faa8a1 into dotnet:master Dec 20, 2015
@gkhanna79 gkhanna79 deleted the SDKPath branch February 2, 2016 19:41
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.

4 participants