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.

Sign up for GitHub

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

Jump to bottom

Consume projections via nupkg vs. checked-in binaries #2733

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

Consume projections via nupkg vs. checked-in binaries #2733

wants to merge 17 commits into from

Conversation

msft-Jeyaram
Copy link
Contributor

@msft-Jeyaram msft-Jeyaram commented May 26, 2017 •
edited by rajsesh
Loading

This will establish the WinObjC.Frameworks.UWP/MSAds packages and will be consumed by projects via the nupkg

  • Removing the pre-checked in binaries, headers
  • Removing the pre-checked in binary package project and removing it from the main build

Fixes #2688, #2687

This change is Reviewable

@msft-Jeyaram
Copy link
Contributor Author

msft-Jeyaram commented May 26, 2017 •
edited
Loading

There is two instance of WINOBJC_SDK\ usage, that is being cleaned up now :)
#ByDesign

@msft-Jeyaram
Copy link
Contributor Author

msft-Jeyaram commented May 26, 2017 •
edited
Loading

@rajsesh-msft the projection code is generated after applying the fixes to winmd2objc (my other CRs) #ByDesign

rajsesh
rajsesh suggested changes May 26, 2017
View reviewed changes
Copy link
Contributor

@rajsesh rajsesh left a comment

Choose a reason for hiding this comment

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

Per offline discussion, let us not check-in the generated code. Since this doesn't need to be built every day, we could just wrap the generation step in a script or a build task. This also doesn't have to be nightly built either.

DHowett-MSFT
DHowett-MSFT reviewed Jun 1, 2017
View reviewed changes
msvc/sdk-build.props Outdated
@@ -24,6 +24,10 @@
</ProjectReference>
</ItemGroup>

<ItemGroup>
<PackageReference Condition="'$(BuildingFrameworksCore)' != 'true'" Include="WinObjC.Frameworks.UWP" Version="*" />
Copy link

Choose a reason for hiding this comment

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

This is an interesting approach!

Do you think it should go down with the other PackageReferences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, we want to avoid Frameworks.Core from consuming this package.

Copy link

Choose a reason for hiding this comment

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

That doesn't mean it can't go in the same block of package references... that just means we have to keep the Condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I assumed you meant to just include it without the condition.
Consider it moved.

build/Tests/UnitTests/Starboard/Starboard.UnitTests.vcxproj Outdated
@@ -223,6 +223,9 @@
<ItemGroup>
<ClInclude Include="..\..\..\..\tests\unittests\Starboard\LifetimeCounting.h" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="WinObjC.Frameworks.UWP" Version="*" />
Copy link

Choose a reason for hiding this comment

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

Indentation is odd here!

@msft-Jeyaram
Removal of projection binaries
8f0868a
@msft-Jeyaram
Remove [Projections] Universal Windows UWP headers
24dda75
@msft-Jeyaram
Remove [Projection] Microsoft Advertising headers
c822b64
@msft-Jeyaram
Initial commit of Frameworks.UWP/MSAds projections
ae074c7
@msft-Jeyaram
remove WinObjc.Frameworks.UWP project from the main build
ace0789
@msft-Jeyaram
Remove the old prebuilts packing WinObjC.Frameworks.UWP project
b42f6ab
@msft-Jeyaram
remove the reference to the old UWP.nuproj that packaged the pre-builts
7e82b8c
@msft-Jeyaram
remove the guid for pre-packed projections project
faa7ebb
@msft-Jeyaram
Remove the projections code, this will be automatically generated'
e4018f1
@msft-Jeyaram
Add project reference for UWP packages
0324a42 
old method was not using the local package, if we can't find a package locally, we look it up in mscodehub
@msft-Jeyaram
Let nuget look at locally built packages in build\outputpackages
c2b97b6
@msft-Jeyaram
Support the exclusion of referencing Frameworks.UWP for Frameworks.Co…
d7b2d73 
…re projects
@msft-Jeyaram
Ignore warnings from UWP headers
8c660f9
@msft-Jeyaram
Make starboard unit tests automatically resolve Frameworks.UWP package
1e05951
@msft-Jeyaram
ensure functional tests reference and resolve nuget package directly
d5aae69
@msft-Jeyaram
make addressbook test reference Frameworks.UWP package
3fa0f34
@msft-Jeyaram
move package reference together
4e5e83b
@msft-Jeyaram
Copy link
Contributor Author

This saw random failures with NotificationQueue and filemanager tests.
Now it second run and local run seem to be fine

@msft-Jeyaram
Copy link
Contributor Author

msft-Jeyaram commented Jun 2, 2017 •
edited
Loading

ping! @rajsesh-msft @DHowett-MSFT

DHowett-MSFT
DHowett-MSFT approved these changes Jun 2, 2017
View reviewed changes
Copy link

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

Moderately apprehensive about using BuildingFrameworksCore to turn off the package reference instead of inserting the package reference in every project that consumes UWP.

Also, don't our internal frameworks avoid Projections now? They should be generally free of it, and we might not want to even ALLOW them to link against it so we don't accidentally reintroduce dependencies.

@MSFTFox MSFTFox mentioned this pull request Feb 8, 2018
Open
@DHowett-MSFT DHowett-MSFT changed the title Implement steps to consume projections via nupkg vs pre-checked in binaries Consume projections via nupkg vs. checked-in binaries Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajsesh rajsesh rajsesh approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@msft-Jeyaram @DHowett-MSFT @rajsesh @msftclas @msftgits @MSFTFox