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

Consume renamed TargetFramework package #64670

Merged
merged 2 commits into from
Feb 3, 2022
Merged

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Feb 2, 2022

Depends on dotnet/arcade#8421

@ghost
Copy link

ghost commented Feb 2, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: -

@ViktorHofer
Copy link
Member Author

@safern @akoeplinger do you know if these failures are caused by my changes?

  • Helix API does not contain an entry for OSX.1014.Amd64.Open
  • Helix API does not contain an entry for Windows.10.Amd64.Client19H1.Ope

cc @dotnet/dnceng

@jonfortescue
Copy link
Member

@ViktorHofer Not caused by your changes. Those queues were both deleted by the latest rollout due to the operating systems reaching end of life.

@garath
Copy link
Member

garath commented Feb 2, 2022

Those queues were both deleted by the latest rollout due to the operating systems reaching end of life.

I believe the change for the OSX queue is happening in #64565

@safern
Copy link
Member

safern commented Feb 2, 2022

The windows one is: #64699

@ViktorHofer
Copy link
Member Author

Thanks all. I'll give this another try tomorrow. Meanwhile @safern or @ericstj would you mind reviewing and approving?

@akoeplinger
Copy link
Member

@ViktorHofer looks like this runs into the same issue as the darc update PR, do you know why the System.Private.CoreLib.Generators.dll ends up in CORE_ROOT: #64376 (comment) ?

@ViktorHofer
Copy link
Member Author

@akoeplinger the NativeAot leg on this PR isn't failing like in #64376 (comment), or am I overlooking something?

@akoeplinger
Copy link
Member

@ViktorHofer yeah that's because #64715 added some handling so the nativeaot doesn't crash, but the underlying issue is still there (and affects the Mono llvm legs)

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Feb 3, 2022

and affects the Mono llvm legs

So the Mono llvm legs should fail on my PR? If not, how can I check if the issue is happening on my PR?

EDIT: Ok I think I know what you mean but I have no idea how that could be caused by my changed. Looking...

@ViktorHofer
Copy link
Member Author

Based on the time this started failing it could be dotnet/arcade@93656ab.

@ViktorHofer
Copy link
Member Author

OK I have a lead.

Projects under src/libraries/ which are located under a "gen" directory
are automatically treated as source generator projects. Because the
CoreLib generator was placed under a different directory "generators",
it was treated as a RuntimeAssembly which gets binplaced into the
runtime path. The runtime tests then take the assemblies from there and
copy them into their CORE_ROOT directory.

Renaming the CoreLib source generators parent directory fixes that so
that it is treated as source generator and it brings consistency into
src/libraries.
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Feb 3, 2022

@akoeplinger the fix is in the second commit. The actual issue existed since the CoreLib source generator was added to src/libraries. In 9580b7d @ericstj disabled binplacing source generators into the runtime path via a convention but CoreLib's source generator didn't comply to that convention as its parent directory was named "generators" instead of "gen".

This popped up with my change now because binplacing didn't work for non multi-targeting projects. Only projects with a <TargetFrameworks /> property were chosen as a binplace candidate and projects with a <TargetFramework /> property were ignored. I fixed that in dotnet/arcade@93656ab and hence the CoreLib generator started to fail (it's the only source generator that doesn't multi-target).

Would appreciate an approval of the second commit (it's just a simple rename). The first commit is already approved.

@MichalStrehovsky
Copy link
Member

@akoeplinger the fix is in the second commit. The actual issue existed since the CoreLib source generator was added to src/libraries. In 9580b7d @ericstj disabled binplacing source generators into the runtime path via a convention but CoreLib's source generator didn't comply to that convention as its parent directory was named "generators" instead of "gen".

Ah, so indeed similar to #62836. I was puzzled what it would be a problem just now. I can sign off on the second commit.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Second commit LGTM

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Feb 3, 2022

In total more than 3 hours for PR validation, that's crazy :O

@ViktorHofer ViktorHofer merged commit 64f3bae into main Feb 3, 2022
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-1 branch February 3, 2022 14:32
@marek-safar
Copy link
Contributor

In total more than 3 hours for PR validation, that's crazy :O

/cc @jkotas

@ericstj
Copy link
Member

ericstj commented Feb 3, 2022

@akoeplinger Alexander Köplinger FTE the fix is in the second commit. The actual issue existed since the CoreLib source generator was added to src/libraries. In 9580b7d @ericstj Eric St. John FTE disabled binplacing source generators into the runtime path via a convention but CoreLib's source generator didn't comply to that convention as its parent directory was named "generators" instead of "gen".

Perhaps we should have stronger rules around project classification. IOW: make sure projects fall into a known convention, specify what they are, otherwise it's an error.

@ericstj
Copy link
Member

ericstj commented Feb 3, 2022

In total more than 3 hours for PR validation, that's crazy :O

Seems this is the long-pole.

CoreCLR Pri0 Runtime Tests Run windows arm64 checked
Duration: 2h 41m 22s

https://dev.azure.com/dnceng/public/_build/results?buildId=1589294&view=logs&j=bb94f37e-a09b-5b51-09b2-ab78e7aa5103

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Feb 3, 2022

CoreCLR Pri0 Runtime Tests Run windows arm64 checked
Duration: 2h 41m 22s

That leg doesn't start immediately as it depends on both the coreclr and libraries product build. Add 20 minutes to it and you are over the 3 hour mark.

Another long pole is the llvmfullaot leg which in total also takes more than 3 hours.

@MattGal
Copy link
Member

MattGal commented Feb 3, 2022

CoreCLR Pri0 Runtime Tests Run windows arm64 checked
Duration: 2h 41m 22s

That leg doesn't start immediately as it depends on both the coreclr and libraries product build. Add 20 minutes to it and you are over the 3 hour mark.

Another long pole is the llvmfullaot leg which in total also takes more than 3 hours.

Just to pile on, since this thread keeps popping up and I had to hit unsubscribe, is that this is 4 days, 19 hours of machine time in Helix tests for the last build alone:

let JobIds = 
Jobs 
| where Source == "pr/public/dotnet/runtime/refs/pull/64670/merge" 
| extend props = parse_json(Properties)
| where props["BuildNumber"]=="20220203.14"
| project JobId;
WorkItems
| where JobId  in (JobIds)
| where Name != JobName
| extend runtime = Finished-Started
| summarize sum(runtime)

(gives 4.19:50:06.7330000)

@jkotas
Copy link
Member

jkotas commented Feb 4, 2022

CoreCLR Pri0 Runtime Tests Run windows arm64 checked
Duration: 2h 41m 22s

It looks like we have hit low Win Arm64 Helix availability. Can we tell what caused it?

In the past week, this job executed in 84 minutes on average. There are jobs running for longer than this job or consuming more machine hours that this job. For example, the Mono llvmfullaot Pri0 Runtime Tests Run Linux arm64 release tool 142 minutes on average last week.

this is 4 days, 19 hours of machine time in Helix tests for the last build alone:

Yes, we know that each CI run in dotnet/runtime consumes days worth of build machine time and days worth of test machine time.

The lowest hanging fruit to improve CI machine costs is to eliminate idle build machines waiting for Helix jobs to finish. Tracked by dotnet/dnceng#1213

@ghost ghost locked as resolved and limited conversation to collaborators Mar 6, 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.