Introduce MSBuild tasks programability #12364

hishamco · 2022-09-10T01:31:50Z

github-actions · 2024-01-29T13:27:58Z

This pull request has merge conflicts. Please resolve those before requesting a review.

Piedone · 2024-03-24T21:24:09Z

Is this something you'd like to revisit any time soon @hishamco or should we close?

hishamco · 2024-03-24T21:28:46Z

I tried this PR last week but there's an issue to discover the task, I will try one more time if all goes well I will update the PR otherwise I will close it

hishamco · 2024-04-14T01:07:37Z

I'm wondering why the assembly file path doesn't resolved properly @MikeAlhayek @Piedone any idea?

hishamco · 2024-04-14T01:50:30Z

Finally, seems I forgot to generalize the build configuration instead of using Debug :) time to finalize the PR

...re/OrchardCore.Application.Cms.Core.Targets/OrchardCore.Application.Cms.Core.Targets.targets

hishamco · 2024-04-14T03:48:32Z

Can we wright unit tests? Why not!!

hishamco · 2024-04-14T05:03:25Z

It's good to demo it this week

Piedone · 2024-04-14T23:39:44Z

Before I deep dive into reviewing this, please answer me here: #4991 (comment) I'd like to first understand what your goal is, and what issue you're trying to solve, and why with the implementation you choose and not the alternatives I asked about. I'm wholly unclear about all of these.

hishamco · 2024-04-14T23:52:48Z

Hope this help

test/OrchardCore.Tests.BuildTasks/fr.po

Piedone

I don't really see the advantages mentioned under #4991 (comment). Now instead of MSBuild one-liners we have classes and a test project, while OrchardCore.Application.Cms.Core.Targets.targets didn't become less complex, just different. Maybe it's faster, but we're talking about creating two folders and three files, so there can't be a significant different.

Probably it would make more sense to use C# tasks for indeed complex MSBuild logic.

Piedone · 2024-04-15T22:52:28Z

test/OrchardCore.Tests.BuildTasks/OrchardCore.Tests.BuildTasks.csproj

+
+  <PropertyGroup>
+    <TargetFrameworks>$(CommonTargetFrameworks)</TargetFrameworks>
+    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>


This won't be needed if #15764 is merged first.

Piedone · 2024-04-15T22:54:15Z

test/OrchardCore.Tests.BuildTasks/OrchardCore.Tests.BuildTasks.csproj

Move this under the test solution folder.

It was there, but I'd like to separate them into another project to make it clear that's not related to the actual OC tests project

All tests and even benchmarks are there, so this is suitable (and it's already in the same file system folder).

Benchmarks in a separated project, right?

I don't understand, sorry.

The benchmarks in a separate project OrchardCore.Benchmarks too, so we don't want not mixed things

I'm talking about moving this project to the test solution folder. Not move it into a different project.

I just noticed that while I was preparing a demo

Piedone · 2024-04-15T22:57:46Z

src/OrchardCore.Build.Tasks/CreateWebRootFolderTask.cs

I get this when trying to build the solution locally:

Try to use CLI, I think there's an issue in some VS versions, but I investigate on that

It should just work in VS too.

I rebase to the main and then try again ..

FYI sometimes VS makes me crazy I notice that in this & sources generator PRs :)

That's unfortunately the case indeed. Same happened for Lombiq/Helpful-Libraries#238.

Piedone · 2024-04-15T22:59:43Z

src/OrchardCore.Build.Tasks/CreateFolderTask.cs

+
+public abstract class CreateWebFolderTask : MSBuildTask
+{
+    public abstract string FolderName { get; }


Suggested change

public abstract string FolderName { get; }

public abstract string FolderPath { get; }

It's a folder name, not a path, such as wwwroot

How it's used is a path. Passing wwwroot/some/sub/folder to it would work too.

Ah, it could be

Piedone · 2024-04-15T23:02:10Z

...re/OrchardCore.Application.Cms.Core.Targets/OrchardCore.Application.Cms.Core.Targets.targets

@@ -1,4 +1,12 @@
 <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+
+  <PropertyGroup>
+    <_OrchardCoreBuildTasksAssembly>$(ProjectDir)..\..\OrchardCore.Build.Tasks\bin\$(Configuration)\$(CommonTargetFrameworks)\OrchardCore.Build.Tasks.dll</_OrchardCoreBuildTasksAssembly>


This will only work if the DLL already exists but if you build the solution for the first time, it won't. Won't this cause issues?

It should be there during the build, if it's not there the GitHub Workflow will not pass

Yeah, but this line will be executed even when one just opens the solution and anything else that MSBuild does, not just during a build.

This will only work if the DLL already exists but if you build the solution for the first time, it won't. Won't this cause issues?>

This is weird, could you please try the following:

Add this reference <ProjectReference Include="..\OrchardCore.Build.Tasks\OrchardCore.Build.Tasks.csproj" /> to OC.Cms.Web

Open CMD and locate the web project

Run dotnet clean

Run dotnet build

Can you please make sure you test all of such scenarios? And in VS, the solution should still work with F5.

src/OrchardCore.Build.Tasks/CopyTranslationFilesTask.cs

github-actions · 2024-04-19T22:28:25Z

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions · 2024-06-09T19:19:55Z

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions · 2024-08-08T20:39:34Z

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.

MikeAlhayek · 2024-08-15T17:45:29Z

for instructions on how to resolve the merge conflicts due to #16572 please follow the step listed in this comment.

github-actions · 2024-10-15T00:57:46Z

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.

github-actions · 2024-10-30T08:43:13Z

Closing this pull request because it has been stale for very long. If you think this is still relevant, feel free to reopen it.

hishamco mentioned this pull request Sep 10, 2022

Introduce MSBuild tasks programability #9247

Closed

Introduce MSBuild tasks programability

005b627

hishamco force-pushed the hishamco/msbuild branch from c8f93fe to 005b627 Compare September 10, 2022 01:35

hishamco mentioned this pull request Oct 9, 2022

GitHub Actions pr_ci.yml improvements #12589

Closed

github-actions bot added the merge conflict label Jan 29, 2024

Merge branch 'main' into hishamco/msbuild

681d5e9

github-actions bot removed the merge conflict label Mar 13, 2024

hishamco added 6 commits April 13, 2024 22:26

Merge branch 'main' into hishamco/msbuild

07e14bc

Create localization folder task

2481095

Create web root folder task

f24094b

Update CopyTranslationFilesTask

189e8fc

Try to fix assembly path

f368274

Another attempt

ab18542

hishamco marked this pull request as ready for review April 14, 2024 00:55

hishamco added 4 commits April 14, 2024 04:15

Add OrchardCore.Build.Tasks eference

86bf637

Move the reference to OrchardCore.Application.Cms.Core.Targets

72c6202

Remove the reference

fdbb298

Use $(Configuration)

c9fc713

hishamco force-pushed the hishamco/msbuild branch from 924a93b to c9fc713 Compare April 14, 2024 01:42

hishamco added 2 commits April 14, 2024 04:54

Condition should be applied on CopyTranslationFilesTask

13d3f66

Skip unchanged files

4957796

hishamco commented Apr 14, 2024

View reviewed changes

...re/OrchardCore.Application.Cms.Core.Targets/OrchardCore.Application.Cms.Core.Targets.targets Outdated Show resolved Hide resolved

hishamco added 2 commits April 14, 2024 05:52

Don't copy the placeholder file if exists

0fde7bc

Fix build tasks assembly path locally

f08266d

hishamco requested a review from Piedone April 14, 2024 03:05

hishamco added 2 commits April 14, 2024 07:06

Add unit tests for CreateLocalizationFolderTask

7b6221b

Add unit tests for CopyTranslationFilesTask

28f5382

hishamco mentioned this pull request Apr 14, 2024

Resource manifest source generator #10639

Closed

Merge branch 'main' into hishamco/msbuild

6fa7c2b

Piedone reviewed Apr 15, 2024

View reviewed changes

test/OrchardCore.Tests.BuildTasks/fr.po Outdated Show resolved Hide resolved

Piedone mentioned this pull request Apr 15, 2024

Treat warnings as errors during CI build (Lombiq Technologies: OCORE-157) #15764

Merged

Piedone requested changes Apr 15, 2024

View reviewed changes

hishamco added 2 commits April 16, 2024 02:20

Merge branch 'main' into hishamco/msbuild

43205b6

Address feedback

c9577ec

hishamco added the don't merge label Apr 16, 2024

github-actions bot added the merge conflict label Apr 19, 2024

Merge branch 'main' into hishamco/msbuild

0a1b8b6

github-actions bot removed the merge conflict label Apr 19, 2024

hishamco mentioned this pull request May 26, 2024

Copy translation message logs twice #16133

Open

github-actions bot added the merge conflict label Jun 9, 2024

github-actions bot added the stale label Aug 8, 2024

github-actions bot removed the stale label Aug 15, 2024

github-actions bot added the stale label Oct 15, 2024

github-actions bot closed this Oct 30, 2024

	public abstract string FolderName { get; }
	public abstract string FolderPath { get; }

Introduce MSBuild tasks programability #12364

Introduce MSBuild tasks programability #12364

Conversation

hishamco commented Sep 10, 2022 • edited by Piedone Loading

github-actions bot commented Jan 29, 2024

Piedone commented Mar 24, 2024

hishamco commented Mar 24, 2024

hishamco commented Apr 14, 2024

hishamco commented Apr 14, 2024

hishamco commented Apr 14, 2024

hishamco commented Apr 14, 2024

Piedone commented Apr 14, 2024

hishamco commented Apr 14, 2024

Piedone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 19, 2024

github-actions bot commented Jun 9, 2024

github-actions bot commented Aug 8, 2024

MikeAlhayek commented Aug 15, 2024

github-actions bot commented Oct 15, 2024

github-actions bot commented Oct 30, 2024

hishamco commented Sep 10, 2022 •

edited by Piedone

Loading