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

Should test restore be part of build or build-tests? #29953

Closed
ericstj opened this issue Jun 19, 2019 · 11 comments · Fixed by #33242 or #33553
Closed

Should test restore be part of build or build-tests? #29953

ericstj opened this issue Jun 19, 2019 · 11 comments · Fixed by #33242 or #33553
Assignees
Labels
area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented Jun 19, 2019

With the test project restore work test projects need to be individually restored before building.

Today this was added to the overall build.cmd, but this is not a necessary step for build and lengthens the time it takes for a build to complete. On my machine an incremental build with no work to do, this is taking 2 minutes for a no-op restore of the test projects on my notebook.

Can we consider moving this to the build-tests phase? So long as an un-restored test project still errors correctly when trying to build without restoring it seems like a reasonable optimization.

Thoughts @stephentoub @safern @wtgodbe @ViktorHofer? Anyone else have numbers here?

@ViktorHofer
Copy link
Member

Why not just do a build -build instead?

@wtgodbe
Copy link
Member

wtgodbe commented Jun 19, 2019

I think I agree with @ericstj here, devs should not have to change their workflow to opt-out of this extra restore if it isn't always necessary

@ViktorHofer
Copy link
Member

Can we consider moving this to the build-tests phase?

The idea of the restore switch was to use it if you want to do an upfront restore because dependencies changed or to be able to work on the plane without internet access. Also we are using the restore task in our yml script as a separate step to clearly indicate if a restore failed. I'm not a fan of mixing those two worlds.

An incremental build without added dependencies should just use the -build switch.

@ericstj
Copy link
Member Author

ericstj commented Jun 19, 2019

Can others share no-op restore times so we understand if my notebook is an outlier or if everyone is seeing a +2-minute addition to builds? Essentially run a build -restore after a successful build.

@ViktorHofer
Copy link
Member

Just tried it locally again:

~20 seconds to glob all test projects (can we do anything about that without hard-coding all projects?)
30 seconds to restore all

@ViktorHofer
Copy link
Member

Ok one more run:

~25 seconds to glob all test projects
15 seconds to restore all

@ericstj
Copy link
Member Author

ericstj commented Jun 19, 2019

Is that really globbing or is it building a bunch of targets/evaluating the projects?

@ViktorHofer
Copy link
Member

ViktorHofer commented Jun 20, 2019

Can't say. The binlog didn't show that well. I thought about the whole restore concept a bit more and want to share my thoughts.

Ideally in the future we don't need to do the upfront restore at all. Most of the stuff that comes from Arcade is already an SDK, ie Arcade.Sdk, Configuration, CoreFxTesting. Other packages can probably be normal PackageReferences where they are needed. Additional work that needs to be done should be sequenced at the right places. When we build a project it would implicitly call into the restore target, exactly how it is today with dotnet build.

We would still need the upfront restore but just as an optional switch for a) CI to factor out restore from build and allow to fail early and b) for scenarios like before you board a plane.

I think we all agree that this is the right path forward. I can't easily estimate how much work this involves but a few known blockers are:

  • Enable project restore for ref/src/every other project.
  • Get rid of external/*

Thoughts?

@tannergooding
Copy link
Member

I am 110% in favor of making our build more like a normal build. This helps ensure that existing tooling "just works" and my guess is that will in turn help productivity.

I may also be an outlier, but first time restore (from a clean enlistment) takes 00:01:28.52 on my box. Subsequent restores take: 00:00:31.55.

@ViktorHofer
Copy link
Member

ViktorHofer commented Jun 23, 2019

I may also be an outlier, but first time restore (from a clean enlistment) takes 00:01:28.52 on my box. Subsequent restores take: 00:00:31.55.

On a clean enlistment, a restore which takes 1-2 minutes is not an outlier. It does several things in that phase:

  • Restore dotnet toolchain
  • Restore arcade sdk
  • Restore tools that are listed in Tools.props
  • Restore depproj files in external/*
  • Build depproj files in external/*
  • Restore test projects

Also not that there are two flavors of a clean enlistment:

  • same content as on ref/head/master, basically what you get when you do a git clean -xdf
  • semi clean, the .dotnet folder with the CLI is still there + other common folders like ".tools". You are usually in that state when you do a .\build -clean.

I am 110% in favor of making our build more like a normal build. This helps ensure that existing tooling "just works" and my guess is that will in turn help productivity.

Thanks for your opinion here. That's one of our main goals. Stuff like VS Test Explorer and common dotnet CLI commands like restore, build and test simply have to work. Because of some burden in our infrastructure this wasn't easily possible but we are not in a much better state and with some further refinements we will be there.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@safern
Copy link
Member

safern commented Mar 10, 2020

@ViktorHofer re-opening this as the PR that fixed it is being reverted.

@safern safern reopened this Mar 10, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner
Projects
None yet
7 participants