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

Alternative fix of #60182 #60298

Merged
merged 3 commits into from
Nov 15, 2021
Merged

Alternative fix of #60182 #60298

merged 3 commits into from
Nov 15, 2021

Conversation

sakno
Copy link
Contributor

@sakno sakno commented Oct 12, 2021

Alternative way to fix #60182 without try-catch block. Quick fix is available as separated PR #60224

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 12, 2021
@ghost
Copy link

ghost commented Oct 12, 2021

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

Alternative way to fix #60182 without try-catch block. Quick fix is available as separated PR #60224

Author: sakno
Assignees: -
Labels:

area-System.Threading

Milestone: -

@mangod9
Copy link
Member

mangod9 commented Nov 8, 2021

Hi @sakno, could you please rebase to main and once CI completes we could get this merged. Thx.

@sakno
Copy link
Contributor Author

sakno commented Nov 8, 2021

@mangod9 , done.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks.

@danmoseley
Copy link
Member

@BruceForstall formatting failed https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/1459736/logs/217

I'll merge as this didn't touch the JIT code (should formatting only trigger if the JIT tree is changed?)

@danmoseley danmoseley merged commit 17d8fe5 into dotnet:main Nov 15, 2021
@BruceForstall
Copy link
Member

@danmoseley It looks like the JIT formatting job runs anytime there's a change in coreclr, and that appears to include System.Private.CoreLib: https://dev.azure.com/dnceng/public/_build/results?buildId=1459736&view=logs&jobId=db5b9618-eefc-58e8-4554-fbce452d4dac&j=c8204876-824e-5bf9-8c45-a4628bfcec7d&t=c0cab5ad-9de6-5568-2ae4-6c24d2f3402f

@safern can probably explain this better.

Maybe we could add more granularity there to allow only running if there's a change in the JIT directory itself.

As for the failure, I did briefly break the job last week (fixed with #61334), but it's not clear if this change was unlucky to pick up the break, or hit some other unknown error.

@danmoseley
Copy link
Member

what folders should it trigger on exactly? then we can create a new group here

- src/libraries/System.Private.CoreLib/*

and then use it here

eq(dependencies.evaluate_paths.outputs['SetPathVars_coreclr.containsChange'], true),

@BruceForstall
Copy link
Member

@danmoseley Something like: #61632? (can move discussion there)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Threading community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CTS.TryReset() concurrency issue
5 participants