Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude things which broken on Loongarch #78850

Closed

Conversation

kant2002
Copy link
Contributor

That's temporary stopgap to be able build on LA when have .NET 7.0.100 SDK
Plan to eventually unlock these features, but let's make build green for now.

This project fails on the Loongarch and not super nescessary for now. Unlock build
Hope that's all what's needed for dotnet#78749
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 25, 2022
@ghost
Copy link

ghost commented Nov 25, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

That's temporary stopgap to be able build on LA when have .NET 7.0.100 SDK
Plan to eventually unlock these features, but let's make build green for now.

Author: kant2002
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

Workaround for dotnet#78854
Basically feed built packages from `./build.sh packs` into test layout build process.
Copy link
Contributor Author

@kant2002 kant2002 left a comment

Choose a reason for hiding this comment

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

One more exclusion for LA

@kant2002
Copy link
Contributor Author

kant2002 commented Dec 3, 2022

@jkotas not sure if you are right person to ping regarding these changes.

@@ -5,5 +5,6 @@

<PropertyGroup>
<RuntimeIdentifier>$(OutputRid)</RuntimeIdentifier>
<NoWarn>$(NoWarn);NU1603</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

Disabling this warning does not sound right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test project want to use Microsoft.NETCore.App.Runtime.linux-loongarch64 version 7.0.0 which is not exists. Since I use locally built version it produces 8.0.0-dev and because I'm early in the cycle I "hope" that it's not important yet and while I make things working, I can remove this and use separate feed/folder with proper versions.

Test project which as for package - src/tests/Common/test_dependencies_fs/test_dependencies.fsproj

Copy link
Member

Choose a reason for hiding this comment

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

@kant2002 I think this shouldn't be necessary anymore since NetCoreAppToolCurrent which this project is targetting is set to net8.0 instead of net7.0 now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed warning. Let's see how it worked out from main after this merges.

@@ -9,6 +9,7 @@
<MSBuildEnableAllPropertyFunctions>1</MSBuildEnableAllPropertyFunctions>
<Language>C#</Language>
<RuntimeIdentifier>$(OutputRid)</RuntimeIdentifier>
<RestoreAdditionalProjectSources Condition="$(TargetArchitecture) == 'loongarch64'">$(RestoreAdditionalProjectSources);$(ArtifactsPackagesDir)</RestoreAdditionalProjectSources>
Copy link
Member

Choose a reason for hiding this comment

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

This makes the build use packages that were just build instead of the one from the feed. Can we unify this with the mechanism that source build uses to achieve the same?

cc @MichaelSimons

Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Which live built packages do you need to restore and reference here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's allow me to use Microsoft.NETCore.App.Runtime.linux-loongarch64
as described in #78854

I use this as temporary stopgap to be able to use test scripts for build tests and run them (to some degree)

Copy link
Member

Choose a reason for hiding this comment

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

This makes the build use packages that were just build instead of the one from the feed. Can we unify this with the mechanism that source build uses to achieve the same?

Source-build does not have a special mechanism to do this within a repo. The source-build mechanism is used to pickup packages that were built from a different repo within the source build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically this line not needed, and only similar line in the file is here. But I assume concerns applied to line below.

Copy link
Member

Choose a reason for hiding this comment

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

@ViktorHofer isn't there some other way of forcing tests to use the live built packages?

Copy link
Member

Choose a reason for hiding this comment

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

@kant2002 I'm curious does the restore error only happen for the test_dependencies.fsproj you mentioned in #78854?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not actually know, since I do not fully understand how test infra setup. So I was worrying that I may break something further down the road. I can play according to your directions if you have some ideas to test

@jkotas
Copy link
Member

jkotas commented Dec 3, 2022

@jkotas not sure if you are right person to ping regarding these changes.

The area owners are the best people to ping.

Copy link
Contributor Author

@kant2002 kant2002 left a comment

Choose a reason for hiding this comment

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

Test project want to use Microsoft.NETCore.App.Runtime.linux-loongarch64 version 7.0.0 which is not existsing. Since I use locally built version it produces 8.0.0-dev and because I'm early in the cycle I "hope" that it's not important yet.

src/tests/Common/test_dependencies_fs/test_dependencies.fsproj

UPDATE this was commit message which I by mistake enter in VS Code Online

…h64 version 7.0.0 which is not existsing. Since I use locally built version it produces 8.0.0-dev and because I'm early in the cycle I "hope" that it's not important yet.


src/tests/Common/test_dependencies_fs/test_dependencies.fsproj
@ghost
Copy link

ghost commented Jan 9, 2023

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

That's temporary stopgap to be able build on LA when have .NET 7.0.100 SDK
Plan to eventually unlock these features, but let's make build green for now.

Author: kant2002
Assignees: -
Labels:

area-Infrastructure, community-contribution

Milestone: -

eng/Subsets.props Outdated Show resolved Hide resolved
@agocke
Copy link
Member

agocke commented Apr 10, 2023

I'd expect that special-casing for loongarch and freebsd would be pretty similar (we don't publish packages for either) but it doesn't look quite the same. Any reason for that?

@kant2002
Copy link
Contributor Author

Is it possible that CrossGen2 on FreeBSD magically works because ABI same as on Nix? Or maybe same situation as with me and LA64, nobody run whole build using dotnet infra? Other option is that some GC hole(there at least one) lead to Crossgen2 getting in the loop and as such never finished.

Here my guesses

@@ -320,7 +321,7 @@
$(CoreClrProjectRoot)tools\dotnet-pgo\dotnet-pgo.csproj;
$(CoreClrProjectRoot)tools\aot\ILCompiler\repro\repro.csproj;
$(CoreClrProjectRoot)tools\r2rtest\R2RTest.csproj" Category="clr" Condition="'$(DotNetBuildFromSource)' != 'true'"/>
<ProjectToBuild Include="$(CoreClrProjectRoot)tools\aot\crossgen2\crossgen2.csproj" Category="clr" />
<ProjectToBuild Condition="'$(Crossgen2Supported)' == 'true'" Include="$(CoreClrProjectRoot)tools\aot\crossgen2\crossgen2.csproj" Category="clr" />
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right to me. Crossgen2 should build just fine. It may not run properly, but if we can't build it I think something else is going on.

Copy link
Member

Choose a reason for hiding this comment

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

#84781 disabled running crossgen2 for riscv. It should be done in the same place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some thinking I'm not so sure that this would be enough to just make changes in src/coreclr/crossgen-corelib.proj. Don't know what Risk-V guys did, but in my case I was able to build whole runtime + almost run all tests using standards method.

Will try to resurrect work. When I file issue, it was very ideal, since I have LA64 .NET 7 SDK and now it would require some jumping from the hoops on my side 😢

@huoyaoyuan
Copy link
Member

/cc @shushanhf Have you looked into this?

@shushanhf
Copy link
Contributor

shushanhf commented Apr 20, 2023

/cc @shushanhf Have you looked into this?

Thanks for your reminding.
I didn't see the lastest talking.

@agocke @jkotas @am11

As @kant2002 said, it is very necessary to adding the LA64's testing env although there is still some work to do.
I will focus on the LoongArch64's developping through the rest of the year.

First, I have started to push some PRs to solve the failed tests within CoreRoot. #69705

Second, for testing the LA64, can we implement it in several steps ?
(1) we can just supporting the building LoongArch64 to avoid the simple building errors;
(2) creating an issue to tracking and solving the resting problems.
If needed, we can supply some LoongArch64 machines.

@am11
Copy link
Member

am11 commented Apr 20, 2023

Does #85038 supersede this PR?

@shushanhf
Copy link
Contributor

Does #85038 supersede this PR?

Only this PR is not enough.
Today I will push anthother PR related this.

If all the testing cases passed, I think there is still some other problems.

@jkotas
Copy link
Member

jkotas commented Apr 20, 2023

we can just supporting the building LoongArch64 to avoid the simple building errors

If you meant adding a LoongArch64 build to the CI, we would need a docker image with Linux x64 hosted cross-build environment created in https://github.com/dotnet/dotnet-buildtools-prereqs-docker repo for that. Depends on #63075 .

@@ -365,7 +366,7 @@
Test="true" Category="tools"/>
</ItemGroup>

<ItemGroup Condition="$(_subset.Contains('+clr.nativecorelib+'))">
<ItemGroup Condition="$(_subset.Contains('+clr.nativecorelib+')) and '$(Crossgen2Supported)' == 'true'">
Copy link
Member

Choose a reason for hiding this comment

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

This seems obsolete with the crossgen-corelib change

@shushanhf
Copy link
Contributor

Hi, @kant2002
The #85038 had been merged.
And we have built the hello-world by SDK8.0-Preview4 on LA64 #69705 (comment)

Next we will improve the reliability for LA64.
The crossgen2 maybe is ok for simple tests for LA64.

@kant2002
Copy link
Contributor Author

@shushanhf where can I get SDK8.0-Preview4 for LA ? I prefer to test whole build process if possible.

@shushanhf
Copy link
Contributor

@shushanhf where can I get SDK8.0-Preview4 for LA ? I prefer to test whole build process if possible.

LA's SDK8.0-preview4 can only build liking hello-world or lager FlightFinder, if building the whole runtime maybe have to wait some time.
Now we are improving the reliability for LA64. I will give you a more stable version later.

@agocke
Copy link
Member

agocke commented Jul 17, 2023

@kant2002 is this still relevant? Or should we close it out?

@agocke agocke added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 17, 2023
@ghost ghost added the no-recent-activity label Jul 31, 2023
@ghost
Copy link

ghost commented Jul 31, 2023

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@ghost
Copy link

ghost commented Aug 15, 2023

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

@ghost ghost closed this Aug 15, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-loongarch64 area-Infrastructure community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants