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

[release/6.0] The signal enum in the native library should match the managed code. #58682

Merged
merged 2 commits into from
Sep 8, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 4, 2021

Backport of #58658 to release/6.0

/cc @stephentoub @JamesWTruher

Customer Impact

Trying to register for SIGWINCH (window resize) will actually register for SIGCONT (making a suspend process continue), and vice versa.

Testing

CI (a new test was added separately/subsequently to validate our mappings).

Risk

Minimal. It's obvious from code inspection that the code was wrong and is now correct.

PosixSignalSIGWINCH = -6,
PosixSignalSIGCONT = -7,
PosixSignalSIGCONT = -6,
PosixSignalSIGWINCH = -7,
Copy link
Member

Choose a reason for hiding this comment

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

For reference, these values map to

[UnsupportedOSPlatform("windows")]
SIGCONT = -6,
/// <summary>Window resized</summary>
[UnsupportedOSPlatform("windows")]
SIGWINCH = -7,
.

@stephentoub
Copy link
Member

cc: @danmoseley

@stephentoub stephentoub added this to the 6.0.0 milestone Sep 7, 2021
@danmoseley
Copy link
Member

@stephentoub @tmds should we pull #58711 into this also?

@stephentoub
Copy link
Member

@stephentoub @tmds should we pull #58711 into this also?

Either way is fine. I wouldn't block fixing the bug on getting that test pulled in as well.

@danmoseley
Copy link
Member

would you mind cherry picking it in? it will only hold us up a couple hours.

@danmoseley
Copy link
Member

Approved otherwise -- localized fix for self evidently wrong code.

@danmoseley danmoseley added the Servicing-approved Approved for servicing release label Sep 7, 2021
* PosixSignalRegistrationTests.Unix: validate signal pal mapping

* Use InvariantCulture for pid ToString

* Invoke kill through shell.

* Remove InvariantCulture
@stephentoub
Copy link
Member

would you mind cherry picking it in?

Done

@danmoseley
Copy link
Member

@mdh1418 could you please port into 6.0 the fix for The PKCS#12 Exportable flag is not supported.. and any other deterministic failures in runtime-staging? We shouldn't have any known failures here.

@steveisok I'm not sure what to make of the test jobs that just terminated on iOSSimulator and tvOSSimulator. how can we make these diagnosable? I see no dump or relevant info in any of the logs -- is there any more info we can gather than:

[19:59:16] dbug: Test run completed
[19:59:16] dbug: Test run crashed before it started (no log file produced)
[19:59:16] dbug: No crash reports, waiting 30 seconds for the crash report service...
[19:59:47] fail: Application test run crashed
                 No test log file was produced

I'm going to merge this though as I can't see how it could have failures specific to just these.

@danmoseley danmoseley merged commit c975e87 into release/6.0 Sep 8, 2021
@danmoseley danmoseley deleted the backport/pr-58658-to-release/6.0 branch September 8, 2021 01:40
@steveisok
Copy link
Member

@danmoseley We have a few commits that @mdh1418 is working to backport that will clean these up. And to answer your question, there are about 7-8 suites that crash immediately for unknown reasons and seemingly only on CI. We will be skipping them for now until we're able to get a better understanding as to why.

@danmoseley
Copy link
Member

Sounds good thanks @steveisok

@mdh1418
Copy link
Member

mdh1418 commented Sep 9, 2021

#58841 Backport PR is open :) !

@ghost ghost locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants