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.

Sign up for GitHub

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

Jump to bottom

[release/5.0] Remove usages of native bootstrapping (#65901) #66406

Merged
merged 3 commits into from
Mar 15, 2022
Merged

[release/5.0] Remove usages of native bootstrapping (#65901) #66406

merged 3 commits into from
Mar 15, 2022

Conversation

agocke
Copy link
Member

@agocke agocke commented Mar 9, 2022

  • Remove usages of native bootstrapping
  • Make sure cmake is in the path for mono wasm builds

Co-authored-by: Adeel Mujahid [email protected]
(cherry picked from commit 8727ac7)

@hoyosjs @agocke
Remove usages of native bootstrapping (dotnet#65901)
50c57b0 
* Remove usages of native bootstrapping
* Make sure cmake is in the path for mono wasm builds

Co-authored-by: Adeel Mujahid <[email protected]>
(cherry picked from commit 8727ac7)
@agocke agocke requested a review from marek-safar as a code owner March 9, 2022 20:15
@ghost ghost assigned agocke Mar 9, 2022
@dotnet-issue-labeler
Copy link

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.

@agocke agocke requested a review from akoeplinger March 9, 2022 20:16
@agocke agocke changed the title Remove usages of native bootstrapping (#65901) [release/5.0] Remove usages of native bootstrapping (#65901) Mar 9, 2022
@agocke
Copy link
Member Author

agocke commented Mar 9, 2022

Hmm, looks like this might require more work for 5.0. I wonder if not using Ninja is making a difference here.

@agocke
Copy link
Member Author

agocke commented Mar 9, 2022

@hoyosjs do these failures look familiar? Also @jkoritzinsky

@ericstj ericstj mentioned this pull request Mar 9, 2022
Merged
@hoyosjs
Copy link
Member

hoyosjs commented Mar 9, 2022

This might need ce4bf13

@jkoritzinsky @agocke
Fix building CoreCLR for x86 with the Windows 10.0.20348.0 SDK (dotne…
ff04d44 
…t#57067)

The SDK now defines CONTEXT_UNWOUND_TO_CALL in more cases, so we get a macro redefinition error. Since the SDK defines it to the same value as we do, just skip our definition if it's already defined.

(cherry picked from commit ce4bf13)
@ghost
Copy link

ghost commented Mar 10, 2022

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

Issue Details
  • Remove usages of native bootstrapping
  • Make sure cmake is in the path for mono wasm builds

Co-authored-by: Adeel Mujahid [email protected]
(cherry picked from commit 8727ac7)

Author: agocke
Assignees: agocke
Labels:

area-Infrastructure
Milestone: -

@agocke
Copy link
Member Author

agocke commented Mar 10, 2022

That seemed to have helped arm32, but not arm64 -- unfortunately I don't have a machine with VS2019 so I can't repro this locally

@akoeplinger
Copy link
Member

akoeplinger commented Mar 10, 2022 •
edited
Loading

It probably needs the changes from 8c6a049#diff-75d3892a37c4ce5e28b7c8d451beb0b8537b5f0526159cabbae6f62c877af1bfR173

# CMake does not support the ARM or ARM64 assemblers on Windows when using the 
# MSBuild generator. When the MSBuild generator is in use, we manually compile the assembly files
# using this function.

@akoeplinger
Copy link
Member

akoeplinger commented Mar 10, 2022 •
edited
Loading

Do we really need to this when .NET 5 goes out of support in ~two months?

@agocke
Copy link
Member Author

agocke commented Mar 10, 2022

This can produce a lot of stability issues, so it's good to get the product in a good state, even if it's going out of support

@agocke agocke requested a review from ericstj March 11, 2022 04:14
@agocke
Copy link
Member Author

agocke commented Mar 11, 2022

I think this fixes the build problems. Test problems are probably unrelated.

@hoyosjs
Copy link
Member

hoyosjs commented Mar 11, 2022

The failures are pretty odd here. The tests are done within 40 seconds, but it gets killed after not having exited in 16 mins. cc: @dotnet/dnceng as I have no idea what could be done here to investigate, other than checking out a machine and see if it repros. Also, cc: @jbarton as it's odd that this is happening only on the crypto workitems.

For example https://helix.dot.net/api/jobs/7cebc4b8-65f5-4054-a2d9-a2033d89123e/workitems/System.Security.Cryptography.Algorithms.Tests?api-version=2019-06-17

@lpatalas
Copy link

cc: dotnet/dnceng as I have no idea what could be done here to investigate

I'm not sure what's happening here but it seems intermittent. For example 7 out of the last 4k runs of System.Security.Cryptography.Algorithms.Tests finished with the same timeout error. I don't see any pattern there so far. It seems like someone would need to reproduce it on standalone VM to be able to tell more.

@akoeplinger
Copy link
Member

Apart from the Crypto timeout it looks like the failures are unrelated:

@agocke
Copy link
Member Author

agocke commented Mar 11, 2022

@hoyosjs anything else?

@hoyosjs
Copy link
Member

hoyosjs commented Mar 12, 2022

From an infra perspective - the changes LGTM. The failures are oddly hitting Crypto, seems consistent. Not sure how this would be related though.

@carlossanlop
Copy link
Member

@hoyosjs @akoeplinger still need a sign-off . Can one of you provide it if the change and the CI look good to you? Then I can merge it.

hoyosjs
hoyosjs approved these changes Mar 12, 2022
View reviewed changes
Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

Without this we won't even be able to get a build. I think the failures should still be taken a look at.

@carlossanlop
Copy link
Member

carlossanlop commented Mar 12, 2022 •
edited
Loading

@agocke @hoyosjs would you consider any of the above failures related to this change, or can I merge it?:

The failures seem to be the same on both CI legs:

  • Libraries Test Run release coreclr Windows_NT x86 Release (windows.amd64.server2022.open.svc)
  • Libraries Test Run release coreclr Windows_NT x64 Debug (windows.amd64.server2022.open.svc)

Failures:

  • System.Security.Cryptography.Algorithms - didn't seem to have any failures, but all the tests were skipped (not sure why they show up as failed, though):

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-66406-merge-7cebc4b865f54054a2/System.Security.Cryptography.Algorithms.Tests/1/console.13b29219.log?sv=2019-07-07&se=2022-03-31T02%3A34%3A29Z&sr=c&sp=rl&sig=j7ZGR7zy057zUasKBWxR7RMjvGQmwD4C0OskQp3NQT0%3D

  • System.IO.FileSystem - The tests that verify a very long path, failed. The paths were preceded with the extended path prefix \\?\ as expected. I noticed these tests are running in windows.amd64.server2022. I do not know if these machines have long paths enabled by default. That could be the reason.
Expected one of: (System.IO.PathTooLongException, System.IO.DirectoryNotFoundException)
Actual: (System.IO.IOException)

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-66406-merge-4d22085f3c51418993/System.IO.FileSystem.Tests/1/console.e1434b0c.log?sv=2019-07-07&se=2022-03-31T02%3A34%3A30Z&sr=c&sp=rl&sig=VXtCINtD42MKFIb3OqxIKODMFTIZD1IuxgB6Q1Oy6VI%3D

  • System.Net.Http.Functional and System.Net.Security: All the issues seem to be caused by not being able to authenticate.

Exception examples:

System.Security.Authentication.AuthenticationException : Authentication failed because the remote party sent a TLS alert: 'HandshakeFailure'.

System.ComponentModel.Win32Exception : The client and server cannot communicate, because they do not possess a common algorithm.

System.AggregateException : One or more errors occurred. (One or more errors occurred. ( Received an unexpected EOF or 0 bytes from the transport stream.)) (One or more errors occurred. (An error occurred while sending the request.))
      ---- System.AggregateException : One or more errors occurred. ( Received an unexpected EOF or 0 bytes from the transport stream.)
      -------- System.IO.IOException :  Received an unexpected EOF or 0 bytes from the transport stream.
      ---- System.AggregateException : One or more errors occurred. (An error occurred while sending the request.)
      -------- System.Net.Http.HttpRequestException : An error occurred while sending the request.
      ------------ System.Net.Http.WinHttpException : Error 12175 calling WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, 'A security error occurred'.

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-66406-merge-4d22085f3c51418993/System.Net.Http.Functional.Tests/1/console.f7fecafa.log?sv=2019-07-07&se=2022-03-31T02%3A34%3A30Z&sr=c&sp=rl&sig=VXtCINtD42MKFIb3OqxIKODMFTIZD1IuxgB6Q1Oy6VI%3D

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-66406-merge-4d22085f3c51418993/System.Net.Security.Tests/1/console.ef773acf.log?sv=2019-07-07&se=2022-03-31T02%3A34%3A30Z&sr=c&sp=rl&sig=VXtCINtD42MKFIb3OqxIKODMFTIZD1IuxgB6Q1Oy6VI%3D

@agocke agocke mentioned this pull request Mar 14, 2022
Merged
jeffschwMSFT
jeffschwMSFT approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. We should treat this as tell mode. Please work with @carlossanlop on a good time to merge.

@agocke
Copy link
Member Author

agocke commented Mar 14, 2022

The test failures seem unrelated -- machine issues, and the crypto tests passed in the checked configuration. I kicked off a re-run to see if the test hang goes away on retry

@hoyosjs
Copy link
Member

hoyosjs commented Mar 15, 2022

They repeat... which is kind of odd.

@agocke
Copy link
Member Author

agocke commented Mar 15, 2022

OK, confirmed this is #64389, we should be good to merge

@dotnet dotnet locked and limited conversation to collaborators Mar 15, 2022
@dotnet dotnet unlocked this conversation Mar 15, 2022
@carlossanlop carlossanlop merged commit 5883f75 into dotnet:release/5.0 Mar 15, 2022
@agocke agocke deleted the backport-65901 branch March 15, 2022 02:12
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jeffschwMSFT jeffschwMSFT jeffschwMSFT approved these changes

@hoyosjs hoyosjs hoyosjs approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@ericstj ericstj Awaiting requested review from ericstj

Assignees

@agocke agocke

Labels
area-Infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@agocke @hoyosjs @akoeplinger @lpatalas @carlossanlop @jeffschwMSFT @danmoseley @jkoritzinsky