feat(core): adding templates with default output path properties to init generator by tzuge · Pull Request #526 · nx-dotnet/nx-dotnet

tzuge · 2022-09-08T22:25:13Z

Adding msbuild customization files (#469 and #470)

Extending the @nx-dotnet/core:init generator to include initialization of MSBuild customization files Directory.Build.props and Directory.Build.targets. The targets file is a placeholder for workspaces to configure customization that should be imported later. The props file includes build output path properties equivalent to what is currently added per project via XML manipulation.

Some remaining work:

Update e2e tests
Documentation updates
Verify idempotence expectation - Do we need this for the targets file and props file independently or is treating them as a pair sufficient?
Remove of the XML manipulation for setting of output paths - What is the backwards compatibility expectation? OutputPath property in projects still needed if Directory.Build.* files are not present (e.g. in workspaces initialized with prior versions.)
Code coverage report path - nx workspaces normally output reports to path like coverage/apps/my-app. This potentially can be configured in a property in Directory.Build.props as well
CheckNxModuleBoundaries target - this should be possible to handle in Directory.Build.targets

…nit generator

nx-cloud · 2022-09-08T22:25:14Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b4fed0f. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 9 targets

Sent with 💌 from NxCloud.

Also, removing use of XML manipulation since it is no longer needed and skipOutputPathManipulation option since it is no longer applicable.

AgentEnder

I know this is still a draft, but I wanted to leave some preliminary thoughts.

This is looking great so far, and is definitely an excellent first contribution. I really like how it simplifies the handling of these actions.

AgentEnder · 2022-09-10T16:15:48Z

packages/core/src/generators/init/templates/root/Directory.Build.targets__tmpl__

+  <Target Name="CheckNxModuleBoundaries" BeforeTargets="Build">    
+    <Exec Command="node $(NodeModulesRelativePath)/<%= checkModuleBoundariesScriptPath %> -p $(NxProjectName)"/>
+  </Target>


I'm not sure this would be accurate when dealing with projects that's names don't match the file structure. Since we control the node script too, let's update it to take the project root as a parameter. Then we could look for the project based on its root path.

We should add project-root as a parameter and accept either... https://github.com/nx-dotnet/nx-dotnet/blob/master/packages/core/src/tasks/check-module-boundaries.ts#L90-L95

If project provided use it, if project-root provided look up the project.

From my understanding of getProjectFileForNxProject, there should only be one dotnet project under each nx project root. We could overload the project argument, so if file path value with file extension is provided (csproj, vbproj, etc.), then it looks for the nx project configuration where the root is part of the path. Alternative would be to have a different argument if there's a good name for it.

I'd prefer to use a different argument, s.t. the original parameter doesn't have any new modality added to it. I'd use --project-root as the flag, should be added as projectRoot in the lines I linked above.

AgentEnder · 2022-09-10T16:25:16Z

packages/core/src/generators/init/generator.spec.ts

+  it('should not add directory build props and targets files if props file exists', async () => {
+    appTree.write('Directory.Build.props', '');
+    const spy = jest.spyOn(devkit, 'generateFiles');
+    await generator(appTree, null, dotnetClient);
+
+    expect(spy).not.toHaveBeenCalled();
+    const hasTargetsFile = appTree.isFile('Directory.Build.targets');
+    expect(hasTargetsFile).toBeFalsy();
+  });


This is good for now, in future I'd like to update these operations to add the target if it's not in there already even if file already exists.

AgentEnder · 2022-09-10T16:26:47Z

packages/core/src/generators/init/generator.ts

+  if (!initialized && !isDryRun()) {
+    const checkModuleBoundariesScriptPath = normalize(
+      relative(
+        host.root,
+        resolve('@nx-dotnet/core/src/tasks/check-module-boundaries'),
+      ),
+    );
+
+    generateFiles(host, path.join(__dirname, 'templates/root'), '.', {
+      tmpl: '',
+      checkModuleBoundariesScriptPath,
+    });
+  }


This doesn't need the isDryRun flag, it is only necessary when the dot net cli is creating files. When we generate files through nx devkit, nx handles dry run for us

tzuge · 2022-09-12T19:53:25Z

packages/core/src/generators/import-projects/generator.ts

    configuration.targets.test = GetTestExecutorConfig();
  }
  addProjectConfiguration(host, projectName, configuration);
-  await manipulateXmlProjectFile(host, {


Should init generator be run as part of import-projects with the removal of this XML manipulation?

Yeah, init should have always been called in here, that's a miss on my earlier part

Ok, I added the call of the init generator without conditions, so it is run even if no projects are found.

Also, I put

return async () => { await installTasks() }

and it's inconsistent with GenerateProject, but I wasn't sure if the not awaited result there is intentional.

… for msbuild task

tzuge · 2022-09-16T23:45:23Z

Hey @AgentEnder, how is the behaviour described by this test implemented?

nx-dotnet e2e › nx g test › should add a reference to the target project

I can't seem to find execution of project-reference generator or addProjectReference on the client from the test generator.

AgentEnder · 2022-09-16T23:57:32Z

Hey @AgentEnder, how is the behaviour described by this test implemented?

nx-dotnet e2e › nx g test › should add a reference to the target project

I can't seem to find execution of project-reference generator or addProjectReference on the client from the test generator.

It's found here: https://github.com/nx-dotnet/nx-dotnet/blob/master/packages/core/src/generators/utils/generate-test-project.ts#L81-L90

…aviour back

AgentEnder · 2022-09-19T18:20:26Z

Accidentally converted out of draft when trying to approve checks, going to leave it in draft until you mark it :). Thanks again!

…find nx project

tzuge · 2022-09-19T19:25:12Z

No problem. I think it's good for review at this point. The commit prefixes don't necessarily make sense, but I'm assuming this will get squashed for the proper conventional commit log entry.

A few notes:

I put addProjectReference back in the test generator but no longer subject to skipOutputPathManipulation option, which has been removed.
--project-root arg on the check module boundaries feels a bit kludgy; I can't imagine any manual execution of the script using that arg
the init generator e2e test is dependent on order of execution of tests since the other generators will run init internally, so it's not guaranteed that the test passes because of the command executed in the test.

…ject and verify call

sonarqubecloud · 2022-09-19T20:48:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

AgentEnder · 2022-09-26T18:57:27Z

This looks good to me 🎉, I'm going to spend some time testing the functionality manually and then we can cut a release. Thanks @tzuge for the large contribution, this will put the project in a more maintainable state!

AgentEnder · 2022-09-28T14:11:31Z

@all-contributors please add @tzuge for code and design

allcontributors · 2022-09-28T14:11:40Z

@AgentEnder

I've put up a pull request to add @tzuge! 🎉

# [1.14.0](v1.13.4...v1.14.0) (2022-10-05) ### Features * **core:** adding templates with default output path properties to init generator ([#526](#526)) ([c57fbd3](c57fbd3)) Oct 5, 2022, 7:19 PM

github-actions · 2022-10-05T19:22:04Z

🎉 This PR is included in version 1.14.0 🎉

The release is available on:

v1.14.0
GitHub release

Your semantic-release bot 📦🚀

github-actions · 2023-02-25T01:41:32Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

feat(core): adding templates with default output path properties to i…

cd51a69

…nit generator

feat(core): including pre build target in Directory.Build.targets

a8d0558

Also, removing use of XML manipulation since it is no longer needed and skipOutputPathManipulation option since it is no longer applicable.

AgentEnder reviewed Sep 10, 2022

View reviewed changes

tzuge commented Sep 12, 2022

View reviewed changes

feat(core): adding project-root arg to check module boundaries script…

3eceebc

… for msbuild task

feat(core): updating e2e tests and putting test project reference beh…

a602067

…aviour back

AgentEnder marked this pull request as ready for review September 19, 2022 18:01

AgentEnder marked this pull request as draft September 19, 2022 18:19

feat(core): updating check module boundaries to use path.relative to …

5badb07

…find nx project

tzuge marked this pull request as ready for review September 19, 2022 19:23

feat(core): update generate-project unit test to mock GenerateTestPro…

b4fed0f

…ject and verify call

AgentEnder merged commit c57fbd3 into nx-dotnet:master Sep 26, 2022

allcontributors bot mentioned this pull request Sep 28, 2022

chore(repo): add tzuge as a contributor for code, and design #532

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2023

Conversation

tzuge commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Adding msbuild customization files (#469 and #470)

Uh oh!

nx-cloud bot commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

AgentEnder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tzuge commented Sep 16, 2022

Uh oh!

AgentEnder commented Sep 16, 2022

Uh oh!

AgentEnder commented Sep 19, 2022

Uh oh!

tzuge commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Sep 19, 2022

Uh oh!

AgentEnder commented Sep 26, 2022

Uh oh!

AgentEnder commented Sep 28, 2022

Uh oh!

allcontributors bot commented Sep 28, 2022

Uh oh!

github-actions bot commented Oct 5, 2022

Uh oh!

github-actions bot commented Feb 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tzuge commented Sep 8, 2022 •

edited

Loading

nx-cloud bot commented Sep 8, 2022 •

edited

Loading

tzuge commented Sep 19, 2022 •

edited

Loading