Skip to content

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

Merged
danmoseley merged 2 commits into
release/6.0from
backport/pr-58658-to-release/6.0
Sep 8, 2021
Merged

[release/6.0] The signal enum in the native library should match the managed code.#58682
danmoseley merged 2 commits into
release/6.0from
backport/pr-58658-to-release/6.0

Conversation

@github-actions

@github-actionsgithub-actionsBot commented Sep 4, 2021

Copy link
Copy Markdown
Contributor

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.

@ghostghost added the area-System.Net label Sep 4, 2021
PosixSignalSIGWINCH = -6,
PosixSignalSIGCONT = -7,
PosixSignalSIGCONT = -6,
PosixSignalSIGWINCH = -7,

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.

For reference, these values map to

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

@stephentoub

Copy link
Copy Markdown
Member

cc: @danmoseley

@stephentoubstephentoub added this to the 6.0.0 milestone Sep 7, 2021
@danmoseley

Copy link
Copy Markdown
Contributor

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

@stephentoub

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@danmoseley

Copy link
Copy Markdown
Contributor

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

@danmoseleydanmoseley 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
Copy Markdown
Member

would you mind cherry picking it in?

Done

@danmoseley

Copy link
Copy Markdown
Contributor

@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.0Sep 8, 2021
@danmoseley
danmoseley deleted the backport/pr-58658-to-release/6.0 branch September 8, 2021 01:40
@steveisok

Copy link
Copy Markdown
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
Copy Markdown
Contributor

Sounds good thanks @steveisok

@mdh1418

mdh1418 commented Sep 9, 2021

Copy link
Copy Markdown
Member

#58841 Backport PR is open :) !

@ghostghost locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.NetServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub@danmoseley@steveisok@mdh1418@JamesWTruher@tmds