-
Notifications
You must be signed in to change notification settings - Fork 26
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
base: main
Are you sure you want to change the base?
Conversation
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).
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?
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.
Do the snupkg need to be uploaded to NuGet manually (just like we currently do for the nukpg) or is it some other mechanism? |
There was a problem hiding this 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 --> |
There was a problem hiding this comment.
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#'" /> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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
This PR is a major refactor of the build targets for this repo.
Directory.Build.props
and similar targets. e.g.: all NuGet packaging props are now common./out/
folder. This gives a single folder to delete to clean all build artifacts./out/obj
for intermediate,/out/bin
for build,/out/pkg
for packages (nuget)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.InternalsVisibleTo
moved to msbuild item & targetglobal.json
added to pin dotnet and msbuild SDKs. This forces consistency between different dev/build environments.obj
folders possibly containing duplicate generated code. To solve this, you can run:or: