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

changing CPU from int to double #66743

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

mikelle-rogers
Copy link
Member

@mikelle-rogers mikelle-rogers commented Mar 16, 2022

Fixes #62525

@ghost ghost assigned mikelle-rogers Mar 16, 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.

src/native/libs/System.Native/pal_time.c Outdated Show resolved Hide resolved
src/native/libs/System.Native/pal_time.c Show resolved Hide resolved
src/native/libs/System.Native/pal_time.c Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Apr 26, 2022

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@ghost
Copy link

ghost commented Jun 18, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@mikelle-rogers mikelle-rogers requested a review from a team July 5, 2022 14:50
@mikelle-rogers mikelle-rogers marked this pull request as ready for review August 1, 2022 22:39
@hoyosjs
Copy link
Member

hoyosjs commented Aug 2, 2022

@stephentoub @ViktorHofer do you know why the analyzer kicked for IDE0200 on a file that wasn't changed? It says:

##[error]src\libraries\System.Private.CoreLib\src\System\Diagnostics\Tracing\RuntimeEventSource.cs(91,75): error IDE0200: (NETCORE_ENGINEERING_TELEMETRY=Build) Lambda expression can be removed

In here

https://github.com/dotnet/runtime/blob/7d148d2f350dea8580aad34527ec97df642f9ba8/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/RuntimeEventSource.cs#L78

I see how that lambda (and a few others there) is no longer needed, but I don't see why it wasn't flagged before. I also can't reproduce the warnings when running locally with the same command as one of the failing legs.

@jkotas
Copy link
Member

jkotas commented Aug 2, 2022

I also can't reproduce the warnings when running locally with the same command as one of the failing legs.

You need to merge from main. This branch is pretty of date with main. New analyzers have been added in the meantime.

@hoyosjs
Copy link
Member

hoyosjs commented Aug 2, 2022

Normally a push should cause a remerge. For example, the last run did a merge on d2264b2 which is from 19 hours ago.

@jkotas
Copy link
Member

jkotas commented Aug 2, 2022

do you know why the analyzer kicked for IDE0200 on a file that wasn't changed?

This PR is changing the methods called from this file. That's why the analyzer kicked in.

@hoyosjs
Copy link
Member

hoyosjs commented Aug 2, 2022

/azp run dotnet-linker-tests,runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Aug 2, 2022

Normally a push should cause a remerge. For example, the last run did a merge on d2264b2 which is from 19 hours ago.

Yes, the github will remerge from main before running the validation, but it won't update the PR.

If you want to reproduce the exact same build error locally, you need to do the same remerge locally too.

@hoyosjs
Copy link
Member

hoyosjs commented Aug 2, 2022

Nevermind, I see that the signature needs to be a Func. Now it all makes sense.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

The merge from main was not done right.

If you are not sure how to repair it, ping me on Teams and I will walk you through it.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 4, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 4, 2022
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM!

@hoyosjs
Copy link
Member

hoyosjs commented Aug 10, 2022

Failure is known issue #73247

@hoyosjs hoyosjs merged commit 8b1aecb into dotnet:main Aug 10, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 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.

Change the cpu-usage EventCounter from int to double
5 participants