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

Revert "Enable inlining P/Invokes into try blocks with no catch or fi… #73551

Merged
merged 1 commit into from
Aug 8, 2022

Conversation

noahfalk
Copy link
Member

@noahfalk noahfalk commented Aug 8, 2022

…lter clauses (#73032)"

This reverts commit 4c07f3d. We believe it is causing recent CI failures.
See #73247

…lter clauses (dotnet#73032)"

This reverts commit 4c07f3d. We believe it is causing recent CI failures.
See dotnet#73247
@noahfalk
Copy link
Member Author

noahfalk commented Aug 8, 2022

@janvorli @jkotas @am11

@noahfalk
Copy link
Member Author

noahfalk commented Aug 8, 2022

cc @karelz

@jkoritzinsky
Copy link
Member

@hoyosjs was investigating another PR for the same failure IIRC. Let’s make sure we’re on the same page before merging this.

@hoyosjs
Copy link
Member

hoyosjs commented Aug 8, 2022

@jkoritzinsky I only disabled the test since it was blocking PRs. The theorized fix was unrelated. I'm looking more a bit at the raw data.

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

If we think this is the cause, we can revert this for now. I actually made the tracking issue for the failures based on the already existing failures of the test in main before merging in the original PR, so your triage seems accurate.

@filipnavara
Copy link
Member

You should run runtime-extra-platforms on the PR.

@jkoritzinsky
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@noahfalk
Copy link
Member Author

noahfalk commented Aug 8, 2022

We did not validate extra platforms when the original change was checked in so I don't see a reason why we should validate it when the change is being reverted. There are a lot of PR failures happening so I don't think we should hold this up on more validation.

@noahfalk noahfalk merged commit 2a14280 into dotnet:main Aug 8, 2022
@filipnavara
Copy link
Member

We did not validate extra platforms when the original change was checked in so I don't see a reason why we should validate it when the change is being reverted. There are a lot of PR failures happening so I don't think we should hold this up on more validation.

Because some of the linked issues seemed to be related to arm32 builds (unless I misread all the discussion) which started spiking with failures in the same timeframe. Linux ARM32 is part of the extra platforms.

@noahfalk
Copy link
Member Author

noahfalk commented Aug 8, 2022

Linux ARM32 is part of the extra platforms

It isn't only in extra platforms. This Linux arm32 test for GetGCMemoryInfo has been failing 100+ PRs in runtime pipeline (example https://runfo.azurewebsites.net/view/build/?number=1928987). It may also be causing problems in runtime-extra, but that doesn't seem necessary to hit this.

@filipnavara
Copy link
Member

filipnavara commented Aug 8, 2022

The reason I asked for running the extra checks was not necessarily to block the merge on it. I saw it is listed as linux/arm32 on runtime pipeline in the linked issue. I'm a bit puzzled on what actually runs as part of the main checks and what runs only on the extras. For example, I had issue in my Kerberos tests recently that only happened on the arm32 pipelines in the extra runs which leads me to believe that there is only subset of tests run in the runtime pipeline.

Anyway, the reason I asked for the explicit extra run is mainly to see whether the other issues that cropped up in the similar time frame and that are only(?) happening on the extra pipelines disappear with the revert or not.

@am11
Copy link
Member

am11 commented Aug 8, 2022

All arm32 legs have passed https://github.com/dotnet/runtime/runs/7731954937 (other extra-platforms pipeline legs are still running but arm32 legs are green).

@filipnavara
Copy link
Member

@am11 It appears "Libraries Test Run release coreclr Linux arm Release" didn't run due to some condition not being met. 😞

@am11
Copy link
Member

am11 commented Aug 8, 2022

The original test GetGCMemoryInfo was failing in coreclr Pri0 Runtime Tests Run Linux arm checked. That test was disabled in #73477, we should re-enable that test.

@noahfalk
Copy link
Member Author

noahfalk commented Aug 8, 2022

I am re-enabling the test in #73595. I also queued extra-platforms to run in that PR so we can get a complete view of what the current failures look like with both Jeremy's change removed + the GetGCMemoryInfo test re-enabled.

jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this pull request Aug 9, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants