Skip to content

[LoongArch64] add coreclr-libraries directory. - #62888

Merged
jkotas merged 20 commits into
dotnet:mainfrom
shushanhf:main_loongarch64_5
Jan 7, 2022
Merged

[LoongArch64] add coreclr-libraries directory.#62888
jkotas merged 20 commits into
dotnet:mainfrom
shushanhf:main_loongarch64_5

Conversation

@shushanhf

@shushanhfshushanhf commented Dec 16, 2021

Copy link
Copy Markdown
Contributor

[LoongArch64] add coreclr-libraries directory.

@ghostghost added community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation labels Dec 16, 2021
@ghost

ghost commented Dec 16, 2021

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost

ghost commented Dec 16, 2021

Copy link
Copy Markdown

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.

jkotas
jkotas reviewed Dec 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ImageFileMachine.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/ProcessorArchitecture.cs Outdated
Comment threadsrc/libraries/System.Runtime.InteropServices.RuntimeInformation/tests/CheckArchitectureTests.cs Outdated
Comment threadsrc/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/Machine.cs Outdated
@ghost

ghost commented Dec 16, 2021

Copy link
Copy Markdown

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

Issue Details

[LoongArch64] add coreclr-libraries directory.

Author:shushanhf
Assignees:-
Labels:

area-System.Runtime, new-api-needs-documentation, community-contribution

Milestone:-

@huoyaoyuan

huoyaoyuan commented Dec 16, 2021

Copy link
Copy Markdown
Member

Notes for updating libraries:

The reference source should also be updated together with implementation when any public api changes. For other libraries the ref source is under the ref folder besides the src directory. It can be also accessed from the corresponding Visual Studio solution. For CoreLib, the ref source is mostly under libraries/System.Runtime/src.

When updating the CoreCLR version of CoreLib, don't forget Mono (under src\mono\System.Private.CoreLib) and NativeAOT (under src\coreclr\nativeaot\System.Private.CoreLib, merged recently) versions.

jkotas
jkotas reviewed Dec 16, 2021
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Reflection/AssemblyName.CoreCLR.cs Outdated
jkotas
jkotas reviewed Dec 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs Outdated
{
int x = (((int)_flags) & 0x70) >> 4;
if (x > 5)
if (x > 6)

@jkotasjkotasDec 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The changes in this file should be reverted.

@shushanhfshushanhfDec 17, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

OK,Thanks.

jkotas
jkotas reviewed Dec 16, 2021
View reviewed changes
Comment threadsrc/libraries/System.Runtime/ref/System.Runtime.cs Outdated
@jkotas

jkotas commented Dec 16, 2021

Copy link
Copy Markdown
Member

I see that you are always squashing your changes into the single commit. Could you please push new commits instead of squashing? We prefer that the changes done based on codereview feedback are done as separate commits to make it easy to see what changed. We will squash all changes as part of the merge.

More details are about our PR process are in https://github.com/dotnet/runtime/blob/main/docs/pr-guide.md#quick-code-review-rules

@shushanhf

shushanhf commented Dec 17, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Notes for updating libraries:

The reference source should also be updated together with implementation when any public api changes. For other libraries the ref source is under the ref folder besides the src directory. It can be also accessed from the corresponding Visual Studio solution. For CoreLib, the ref source is mostly under libraries/System.Runtime/src.

When updating the CoreCLR version of CoreLib, don't forget Mono (under src\mono\System.Private.CoreLib) and NativeAOT (under src\coreclr\nativeaot\System.Private.CoreLib, merged recently) versions.

Thanks for your review and suggestion~

@shushanhf

shushanhf commented Dec 17, 2021

Copy link
Copy Markdown
ContributorAuthor

I see that you are always squashing your changes into the single commit. Could you please push new commits instead of squashing? We prefer that the changes done based on codereview feedback are done as separate commits to make it easy to see what changed. We will squash all changes as part of the merge.

More details are about our PR process are in https://github.com/dotnet/runtime/blob/main/docs/pr-guide.md#quick-code-review-rules

Thanks !
I will update based on the previous patch.

@shushanhf

shushanhf commented Dec 21, 2021

Copy link
Copy Markdown
ContributorAuthor

Now this PR can be compiled sucessfully by the CI on the native mode.

What should I do next ?

@shushanhf

shushanhf commented Jan 5, 2022

Copy link
Copy Markdown
ContributorAuthor

@shushanhf This API was approved so this PR is getting close to getting merged.

Could you please resolve the merge conflicts?

Is this PR ok now ?

@jkotas

jkotas commented Jan 5, 2022

Copy link
Copy Markdown
Member

Is this PR ok now ?

No, it has a bunch of conflicts. You should see "This branch has conflicts that must be resolved".

You need to merge from current main to get these conflicts resolved.

@shushanhf

shushanhf commented Jan 5, 2022

Copy link
Copy Markdown
ContributorAuthor

Is this PR ok now ?

No, it has a bunch of conflicts. You should see "This branch has conflicts that must be resolved".

You need to merge from current main to get these conflicts resolved.

I have a question that needs your help:
How to rebase the PR's patch to the latest main branch ?

@shushanhf

shushanhf commented Jan 5, 2022

Copy link
Copy Markdown
ContributorAuthor

Is this PR ok now ?

No, it has a bunch of conflicts. You should see "This branch has conflicts that must be resolved".
You need to merge from current main to get these conflicts resolved.

I have a question that needs your help: How to rebase the PR's patch to the latest main branch ?

If I merge the main directly to this PR and resolv the conflicts, then I push to this PR, is it OK?

@jkotas

jkotas commented Jan 5, 2022

Copy link
Copy Markdown
Member

Yep

@shushanhf

shushanhf commented Jan 5, 2022

Copy link
Copy Markdown
ContributorAuthor

Yep

OK, thanks.
I will update now.

qiaopengcheng
Merge branch 'main' into main_loongarch64_5
cdcce02
Conflicts:
src/libraries/Common/src/Interop/Unix/System.Native/Interop.ProcessorArchitecture.cs
src/libraries/System.Runtime.InteropServices.RuntimeInformation/ref/System.Runtime.InteropServices.RuntimeInformation.cs
src/libraries/System.Runtime.InteropServices.RuntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/RuntimeInformation.Unix.cs
src/native/libs/System.Native/pal_runtimeinformation.c
@shushanhf

shushanhf commented Jan 5, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Yep

OK, thanks.
I will update now.

Yep

Should this file be modified ？

diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/RuntimeInformation.ProcessArchitecture.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/RuntimeInformation.ProcessArchitecture.cs
index 298d039..0da595b 100644
--- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/RuntimeInformation.ProcessArchitecture.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/RuntimeInformation.ProcessArchitecture.cs
@@ -18,6 +18,8 @@ public static Architecture ProcessArchitecture
=> Architecture.Wasm;
#elif TARGET_S390X
=> Architecture.S390x;
+#elif TARGET_LOONGARCH64
+ => Architecture.LoongArch64;
#else
#error Unknown Architecture
#endif

If need, I will update it.

jkotas
jkotas reviewed Jan 5, 2022
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
@jkotas

jkotas commented Jan 6, 2022

Copy link
Copy Markdown
Member

The last build break is: "The generated CompatibilityMap differs from runtime.compatibility.json and UpdateRuntimeFiles was not specified. Please specify UpdateRuntimeFiles=true to commit the changes."

Could you please run the build with /p:UpdateRuntimeFiles=true and commit the result? It should fix it.

@shushanhf

shushanhf commented Jan 6, 2022

Copy link
Copy Markdown
ContributorAuthor

The last build break is: "The generated CompatibilityMap differs from runtime.compatibility.json and UpdateRuntimeFiles was not specified. Please specify UpdateRuntimeFiles=true to commit the changes."

Could you please run the build with /p:UpdateRuntimeFiles=true and commit the result? It should fix it.

Thanaks ~
I will do it now.

@shushanhf

shushanhf commented Jan 6, 2022

Copy link
Copy Markdown
ContributorAuthor

The last build break is: "The generated CompatibilityMap differs from runtime.compatibility.json and UpdateRuntimeFiles was not specified. Please specify UpdateRuntimeFiles=true to commit the changes."

Could you please run the build with /p:UpdateRuntimeFiles=true and commit the result? It should fix it.

Hi, @jkotas

Running the command ./build.sh libs -lc release with the following patch,

qiao@deb-x64:~/work_qiao/runtime$ git diff
diff --git a/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj b/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
index 1351a67f30f..5c3e395a0fc 100644
--- a/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
+++ b/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
@@ -75,7 +75,7 @@
RuntimeJson="runtime.json"
CompatibilityMap="runtime.compatibility.json"
RuntimeDirectedGraph="$(OutputPath)runtime.json.dgml"
- UpdateRuntimeFiles="$(UpdateRuntimeFiles)" />
+ UpdateRuntimeFiles="True" />
</Target>
</Project>

Is this right ?

I didn't meet the error https://github.com/dotnet/runtime/pull/62888/files#annotation_2513131987
by the command ./build.sh libs -lc release.
How to test and reproduce the error ?
Thanks

@jkotas
Run /p:UpdateRuntimeFiles=true
712e19f
cd src/libraries/Microsoft.NETCore.Platforms/src
dotnet build -c Release Microsoft.NETCore.Platforms.csproj /p:UpdateRuntimeFiles=true /t:UpdateRuntimeJson
@jkotas

jkotas commented Jan 7, 2022

Copy link
Copy Markdown
Member

I have run:

cd src/libraries/Microsoft.NETCore.Platforms/src
dotnet build -c Release Microsoft.NETCore.Platforms.csproj /p:UpdateRuntimeFiles=true /t:UpdateRuntimeJson

and pushed up the result. It should fix the build break.

@shushanhf

shushanhf commented Jan 7, 2022

Copy link
Copy Markdown
ContributorAuthor

I have run:

cd src/libraries/Microsoft.NETCore.Platforms/src
dotnet build -c Release Microsoft.NETCore.Platforms.csproj /p:UpdateRuntimeFiles=true /t:UpdateRuntimeJson

and pushed up the result. It should fix the build break.

Thanks,

@shushanhf

shushanhf commented Jan 7, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

build -c Release Microsoft.NETCore.Platforms.csproj /p:UpdateRuntimeFiles=true /t:UpdateRuntimeJson

@jkotas
Thanks for you!

I runned the command and the results is :

qiao@deb-x64:~/work_qiao/runtime/src/libraries/Microsoft.NETCore.Platforms/src$ ~/work_qiao/runtime/.dotnet/dotnet build -c Release Microsoft.NETCore.Platforms.csproj /p:UpdateRuntimeFiles=true /t:UpdateRuntimeJson
Microsoft (R) Build Engine version 17.0.0+c9eb9dd64 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.
Determining projects to restore...
Restored /data2_users/work_qiao/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj (in 217 ms).
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:00:04.42

and the modified files are:

qiao@deb-x64:~/work_qiao/runtime$ git st .
On branch main_loongarch64_5
Your branch is up to date with 'origin/main_loongarch64_5'.
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git checkout -- <file>..." to discard changes in working directory)
modified: src/libraries/Microsoft.NETCore.Platforms/src/runtime.compatibility.json
modified: src/libraries/Microsoft.NETCore.Platforms/src/runtime.json

and the corresponding differences liking the 712e19f

@jkotas

jkotas commented Jan 7, 2022

Copy link
Copy Markdown
Member

Closing&reopening to trigger the CI

@jkotasjkotas closed this Jan 7, 2022
@jkotasjkotas reopened this Jan 7, 2022
@jkotas
jkotas merged commit c49c65d into dotnet:mainJan 7, 2022
@ghostghost locked as resolved and limited conversation to collaborators Feb 6, 2022
@shushanhf
shushanhf deleted the main_loongarch64_5 branch May 11, 2022 01:12
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes
+1 more reviewer
@hez2010hez2010hez2010 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

arch-loongarch64area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shushanhf@huoyaoyuan@jkotas@hez2010@BruceForstall