Revive #48505 #54914

ViktorHofer · 2021-06-29T16:42:50Z

Revive #48505 and try to find out why the change caused official builds to break.

I couldn't apply the patch in src/tests/build.sh as sources changed: https://github.com/dotnet/runtime/pull/48505/files#diff-056300437704f257a4c8567b61dfdb647da75c5ef93baefec47709796792f182.

cc @am11 @ericstj

Revive #48505 and try to find out why the change caused official builds to break.

dotnet-issue-labeler · 2021-06-29T16:42:54Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ViktorHofer · 2021-06-29T17:09:35Z

Internal build which should then contain the binlog: https://dnceng.visualstudio.com/internal/_build/results?buildId=1211606&view=results.

ViktorHofer · 2021-06-29T19:00:25Z

@ericstj the binlog is available in case you want to take a look.

am11 · 2021-06-29T19:16:16Z

Thanks @ViktorHofer!

Does it fix the official build issue which was encountered previously (#48647)?
Few things have changed since, I am looking into FreeBSD cross-build error:

/__w/1/s/.dotnet/sdk/6.0.100-preview.4.21255.9/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(101,5): error NETSDK1084: There is no application host available for the specified RuntimeIdentifier 'freebsd-x64'. [/__w/1/s/Build.proj]

ViktorHofer · 2021-06-29T19:25:46Z

Does it fix the official build issue which was encountered previously (#48647)?

Unfortunately not yet. I opened the PR so that we can take a look at the binlog that is produced in the internal build to figure out what went wrong. @ericstj mentioned that a binlog should tell.

am11 · 2021-06-29T20:18:28Z

FreeBSD error is due to crossgen2's dependency on internal property, the one which starts with underscore (which as I understood it, is discouraged)

runtime/src/coreclr/tools/aot/crossgen2/crossgen2.csproj

Line 10 in 53e6b47

    
           <AppHostRuntimeIdentifier>$(_runtimeOS)-$(TargetArchitectureAppHost)</AppHostRuntimeIdentifier>

changing this line to <AppHostRuntimeIdentifier>$(PackageRID)</AppHostRuntimeIdentifier> fixes the error on FreeBSD cross leg.

ViktorHofer · 2021-06-29T20:20:44Z

Great observation. Feel free to push into the branch if you want to :)

am11 · 2021-06-29T20:31:16Z

I do not have push access to this branch's remote, pushed to mine am11@f7a6ca2.

src/coreclr/tools/aot/crossgen2/crossgen2.csproj

Co-authored-by: Adeel Mujahid <[email protected]>

am11 · 2021-06-30T13:51:24Z

I couldn't apply the patch in src/tests/build.sh as sources changed: https://github.com/dotnet/runtime/pull/48505/files#diff-056300437704f257a4c8567b61dfdb647da75c5ef93baefec47709796792f182.

To make the test legs green, we would still need to apply the modified form of that patch to make it propagate /p:CrossBuild which we set in top level shell script (eng/build.sh) and want it to be available in all child contexts:

--- a/src/tests/build.sh
+++ b/src/tests/build.sh
@@ -573,6 +573,10 @@ if [[ "${__BuildArch}" != "${__HostArch}" ]]; then
     __CrossBuild=1
 fi

+if (( __CrossBuild == 1 )); then
+    __UnprocessedBuildArgs+=("/p:CrossBuild=true")
+fi
+

ViktorHofer · 2021-06-30T14:18:38Z

To make the test legs green, we would still need to apply the modified form of that patch to make it propagate /p:CrossBuild which we set in top level shell script (eng/build.sh) and want it to be available in all child contexts:

Applied a minor variation of your patch. Does that look right?

am11 · 2021-06-30T14:30:58Z

Yup, yours is more consistent with the repo style 👍 (it was just numerical comparison instead of string-y one but consistency takes precedence)

src/tests/build.sh

Co-authored-by: Adeel Mujahid <[email protected]>

am11 · 2021-07-02T10:45:18Z

Remaining failures are unrelated to PR changes. It also does not break the developer workflow, including Windows/VisualStudio and Alpine Linux.

Internal build which should then contain the binlog: https://dnceng.visualstudio.com/internal/_build/results?buildId=1211606&view=results.

@ViktorHofer, (since I cannot access this link) could you please paste the build command which fails on that leg? I will also have a look.

am11 · 2021-07-04T04:56:20Z

One difference between public CI legs and internal build could be that public CI is passing OutputRid explicitly to the cross compilation legs, and perhaps the internal build is not? The reason why we can't rely on the "calculated OutputRid" is that it has two different meaning, which is what this change aims to align.

This change is:

PackageRID: used to restore packages; in cross compilation, its value is the RID of host (or build) machine.
OutputRid: used to "create" .nupkgs; in cross compilation, its value is the RID of target machine.

(without any weird exceptional cases, like we have in main branch..)

If you could cherry-pick am11@c01d07c commit on your PR's branch. that should fix the internal build without modifying its .yml file. If not, then I would need binlog / more info to understand the problem.

ViktorHofer · 2021-07-05T15:27:51Z

Triggered a new internal build: https://dnceng.visualstudio.com/internal/_build/results?buildId=1220899&view=results.

I don't think sharing the internal binlog is a good idea because of potential secrets/env vars but will at least share the command output to let you know if the patch was successful.

Thanks @am11 for your continuous help :)

Thefrank · 2021-07-06T03:00:06Z

Will this also resolve the issue of native builds (in my case FreeBSD) trying to restore Linux ILASM/ILDASM instead of FreeBSD?
The binlog that was generated seemed to indicate a similar (maybe exact?) RID/variable issue.

am11 · 2021-07-06T03:27:11Z

@Thefrank, if you are referring to #55133, then yes. 🙂

ViktorHofer · 2021-07-06T06:45:31Z

Awesome, the internal build succeeded 💯

akoeplinger

Looks like a good step in the right direction, but would love to simplify it even more :)

akoeplinger · 2021-07-06T09:16:29Z

Directory.Build.props

+    <_hostOS Condition="'$(TargetOS)' == 'Browser'">Browser</_hostOS>
+    <TargetOS Condition="'$(TargetOS)' == ''">$(_hostOS)</TargetOS>


from what I understood _hostOS is the OS where the build runs right? in that case Browser doesn't make sense here (I assume we have some other wrong assumption about this somewhere else so this is needed right now).

Makes sense. Let's untangle this when we eliminate remaining properties which are conflicting with each other.

I think in case of browser, value of _hostOS is used in PackageRID below. For other platforms, it is also used to calculate OutputRid (through _portableOS) and MicrosoftNetCoreIlasmPackageRuntimeId when cross-building.

ViktorHofer · 2021-07-06T09:39:22Z

I see a lot work items stopping during execution and the console log doesn't tell what went wrong. @dotnet/dnceng can you please take a look?

I.e. https://dev.azure.com/dnceng/public/_build/results?buildId=1220900&view=ms.vss-test-web.build-test-results-tab&runId=36394826&resultId=176021&paneView=debug

ilyas1974 · 2021-07-06T14:31:44Z

Thank you for letting us know. I have created https://github.com/dotnet/core-eng/issues/13596 to investigate this.

clamp03 · 2021-07-07T05:31:32Z

src/coreclr/tools/aot/crossgen2/crossgen2.csproj

    <TargetArchitectureAppHost Condition="'$(TargetArchitectureAppHost)'=='armel'">arm</TargetArchitectureAppHost>

-    <AppHostRuntimeIdentifier>$(_runtimeOS)-$(TargetArchitectureAppHost)</AppHostRuntimeIdentifier>
+    <AppHostRuntimeIdentifier>$(PackageRID)</AppHostRuntimeIdentifier>


ARMEL build is broken because target architecture is not updated to arm for crossgen2.
Please fix it. Thank you.

@clamp03 can you please open a new issue for that? @am11 do you have time to look at the regression?

armel is another community supported platform, which gets broken time to time. For community related things to sustain in the continuous development (for the busy repo like runtime), there should be a CI leg IMO (as we added one last year for GCC build validation). I have the next batch of changes in progress, and will fix it in in that batch soon. Meanwhile, you can apply a local patch and get by it (and please consider adding CI leg as it won't be the last armel breakage otherwise).

cc @jkotas @safern regarding the armel CI protection discussion above.

Armel fix is pushed in 55279: https://github.com/dotnet/runtime/pull/55279/files#diff-9da24614831c308827a1ae533ffea392c97638c261dd42bd0f5226baa136d16eR137-R169.

I think it is fine to add an armel CI build that just does a traversal build. Maybe in the dev-innerloop pipeline?

Adding armel + Tizen CI build leg is tracked by #2394

As @safern said, we are open to adding CI build legs to protect community supported configurations. We expect the community to set them up. The steps for how to do that are in #2394 (comment)

safern

LGTM

Revive #48505

0fff6f3

Revive #48505 and try to find out why the change caused official builds to break.

Update build.proj

01975b9

Update crossgen2.csproj

bbfbb07

am11 reviewed Jun 29, 2021

View reviewed changes

src/coreclr/tools/aot/crossgen2/crossgen2.csproj Outdated Show resolved Hide resolved

Update src/coreclr/tools/aot/crossgen2/crossgen2.csproj

a3af296

Co-authored-by: Adeel Mujahid <[email protected]>

Update build.sh

9b11763

am11 reviewed Jun 30, 2021

View reviewed changes

src/tests/build.sh Outdated Show resolved Hide resolved

Update src/tests/build.sh

9986925

Co-authored-by: Adeel Mujahid <[email protected]>

am11 mentioned this pull request Jul 4, 2021

Crosscompile for FreeBSD produce wrongly named ILAsm, ILDAsm, TestHost and native.System.IO.Ports nupkg's #55133

Closed

Use OutputRid when creating .nupkgs, instead of PackageRID

7d7e19d

ViktorHofer closed this Jul 5, 2021

ViktorHofer reopened this Jul 5, 2021

ViktorHofer marked this pull request as ready for review July 6, 2021 06:45

akoeplinger approved these changes Jul 6, 2021

View reviewed changes

ViktorHofer merged commit 566b53a into main Jul 6, 2021

ViktorHofer deleted the ViktorHofer-patch-1 branch July 6, 2021 09:39

clamp03 reviewed Jul 7, 2021

View reviewed changes

safern reviewed Jul 7, 2021

View reviewed changes

kunalspathak mentioned this pull request Jul 14, 2021

Generate CORE_ROOT failing for linux_musl arm/arm64 #55657

Closed

This was referenced Jul 14, 2021

_packageRID has wrong value for runtime tests build for Android Arm64 and Arm32 #55669

Closed

Do not set CrossBuild to true when building runtime tests for Android #55641

Merged

ghost locked as resolved and limited conversation to collaborators Aug 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revive #48505 #54914

Revive #48505 #54914

ViktorHofer commented Jun 29, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Jun 29, 2021

ViktorHofer commented Jun 29, 2021

ViktorHofer commented Jun 29, 2021

am11 commented Jun 29, 2021

ViktorHofer commented Jun 29, 2021

am11 commented Jun 29, 2021

ViktorHofer commented Jun 29, 2021

am11 commented Jun 29, 2021

am11 commented Jun 30, 2021 •

edited

Loading

ViktorHofer commented Jun 30, 2021

am11 commented Jun 30, 2021

am11 commented Jul 2, 2021

am11 commented Jul 4, 2021 •

edited

Loading

ViktorHofer commented Jul 5, 2021

Thefrank commented Jul 6, 2021

am11 commented Jul 6, 2021

ViktorHofer commented Jul 6, 2021

akoeplinger left a comment

akoeplinger Jul 6, 2021

ViktorHofer Jul 6, 2021

am11 Jul 6, 2021 •

edited

Loading

ViktorHofer commented Jul 6, 2021

ilyas1974 commented Jul 6, 2021

clamp03 Jul 7, 2021

ViktorHofer Jul 7, 2021

am11 Jul 7, 2021 •

edited

Loading

ViktorHofer Jul 7, 2021

am11 Jul 7, 2021

safern Jul 7, 2021

jkotas Jul 7, 2021

safern left a comment

		<_hostOS Condition="'$(TargetOS)' == 'Browser'">Browser</_hostOS>
		<TargetOS Condition="'$(TargetOS)' == ''">$(_hostOS)</TargetOS>

Revive #48505 #54914

Revive #48505 #54914

Conversation

ViktorHofer commented Jun 29, 2021 • edited Loading

dotnet-issue-labeler bot commented Jun 29, 2021

ViktorHofer commented Jun 29, 2021

ViktorHofer commented Jun 29, 2021

am11 commented Jun 29, 2021

ViktorHofer commented Jun 29, 2021

am11 commented Jun 29, 2021

ViktorHofer commented Jun 29, 2021

am11 commented Jun 29, 2021

am11 commented Jun 30, 2021 • edited Loading

ViktorHofer commented Jun 30, 2021

am11 commented Jun 30, 2021

am11 commented Jul 2, 2021

am11 commented Jul 4, 2021 • edited Loading

ViktorHofer commented Jul 5, 2021

Thefrank commented Jul 6, 2021

am11 commented Jul 6, 2021

ViktorHofer commented Jul 6, 2021

akoeplinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

ViktorHofer commented Jul 6, 2021

ilyas1974 commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

ViktorHofer commented Jun 29, 2021 •

edited

Loading

am11 commented Jun 30, 2021 •

edited

Loading

am11 commented Jul 4, 2021 •

edited

Loading

am11 Jul 6, 2021 •

edited

Loading

am11 Jul 7, 2021 •

edited

Loading