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

Multitarget extensions packages to remove dependencies for source build #2090

Merged
merged 9 commits into from
Aug 7, 2019

Conversation

JunTaoLuo
Copy link

Addresses #2056

cc @crummel @dseefeld

@JunTaoLuo
Copy link
Author

JunTaoLuo commented Jul 27, 2019

FYI not ready for review, FileProviders.Embedded has msbuild props/targets that need to be added for netcoreapp3.0 actually it should still work, I'll test it to be sure.

@JunTaoLuo JunTaoLuo changed the base branch from master to release/3.0 August 6, 2019 00:44
@JunTaoLuo
Copy link
Author

@anurse who would be a good candidate to review this? The main change is to multitarget some of the packages built for source build with netcoreapp3.0 and remove dependencies. However there are a few areas that needed some changes:

  1. The packaging of FilesProviders.Embedded had to be changed since the TFMs we target is conditional
  2. For the projects where we are adding the netcoreapp3.0 tfm, I also made sure the tests test both the netcoreapp3.0 and netstandard 2.0 binaries. Luckily most tests already test on netcoreapp3.0 and net472 so the coverage is mostly sufficient. However, I guess we might also want to test on netcoreapp2.2 to ensure we are testing netstandard2.0 binaries in Core. Do you think that would be useful?
  3. I had to update the test logic in Config.UserSecrets to accommodate more tfm for test coverage.

@JunTaoLuo
Copy link
Author

JunTaoLuo commented Aug 6, 2019

cc @pranavkm @ajaybhargavb to take a look at FileProviders changes
cc @HaoK for Config changes
cc @BrennanConroy for Logging changes
cc @Tratcher for Hosting changes
cc @rynowak @anurse for everything

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

:shipit: for Hosting

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

:shipit: for logging

Copy link
Member

@HaoK HaoK left a comment

Choose a reason for hiding this comment

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

Config looks good assuming the net472 change to user secrets is expected

@@ -1,10 +1,14 @@
<!-- This file is automatically generated. -->
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<TargetFrameworks>netstandard2.0;netcoreapp3.0</TargetFrameworks>
Copy link

Choose a reason for hiding this comment

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

Spoke to @JunTaoLuo offline. We need to make sure this TFM works when we're source building netcoreapp3.1. I'd personally prefer not to cross-compile 3 different TFMs just for source build.

Copy link
Author

Choose a reason for hiding this comment

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

I spoke with @crummel, turns out they are tracking it in dotnet/source-build#759. Which is currently triaged for 3.1.

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

I suspect this is fine but please fill in the details about ref/ projects

@JunTaoLuo
Copy link
Author

I don't see an appropriate label but this has been approved by shiproom.

@JunTaoLuo JunTaoLuo merged commit 3ec8c35 into release/3.0 Aug 7, 2019
@ghost ghost deleted the johluo/multitarget branch August 7, 2019 20:26
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request Feb 12, 2020
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request Feb 15, 2020
maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Feb 28, 2020
maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 2, 2020
maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 11, 2020
maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 11, 2020
maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 27, 2020
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request Nov 17, 2020
@ghost ghost locked as resolved and limited conversation to collaborators May 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants