Force load active configured projects by davkean · Pull Request #3032 · dotnet/project-system

davkean · 2017-12-08T06:09:14Z

(this depends on and includes #3020, just look at the last commit)

CPS always force loads the active configuration during project initialization and when the active configuration changes. This makes sure that any ConfiguredProjectAutoLoad services are created and run.

This is the equivalent, except for all configurations that we consider "active". For example in the following multi-targeted project:

 -> All known project configurations:

      Configuration Platform    TargetFramework
      -------------------------------------------
              Debug |   AnyCPU  |           net45
              Debug |   AnyCPU  |           net46
            Release |   AnyCPU  |           net45
            Release |   AnyCPU  |           net46

  -> Active solution configuration:

              Debug |   AnyCPU

These configurations are explicitly loaded:

              Debug |   AnyCPU  |           net45
              Debug |   AnyCPU  |           net46

When active solution configuration changes to:

            Release |   AnyCPU

These configurations are explicitly loaded:

            Release |   AnyCPU  |           net45
            Release |   AnyCPU  |           net46

Previously, these configurations were implicitly loaded when an auto-load component (typically language service) happened to call through IActiveConfiguredProjectsProvider. This makes loading of configurations deliberate and explicit, and always occur regardless of the auto-load component that is run.

This basically moves loading these configs from implicit model when a random component calls IActiveConfiguredProjectsProvider.GetActiveConfiguredProjectsAsync (which is being replaced) to a more explicit and predictable model.

davkean · 2017-12-08T06:10:19Z

tag @lifengl as per discussed today.

Tests are coming.

davkean · 2017-12-08T06:12:56Z

@lifengl This looks like the right time to do this - just after we've flowed the configured project capabilities -> unconfigured project. I could go this or AfterLoadInitialConfiguration

Pilchie · 2017-12-13T01:17:51Z

Note that @lifengl is on vacation right now.

This change looks okay to me, but waiting for tests.

davkean · 2017-12-13T22:46:58Z

Tests are coming, it took me about 2 hours to figure out how to Moq dataflow.

CPS force loads the active configuration during project initialization and when the active configuration changes. This ensures that any ConfiguredProjectAutoLoad services are created and run. This is the equivalent, except for all configurations that we consider "active". In the following multi-targeted project: -> All known project configurations: Configuration Platform TargetFramework ------------------------------------------- Debug | AnyCPU | net45 Debug | AnyCPU | net46 Release | AnyCPU | net45 Release | AnyCPU | net46 -> Active solution configuration: Debug | AnyCPU | net45 These configurations are explicitly loaded: Debug | AnyCPU | net45 Debug | AnyCPU | net46 Previously, these configurations were implicitly loaded when an auto-load component (typically language service) happend to call through IActiveConfiguredProjectsProvider. This makes loading of configurations deliberate and explicit, and always occur regardless of auto-load component that is run.

We don't need to Dispose on a background thread as we don't hold any locks in Dispose.

davkean · 2017-12-14T03:08:57Z

@Pilchie Pushed tests. Note I've slightly changed the implementation to make it test consumable (TargetBlock needs to be public so that I can await on Completion) and I now await all Loads so that any exceptions don't get lost, fault the block and NFW.

lifengl · 2017-12-14T04:12:51Z

Looks good, two questions: When a new configuration is added, this code won’t run to see whether to load it. Will that be handled by some other code? Should we protect or stop loading configured projects when the project is unloaded in the middle? Or is it possible that a configuration it planned to load was removed in another thread?

…

Sent from my phone On Dec 14, 2017, at 4:09 PM, David Kean <notifications@github.com<mailto:notifications@github.com>> wrote: @Pilchie<https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpilchie&data=02%7C01%7C%7Cfaf7671caf5a4a56bd1608d542a00719%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636488177432708516&sdata=sDi8eFxdg9eTm%2F6OtorpEKSPFEC%2FoKoa1NRIBdbueJA%3D&reserved=0> Pushed tests. Note I've slightly changed the implementation to make it test consumable (TargetBlock needs to be public so that I can await on Completion) and I now await all Loads so that any exceptions don't get lost, fault the block and NFW. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fproject-system%2Fpull%2F3032%23issuecomment-351595602&data=02%7C01%7C%7Cfaf7671caf5a4a56bd1608d542a00719%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636488177432708516&sdata=anmTTN0t1SSKW%2FS8u7ylh0J%2Fo8Y1gOEEk2BDk3mX7Hw%3D&reserved=0>, or mute the thread<https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FALGWwvAcFWaRGODiVuW8kbqdlSZnVx_5ks5tAJFMgaJpZM4Q6o_Y&data=02%7C01%7C%7Cfaf7671caf5a4a56bd1608d542a00719%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636488177432708516&sdata=U2RQh%2F%2BlBxvKgJCe0EEBvSf7cxwViI1WMd0N1cSxr1E%3D&reserved=0>.

Pilchie · 2017-12-14T04:14:35Z

+        }
+
+        public ITargetBlock<IProjectVersionedValue<IConfigurationGroup<ProjectConfiguration>>> TargetBlock
+        {


not => _targetBlock;

Pilchie · 2017-12-14T04:15:50Z

+            foreach (ProjectConfiguration configuration in e.Value)
+            {
+                await _project.LoadConfiguredProjectAsync(configuration)
+                              .ConfigureAwait(false);


What is the threading here? Worth fanning out and doing a Task.WhenAll() instead?

Yeah, after submitted this - had a similar thought, but wondered about competition for the UI thread. @lifengl Thoughts?

davkean · 2017-12-14T04:31:12Z

@lifengl

When a new configuration is added, this code won’t run to see whether to load it. Will that be handled by some other code?

When a new config is added, and it is considered active - I would expect to get callback here. I only want to load active configs, nothing else.

Should we protect or stop loading configured projects when the project is unloaded in the middle?

Yeah, that's a good concern.

Or is it possible that a configuration it planned to load was removed in another thread?

Hmm, let me play around with what happens in that case.

jmarolf · 2017-12-14T06:06:25Z

+            var configurationGroups = IConfigurationGroupFactory.CreateFromConfigurationNames(configurationNames);
+
+            var results = new List<string>();
+            var project = UnconfiguredProjectFactory.ImplementLoadConfiguredProjectAsync(configuration =>  {


I believe we want the { on its own line

jviau · 2017-12-14T22:30:18Z

+        [AppliesTo(ProjectCapability.CSharpOrVisualBasicOrFSharpLanguageService)]
+        public Task InitializeAsync()
+        {
+            Initialize();


Should probably use EnsureInitialized()

Nice catch.

- Stop loading configs while unloading - Stop unload from occuring while in the middle of loading a config

davkean · 2017-12-15T06:31:57Z

Updated the PR to address comments.

Now stop loading configs if the project starts unloading and prevent unloading while loading a config.
Fixed the double-initialization
Few style things

A couple of things;

Don't understand this "When a new configuration is added, this code won’t run to see whether to load it. Will that be handled by some other code?" I suspect you are referring to the fact that we don't add/remove configs when the project is loaded? If so, yes - we need to make sure CPS has the right call backs to enable that. I'm going to open a new bug for that, post 15.6.
Don't overlap loading of configs. Wouldn't mind Lifeng to chime in with thoughts around this.
Don't handle when a configuration is removed. I suspect here long term we should unload unused/remove configs (that would clean up the novariables as well).

dnfclas added the cla-already-signed label Dec 8, 2017

davkean changed the title ~~Force load configured projects~~ Force load active configured projects Dec 8, 2017

davkean commented Dec 8, 2017

View reviewed changes

davkean requested a review from a team December 8, 2017 06:13

dnfadmin removed the cla-already-signed label Dec 13, 2017

davkean added 6 commits December 14, 2017 11:19

Rename to ActiveConfiguredProjectsLoader

6234626

Await the result of loading the projects

80d16f0

Turn on synchronous disposable

9aa1561

We don't need to Dispose on a background thread as we don't hold any locks in Dispose.

Complete target block in dispose.

d1bc791

Add tests for ActiveConfiguredProjectsLoader

4faca5e

davkean force-pushed the ForceConfiguredProjects branch from dbfad6e to 4faca5e Compare December 14, 2017 03:05

Pilchie approved these changes Dec 14, 2017

View reviewed changes

jmarolf reviewed Dec 14, 2017

View reviewed changes

jviau reviewed Dec 14, 2017

View reviewed changes

davkean added 4 commits December 15, 2017 09:56

Prevent double-initialization

b208851

Brace on new line

7b1a6ed

Don't attempt load configs while unloading

cee19e2

- Stop loading configs while unloading - Stop unload from occuring while in the middle of loading a config

Property -> Expression body

a748bc0

davkean merged commit b65f6af into dotnet:master Dec 18, 2017

davkean deleted the ForceConfiguredProjects branch December 18, 2017 03:28

davkean mentioned this pull request Dec 18, 2017

Editor doesn't handle conditional compilation symbol with netstandard or .net core #2733

Closed

Conversation

davkean commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davkean commented Dec 8, 2017

Uh oh!

davkean Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pilchie commented Dec 13, 2017

Uh oh!

davkean commented Dec 13, 2017

Uh oh!

davkean commented Dec 14, 2017

Uh oh!

lifengl commented Dec 14, 2017 via email

Uh oh!

Pilchie Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

Pilchie Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

davkean Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

davkean commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarolf Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

jviau Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

davkean Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

davkean commented Dec 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

davkean commented Dec 8, 2017 •

edited

Loading

davkean Dec 8, 2017 •

edited

Loading

davkean commented Dec 14, 2017 •

edited

Loading