Skip to content

Convert ProjectTemplates to build using ProjectReferences #6935

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
Merged
merged 2 commits into from
Jan 23, 2019
Merged

Convert ProjectTemplates to build using ProjectReferences #6935

merged 2 commits into from
Jan 23, 2019

Conversation

natemcmaster
Copy link
Contributor

Resolves #4246

Changes:

@Tratcher Tratcher removed their request for review January 22, 2019 21:42
@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 23, 2019
@natemcmaster natemcmaster requested a review from javiercn January 23, 2019 17:43
javiercn
javiercn approved these changes Jan 23, 2019
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

To the degree to which I can attest them, the changes look good to me. So long as you can build a working template from .\build.cmd /t:Pack we should be good. (Having the E2E tests run would be good too).

@natemcmaster
Copy link
Contributor Author

The new command you'll need to run is src/ProjectTemplates/build.cmd -pack if you want to product packages for just the templates. I tested on my own and it should still work. The tricky piece is that the templates will expect packages like Mvc.NewtonsoftJson 3.0.0-preview-t000 to exist. I recommend running $root/build.cmd -pack -all to produce all packages the repo supports.

@natemcmaster
Copy link
Contributor Author

I'm going to wait on merging this until after we've stabilized Preview 2.

@natemcmaster
Copy link
Contributor Author

Nevermind. Just learned that we have respin the build 2 or 3 more times to consume partner dependency updates. PR checks are all green, so I'm going to merge this. I'll monitor CI to make sure this doesn't break the build.

@natemcmaster natemcmaster merged commit 33bc457 into dotnet:master Jan 23, 2019
@natemcmaster natemcmaster deleted the temp-ref branch January 23, 2019 23:54
@natemcmaster natemcmaster mentioned this pull request Jan 24, 2019
Closed
ryanbrandenburg
ryanbrandenburg reviewed Jan 24, 2019
View reviewed changes
Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

I had a couple minor items we might want to file followups for depending, but otherwise this looks awesome.

build/buildorder.props
@@ -7,6 +7,5 @@
</ItemDefinitionGroup>

<ItemGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just murder this file now or should we keep it in case we have a temporary need for buildorder building?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can murder this now, along with a lot of other code. See #6995

build/repo.props
@@ -125,6 +127,10 @@
@(ProjectToBuild);
@(ProjectToExclude);" />

<!--
Use caution to avoid deep recursion. If the globbing pattern picks up something which exceeds MAX_PATH,
the entire pattern will silently fail to evaluate correctly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can make it fail loudly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MSBuild will fail to expand the glob, which produces an error like "Cannot find project C:\src\aspnet\AspNetCore\src\Something*.csproj" when we try to invoke build and restore targets.

build/submodules.props
@@ -36,6 +36,5 @@
</PropertyGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume since TestsRequiredTheSharedRuntime is still above that we didn't solve that problem in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise should we consider this file for deletion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4321 is still unresolved. I have one change in the works that addresses part of the problem, but it's a tricky enough challenge that I didn't tackle it completely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanbrandenburg ryanbrandenburg ryanbrandenburg left review comments

@muratg muratg muratg approved these changes

@javiercn javiercn javiercn approved these changes

@mkArtakMSFT mkArtakMSFT Awaiting requested review from mkArtakMSFT

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@natemcmaster @muratg @javiercn @ryanbrandenburg @Eilon