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

Migrate GenerateFileFromTemplate to Microsoft.DotNet.Build.Tasks.Templating #7403

Merged
merged 11 commits into from
Jun 9, 2021
Merged

Migrate GenerateFileFromTemplate to Microsoft.DotNet.Build.Tasks.Templating #7403

merged 11 commits into from
Jun 9, 2021

Conversation

JunTaoLuo
Copy link

Fixes #204. This will need to be backported to 3.1 and 5.0 after approval

dougbu
dougbu approved these changes May 18, 2021
View reviewed changes
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.

My comments are all nits but I would like to hear from @alexperovich and / or @markwilkie before we merge.

src/Microsoft.DotNet.Build.Tasks.Templating/GenerateFileFromTemplate.cs Outdated Show resolved Hide resolved
src/Microsoft.DotNet.Build.Tasks.Templating/GenerateFileFromTemplate.cs Outdated Show resolved Hide resolved
src/Microsoft.DotNet.Build.Tasks.Templating/GenerateFileFromTemplate.cs Outdated Show resolved Hide resolved
src/Microsoft.DotNet.Build.Tasks.Templating/GenerateFileFromTemplate.cs Outdated Show resolved Hide resolved
src/Microsoft.DotNet.Build.Tasks.Templating/GenerateFileFromTemplate.cs Outdated Show resolved Hide resolved
src/Microsoft.DotNet.Build.Tasks.Templating/MSBuildListSplitter.cs Outdated Show resolved Hide resolved
@dougbu dougbu requested a review from alexperovich May 18, 2021 19:11
@dougbu
Copy link
Member

dougbu commented May 18, 2021

/fyi @rainersigwald (re dotnet/msbuild#4018) @ericstj (re #204) @ilyas1974 and @ViktorHofer (re tail end of #135)

@dougbu
Copy link
Member

dougbu commented May 18, 2021

@rainersigwald this migrationf from aspnet/BuildTools reminds me of a rather unrelated question: Why is the <JoinItems> task in the .NET SDK and not together with the more general msbuild tasks and targets❔ I'm reminded of the discrepancy when I reach for Remove with MatchOnMetadata (a poorly-documented and limited option) or difficult-to-grok task batching alternatives in projects that don't use that SDK.

@rainersigwald
Copy link
Member

Why is the <JoinItems> task in the .NET SDK and not together with the more general msbuild tasks and targets❔

It was needed there first. I am not opposed in principle to "promoting" it.

@ViktorHofer
Copy link
Member

Great to see progress on this 👍

Does one task justify an additional assembly and with it package? Why not add it to the Arcade.Sdk project instead?

@ViktorHofer
Copy link
Member

That's the implementation that we use in dotnet/runtime: https://github.com/dotnet/runtime/blob/main/src/tasks/installer.tasks/GenerateRunScript.cs which looks simpler. Wouldn't that suffice?

@JunTaoLuo
Copy link
Author

@ViktorHofer it was mentioned that this task shouldn't be included in the SDK: #204 (comment) so we decided to create a task package instead. As for comparison between GenerateRunScript.cs, this task is more general and can be used to substitute a user specified set of properties instead of just the specific [[RunCommands]] and [[EchoRunCommands]]

ViktorHofer
ViktorHofer approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

LGTM. What about tests? :)

@JunTaoLuo
Copy link
Author

There were no tests in the original implementation, maybe we should not block on that in this PR and file followup to add tests?

@ViktorHofer
Copy link
Member

There were no tests in the original implementation, maybe we should not block on that in this PR and file followup to add tests?

Sure, works for me.

@lukas-lansky lukas-lansky mentioned this pull request May 19, 2021
Merged
John Luo and others added 8 commits May 29, 2021 00:08
Migrate GenerateTemplateFromFile to Microsoft.DotNet.Build.Tasks.Temp…
4c2efe4 
…lating
@ViktorHofer
Update src/Microsoft.DotNet.Build.Tasks.Templating/build/Microsoft.Do…
fd680db 
…tNet.Build.Tasks.Templating.props

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@dougbu @rainersigwald @ViktorHofer
Apply suggestions from code review
97ffb60 
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Update GenerateFileFromTemplate.cs
7d4af53
Update MSBuildListSplitter.cs
8d5d7ee
Update Microsoft.DotNet.Build.Tasks.Templating.csproj
f911aae
Import
3c0c88d
Update style and address feedback
ceaa936
@JunTaoLuo
Copy link
Author

Hey all, I've addressed the style guidelines and other feedback.

dougbu
dougbu reviewed May 29, 2021
View reviewed changes
src/Microsoft.DotNet.Build.Tasks.Templating/MSBuildListSplitter.cs Outdated Show resolved Hide resolved
src/Microsoft.DotNet.Build.Tasks.Templating/Microsoft.DotNet.Build.Tasks.Templating.csproj Outdated
@@ -0,0 +1,29 @@
<!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. -->
Copy link
Member

Choose a reason for hiding this comment

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

/fyi @markwilkie four projects in this repo use a different header:

<!-- Copyright (c)  Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information. -->

They should probably be made consistent.

@dougbu
Copy link
Member

dougbu commented Jun 3, 2021

This PR has been reviewed though I don't see any :shipit:s from Core-Eng folks. Unless I hear different before later tonight, I'll merge. Please sing out w/ objections if you have any.

@markwilkie
Copy link
Member

While I have no context on the code itself, the intent is good with me. Assuming the right testing has happened, I'm find w/ merging.

ChadNedzlek
ChadNedzlek requested changes Jun 3, 2021
View reviewed changes
src/Microsoft.DotNet.Build.Tasks.Templating/GenerateFileFromTemplate.cs Outdated
/// The destination for the generated file.
/// </summary>
[Required]
[Output]
Copy link
Member

Choose a reason for hiding this comment

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

This "Output" is not correct, it's an input parameter. It's not returned by this task.

Copy link
Author

Choose a reason for hiding this comment

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

Looks like we use it as output here: https://github.com/dotnet/aspnetcore/blob/52eff90fbcfca39b7eb58baad597df6a99a542b0/src/ProjectTemplates/GenerateContent.targets#L45-L52.

Copy link
Author

Choose a reason for hiding this comment

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

Though I think it may be more correct to set the value to the value escaped by https://github.com/aspnet/BuildTools/blame/main/src/Internal.AspNetCore.BuildTasks/GenerateFileFromTemplate.cs#L55.

We could probably also separate it out into a separate out parameter like ResolvedOutputPath or EscapedOutputPath but so far we haven't run into any issues in our builds due to this being an Output parameter so I would prefer if we don't block on this change and file followup issues to address.

Copy link
Member

Choose a reason for hiding this comment

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

This path is never set by this code. Marking it output is incorrect, since it doesn't actually output anything. We should not be checking in new, incorrect, code.

@dougbu dougbu mentioned this pull request Jun 4, 2021
Closed
@JunTaoLuo JunTaoLuo changed the title Migrate GenerateTemplateFromFile to Microsoft.DotNet.Build.Tasks.Templating Migrate GenerateFileFromTemplate to Microsoft.DotNet.Build.Tasks.Templating Jun 8, 2021
@JunTaoLuo JunTaoLuo merged commit e81d8c9 into main Jun 9, 2021
@JunTaoLuo JunTaoLuo deleted the johluo/template branch June 9, 2021 20:49
@ViktorHofer
Copy link
Member

🎉🎉🎉

@ViktorHofer ViktorHofer mentioned this pull request Jun 9, 2021
Open
JunTaoLuo pushed a commit that referenced this pull request Jun 10, 2021
@ViktorHofer @dougbu @rainersigwald
Migrate GenerateFileFromTemplate to Microsoft.DotNet.Build.Tasks.Temp…
941a0bb 
…lating (#7403)

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
JunTaoLuo pushed a commit that referenced this pull request Jun 10, 2021
@ViktorHofer @dougbu @rainersigwald
Migrate GenerateFileFromTemplate to Microsoft.DotNet.Build.Tasks.Temp…
fe9fc3f 
…lating (#7403)

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
This was referenced Jun 10, 2021
Merged
Merged
Merged
markwilkie pushed a commit that referenced this pull request Oct 5, 2021
@ViktorHofer @dougbu @rainersigwald
[3.x] Migrate GenerateFileFromTemplate to Microsoft.DotNet.Build.Task…
62ba17e 
…s.Templating (#7503)

* Migrate GenerateFileFromTemplate to Microsoft.DotNet.Build.Tasks.Templating (#7403)

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>

* Cherry-pick fixes

* TFM update

* Fix syntax for older SDKs

* More syntax fixes

Co-authored-by: John Luo <johluo@microsoft.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
markwilkie pushed a commit that referenced this pull request Oct 5, 2021
@ViktorHofer @dougbu @rainersigwald
[5.0] Migrate GenerateFileFromTemplate to Microsoft.DotNet.Build.Task…
9661b51 
…s.Templating (#7502)

* Migrate GenerateFileFromTemplate to Microsoft.DotNet.Build.Tasks.Templating (#7403)

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>

* 5.0 fixes

* TFM update

Co-authored-by: John Luo <johluo@microsoft.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
@danmoseley danmoseley mentioned this pull request Feb 7, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukas-lansky lukas-lansky lukas-lansky left review comments

@dougbu dougbu dougbu approved these changes

@rainersigwald rainersigwald rainersigwald approved these changes

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@ChadNedzlek ChadNedzlek ChadNedzlek approved these changes

@markwilkie markwilkie Awaiting requested review from markwilkie

@alexperovich alexperovich Awaiting requested review from alexperovich

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

Successfully merging this pull request may close these issues.

Source file pre-processing
8 participants
@JunTaoLuo @dougbu @rainersigwald @ViktorHofer @markwilkie @lukas-lansky @alexperovich @ChadNedzlek