Adding missing Configurations.props for refs and fixing the existing ones by joperezr · Pull Request #15291 · dotnet/corefx

joperezr · 2017-01-18T22:25:19Z

cc: @ericstj @tarekgh @weshaggard
FYI: @chcosta

Fixing the few Configurations.props we have for refs to remove their claimed netstandard config.
Adding uap configuration to all refs.
Adding configuration file to refs that didn't have one already.
Fixed uap logic on targetGroups in order to select the right imports.

joperezr · 2017-01-18T22:38:50Z

@tarekgh With these changes, building src\ref.builds with /p:TargetGroup=netfx will skip all of the reference assemblies. build.cmd -framework:netfx will not work yet, because it still will try to build some configurations of src\src.builds and have a bunch of issues.

joperezr · 2017-01-18T22:39:45Z

@weshaggard let me know if these changes are more or less what you had in mind, and if so I'll go ahead and start working on adding the right configurations to the implementation projects.

tarekgh · 2017-01-18T22:48:00Z

LGTM

weshaggard · 2017-01-18T22:48:05Z

    <BuildConfigurations>
-      netstandard;
      netcoreapp;
+      uap;


With these we should also clean out the netcoreapp #ifdef's from the project and source files.

weshaggard · 2017-01-18T22:50:36Z

Yes this is along the lines of what I was expecting. You should also go through and clean out the #ifdefs.

@ericstj should we add a default define for each configuration so it is present for each build config automatically?

joperezr · 2017-01-19T00:28:41Z

You should also go through and clean out the #ifdefs.

Done with 904aa39

chcosta · 2017-01-19T00:37:31Z

      <TargetingPackDir Include="$(RefPath)" />
      <TargetingPackDir Condition="'$(IsNETCoreAppRef)' == 'true'" Include="$(BinDir)netcoreapp\pkg\ref" />
    </ItemGroup>
+    <ItemGroup Condition="'$(NuGetTargetMoniker)' == '' Or $(NuGetTargetMoniker.Contains('UAP'))">


Just to clarify, if '$(NuGetTargetMoniker)' == '', then the TargetingPackDir includes both $(RefPath) and $(RefRootPath)uap/?

RefPath should already be set to $(RefRootPath)uap so we shouldn't need to add this. Also this is going to cause issues for tests which blindly take *.dll from all the TargetingPackDir's

'$(NuGetTargetMoniker)' == '' part of the condition was actually copy/pasted by mistake, the only condition that should be there is the UAP one. @weshaggard RefPath is not set to UAP since in dir.props it gets initialized to netcoreapp, so perhaps the change has to go there instead. I'll remove this small change on the file and try to figure out a better way to calculate this.

Ah yes I forget we hardcoded that. We do need to move to a place where we compute that variable based on the BuildConfiguration.

BuildConfiguration or Configuration? I'd imagine that it should be based off of Configuration, in which case it can just be a derived property in the data model.

No this one should be based on BuildConfiguration, as we want to consistently create the targeting pack folders per BuildConfiguration. Similar to the runtime folders.

I think that's broken. Consider I build a project with only a netstandard configuration for both BuildConfiguration netcoreapp and uap: separate vertical builds on same machine. It will reuse the same intermediate/bin directories (per design) but with different refs we'll potentially have different assemblies.

I'm not sure I follow. It will be the same assembly just loaded from a different ref directory. The entire idea is that we want an easy reference resolution from flat targeting pack for each vertical BuildConfiguration and not have to try and add a bunch of logic to pick the correct reference for every library. We aren't in the contract store or nuget resolution world when we build a vertical. To resolve a Reference you are only looking in one targeting pack directory.

ericstj · 2017-01-19T01:10:55Z

+  <PropertyGroup>
+    <BuildConfigurations>
+      netcoreapp;
+      uap;


I don't think this one is supported on UAP.

I believe it is, by looking at some pre-dev/eng packages it seems to have a netstandard1.3 reference assembly.

ericstj · 2017-01-19T01:11:11Z

+  <PropertyGroup>
+    <BuildConfigurations>
+      netcoreapp;
+      uap;


Supported on UAP?

What do you think our best way to figure out uap support?

Look at existing pkgproj, or package reports from pre-dev/eng build.

ericstj · 2017-01-19T01:14:37Z

    <BuildConfigurations>
      netcoreapp;
-      netstandard1.0;
-      <!-- portable_net40+sl4+win8+wp8-Windows_NT; Looks like our filtering mechanism doesn't understand this targetgroup. Do we even need it? -->


Yes we need it. Please add to data model.

ericstj · 2017-01-19T01:14:40Z

  <PropertyGroup>
    <BuildConfigurations>
      netcoreapp;
-      netstandard1.0;


We need to preserve the older configs of this one. It'll ship a package.

... And on desktop I'd expect it to build the netstandard1.0 config.

ericstj · 2017-01-19T01:15:00Z

    <TargetingPackReference Include="System.Core" />
  </ItemGroup>
-  <ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp'">
+  <ItemGroup>


Don't remove this, update the condition.

ericstj · 2017-01-19T01:16:30Z

+      <Imports>netcore50aot</Imports>
+      <CompatibleWith>netstandard2.0</CompatibleWith>
+    </TargetGroups>
+    <TargetGroups Include="uap">


Should we also add a uapaot?

Probably and we are going to need some corert variants as well.

…ones

…eedback

joperezr · 2017-01-19T22:00:30Z

@weshaggard @ericstj I've addressed the feedback and removed the uap configurations for the projects where we didn't support uwp previously. Also, I've added Eric's new logic for calculating RefPath based on the original TargetGroup

…for uap

weshaggard · 2017-01-20T16:44:44Z

    <BuildConfigurations>
-      netcoreapp;
-      uap;
+      ;


Is this one intentionally empty?

yes. We haven't really added anything to these DirectoryServices assemblies yet, so it makes no sense to have them building on any framework. Plus, the code on the implementation side that we do have doesn't compile yet, so build is disabled there as well.

…ones (dotnet/corefx#15291) Adding missing Configurations.props for refs and fixing the existing ones Commit migrated from dotnet/corefx@2847e0e

weshaggard reviewed Jan 18, 2017

View reviewed changes

chcosta reviewed Jan 19, 2017

View reviewed changes

ericstj reviewed Jan 19, 2017

View reviewed changes

karelz added area-Infrastructure-libraries dev-eng labels Jan 19, 2017

karelz assigned joperezr Jan 19, 2017

weshaggard mentioned this pull request Jan 19, 2017

Add ConcurrentBag/Queue.Clear #15292

Merged

joperezr added 4 commits January 19, 2017 13:49

Adding missing Configurations.props for refs and fixing the existing …

d47f9c3

…ones

Removing #ifdefs from reference assemblies

e7c51bf

Remove uap configurations for libraries that don't support uap + PR f…

47b58e6

…eedback

Better calculation for RefPath

1c4b28c

joperezr force-pushed the AddMissingConfigsForRefs branch from 904aa39 to 1c4b28c Compare January 19, 2017 21:58

Updating Configurations.props on refs once more to get the right set …

f4b39ca

…for uap

joperezr merged commit 2847e0e into dotnet:master Jan 20, 2017

joperezr deleted the AddMissingConfigsForRefs branch January 20, 2017 00:38

weshaggard reviewed Jan 20, 2017

View reviewed changes

karelz modified the milestone: 2.0.0 Jan 21, 2017

justinvp mentioned this pull request Jan 25, 2017

Expose String.Trim overloads that take a single char #15334

Merged

Conversation

joperezr commented Jan 18, 2017

Uh oh!

joperezr commented Jan 18, 2017

Uh oh!

joperezr commented Jan 18, 2017

Uh oh!

tarekgh commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard commented Jan 18, 2017

Uh oh!

joperezr commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj Jan 19, 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!

weshaggard Jan 19, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj Jan 19, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ericstj Jan 19, 2017 •

edited

Loading

weshaggard Jan 19, 2017 •

edited

Loading

ericstj Jan 19, 2017 •

edited

Loading