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

Refactoring of build targets, common output #224

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jviau
Copy link
Member

@jviau jviau commented Feb 3, 2023

This PR is a major refactor of the build targets for this repo.

  1. Common build props/targets are extracted to Directory.Build.props and similar targets. e.g.: all NuGet packaging props are now common.
  2. All output has been redirected to /out/ folder. This gives a single folder to delete to clean all build artifacts.
    • Subfolders are used for different sections: /out/obj for intermediate, /out/bin for build, /out/pkg for packages (nuget)
  3. NuGet debug symbols moved to snupkg. This is the recommended way to deliver debug symbols, as it is now a separate package that only IDEs download on demand. This keeps nupkg smaller for CI builds.
  4. InternalsVisibleTo moved to msbuild item & target
  5. global.json added to pin dotnet and msbuild SDKs. This forces consistency between different dev/build environments.

⚠️ IMPORTANT ⚠️ after this PR is complete your local builds may fail. This is due to the leftover obj folders possibly containing duplicate generated code. To solve this, you can run:

Get-ChildItem bin -Recurse -Directory | Remove-Item -Force -Recurse
Get-ChildItem obj -Recurse -Directory | Remove-Item -Force -Recurse

or:

git clean -xdf

@sebastianburckhardt
Copy link
Member

Common build props/targets are extracted to Directory.Build.props and similar targets. e.g.: all NuGet packaging props are now common.

I understand that it is in general desirable to not repeat common sections and factor them out into a new file. However, I am a bit lost as to why that means we need now 8 new files contanining configurations of some kind. I think it is a bit extreme and creates more problems than it solves. For example, having to navigate a non-trivial hierarchy of files with complex rules to figure out how a project is built is something I would always want to avoid. I am not an expert in the advanced features of msbuild and I don't really want to have to be one.

For example, I would rather repeat the few lines that add xunit to the project in the two test projects, rather than creating a new .props file that adds the XUnit information automatically , but then needs a special condition to test the name of the project to avoid over-applying this rule, plus an extra exception from that rule for a project with a name that ends in "Tests" but is not a test project.

Honestly, I find it very difficult now to analyze how the build even works. Are the Directory.build.props and Directory.Build.targets "magically" named to be pulled in by msbuild? or are they explicitly included somewhere? Since a lot of them have the same name I guess their interpretation is somehow dependent on the directory path, are they cumulative? Kind of like .gitignore?

If that is indeed how they work, I am not a fan of this type of system (using magically named path-sensitive files) I find it unwieldy. For example, in VS, these files don't show up in the Solution Explorer, so I have to use another way to locate them (I have to look at multiple levels to find them all?) and open them. And once I have these files open in the editor, I can get easily confused as to which file from which directory I am looking at (because they have the same name).

global.json added to pin dotnet and msbuild SDKs. This forces consistency between different dev/build environment

Does the content of this global.json need to be updated from time to time? If yes, how would I figure out what to update it to?

InternalsVisibleTo moved to msbuild item & target

Why? It is vastly more complicated looking. The contents of InternalsVisibleTo.targets are unintelligible to me. I would not be able to debug/maintain it if needed.

NuGet debug symbols moved to snupkg. This is the recommended way to deliver debug symbols, as it is now a separate package that only IDEs download on demand. This keeps nupkg smaller for CI builds.

Do the snupkg need to be uploaded to NuGet manually (just like we currently do for the nukpg) or is it some other mechanism?

Copy link
Member

@sebastianburckhardt sebastianburckhardt left a comment

Choose a reason for hiding this comment

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

Some initial comments.

<PropertyGroup>
<TargetFramework>net6.0</TargetFramework>
<AzureFunctionsVersion>v4</AzureFunctionsVersion>
<IsTestProject>false</IsTestProject> <!-- Not a unit test project -->
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can simplify the logic and instead set it to "true" when it is a test project. I think that would be less of a maintenance pitfall.

BeforeTargets="BeforeCompile">

<WriteCodeFragment AssemblyAttributes="@(_InternalsVisibleToAttribute)" Language="$(Language)" OutputFile="$(GeneratedInternalsVisibleToFile)">
<Output TaskParameter="OutputFile" ItemName="CompileBefore" Condition="'$(Language)' == 'F#'" />
Copy link
Member

Choose a reason for hiding this comment

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

Why is there some check for F# language here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This file was borrowed from the dotnet team - so this is a remnant of that. I'll remove the F# check.

<Project>

<PropertyGroup>
<IsTestProject Condition="$(MSBuildProjectName.EndsWith('Tests'))">true</IsTestProject>
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this rule since it requires an exception (see comment below). I don't think this needs to be automatic, we can just set IsTestProject explicitly in all the projects that are test projects. I think it would be more readable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants