Updating the repo to use Directory.Build.props/targets files

[tannergooding]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• I have provided test coverage for my change (where applicable)

Description

This completes the first half of #867 by moving ImageSharp to use centralized Directory.Build.props and Directory.Build.targets files for managing project and repo-wide configuration settings.

This also sets everything up to support strong-name signing, but is first dependent on SixLabors/SharedInfrastructure#1 being merged and then the various dependencies being updated to consume that strong name key.

After this is merged, work can begin on a follow up PR which updates the build scripts for the repository.

[tannergooding]

Configuration options from the previous are kept and the remaining defaults are explicitly listed. This ensures that any user-specific VS configuration doesn't take precedence instead.

[JimBobSquarePants]

I wasn't aware this was still in the repo. Ideally we would use the one from Standards

https://github.com/SixLabors/Standards/blob/8b085c0ec4fb64797b9965741f7138f8f66a6b44/.editorconfig

[JimBobSquarePants]

Super happy to update the Standards one with these defaults.

[tannergooding]

This needs to live in the appropriate repo root either way, as it is hierarchical in nature and wouldn't be impactful from the standards directory.

[JimBobSquarePants]

I think we can add it from anywhere as long as it's added to the solution and marked as root.

https://docs.microsoft.com/en-us/visualstudio/ide/create-portable-custom-editor-options?view=vs-2019#add-and-remove-editorconfig-files

[tannergooding]

editorconfig isn't some standard that Microsoft/Visual Studio created, it is an external standard that we adopted because of its wide-spread use.

The way it is meant to be used (and the way it works) is described both here: https://docs.microsoft.com/en-us/visualstudio/ide/create-portable-custom-editor-options?view=vs-2019#file-hierarchy-and-precedence and at the source of truth here: https://editorconfig.org/#file-location.

Things won't function (in VS and in other editors) if it isn't located in the current directory or a parent of the current directory.

[JimBobSquarePants]

Man that's such an incredibly frustrating design. I'm going o have to rethink the entire way I enforce standards now. (╯°□°）╯︵ ┻━┻

[tannergooding]

The end_of_line default was changed to lf since that is what git will check it in as (all platforms/by default) and is what git will check it out as on Unix. It makes for better cross-platform consistency.

[Paxxi]

Won't this make a mess of files if a user has set autocrlf=true in their git global settings?
Might want to set eol in the gitattributes file to make sure they're checked out as lf https://www.git-scm.com/docs/gitattributes

[tannergooding]

Good point. I hadn't looked at the .gitattributes file as part of this PR, will update.

[tannergooding]

.gitattributes file was updated to explicitly call out eol=lf as the default and to explicitly specify various attributes for existing file types to ensure that it is handled correctly.

[Paxxi]

I think you need to set bat/cmd to always be crlf or they won't run. Not at computer so can't test it, going on memory

[tannergooding]

bat/cmd definitely work with lf line endings. I have it configured that way in all of my projects and did just test locally (and I don't believe it is Windows 10 specific).

[Paxxi]

Perfect 😀

[tannergooding]

This is the only new option that differs from the VS default. The VS default is true for compatibility reasons. This option is used in combination with csharp_indent_case_contents and you have effectively the following combinations:

(VS Default) csharp_indent_case_contents=true, csharp_indent_case_contents_when_block=true:

switch (value)
{
    case 1:
        break;

    case 2:
        {
            break;
        }
}

csharp_indent_case_contents=false, csharp_indent_case_contents_when_block=true:

switch (value)
{
    case 1:
    break;

    case 2:
        {
            break;
        }
}

csharp_indent_case_contents=false, csharp_indent_case_contents_when_block=false:

switch (value)
{
    case 1:
    break;

    case 2:
    {
        break;
    }
}

(The one I chose) csharp_indent_case_contents=true, csharp_indent_case_contents_when_block=false:

switch (value)
{
    case 1:
        break;

    case 2:
    {
        break;
    }
}

[JimBobSquarePants]

You chose..... Wisely.

[tannergooding]

Once strong-name signing is enabled, it should just involve:

• Updating the standards submodule
• Updating the SixLabors dependencies to the strong-name signed versions
• Flipping this to true
• Adding the PublicKey metdata to the InternalsVisibleTo items

[tannergooding]

Package versions are all listed/managed here. VS won't update these currently (but likely will eventually).

This allows you to ensure that you can upgrade all projects at the same time, easily.

[JimBobSquarePants]

It's a damn shame the tooling is behind here. What happens when you update a package through the manager?

[tannergooding]

It will change the version in the project file directly.

[tannergooding]

Updated to use PackageReference Update="package-name" Version="" rather than the *PackageVersion properties, as suggested on the twitter thread.

It, unfortunately, doesn't work with the UX either (project file will still be directly changed), but it seems a lot "cleaner" way of doing this.

[tannergooding]

This is the Roslyn compiler toolset and ensures all machines build with the same compiler version, regardless of what is on the box.

IsImplicitlyDefined=true just means users can't remove this from the list of NuGet packages in the UX.

[JimBobSquarePants]

TIL

[tannergooding]

All of these are managed via the RestoreSources property in the Directory.Build.props

[tannergooding]

The Directory.Build.targets allows you to manage InternalsVisibleTo via these item groups.

[tannergooding]

This just keeps the InternalsVisibleTo from showing up in the UX as "files". The ItemDefinitionGroup allows you to add this metadata to all InternalsVisibleTo items centrally.

[tannergooding]

A simple target to dynamically create the InternalsVisibleTo file, similarly to how AssemblyInfo is generated in SDK style projects.

[tannergooding]

All of this removed stuff is now centrally managed.

[tannergooding]

These are now project metadata in the form of InternalsVisibleTo items

[tannergooding]

This appeared to be the only think preventing TargetFrameworks from working correctly...

[JimBobSquarePants]

What's happening?

[tannergooding]

In master right now, this project is only targeting a single tfm with TargetFrameworks being commented out.

It looks like the double inclusion of an entry point was what was preventing the other tfms from working and that filtering out Program.cs for the relevant tfms allows everything to work.

I explicitly called it out in case there was some other reason this should stay targeting a single tfm.

[codecov]

Codecov Report

Merging #920 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #920      +/-   ##
==========================================
- Coverage   89.26%   89.26%   -0.01%     
==========================================
  Files        1076     1076              
  Lines       47210    47209       -1     
  Branches     3295     3315      +20     
==========================================
- Hits        42141    42140       -1     
  Misses       4366     4366              
  Partials      703      703

Impacted Files	Coverage Δ
...iles/ICC/DataWriter/IccDataWriter.NonPrimitives.cs	100% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07ec651...811efc4. Read the comment docs.

[codecov]

Codecov Report

Merging #920 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #920      +/-   ##
==========================================
- Coverage   89.26%   89.26%   -0.01%     
==========================================
  Files        1076     1076              
  Lines       47210    47209       -1     
  Branches     3295     3315      +20     
==========================================
- Hits        42141    42140       -1     
  Misses       4366     4366              
  Partials      703      703

Impacted Files	Coverage Δ
...iles/ICC/DataWriter/IccDataWriter.NonPrimitives.cs	100% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07ec651...81c9deb. Read the comment docs.

[tannergooding]

This was bumped to 7.12.0 as part of centralizing the package versions.

[tannergooding]

I think the codecov "regression" may be due to a dead-code optimization done by the compiler as the code is currently annotated and is indeed not used:

// TODO: This is not used?
byte mb = (byte)((minor << 4) | bugfix);

[tannergooding]

I think this is all good now. CC. @JimBobSquarePants, @antonfirsov

[JimBobSquarePants]

I wasn't aware this was still in the repo. Ideally we would use the one from Standards

https://github.com/SixLabors/Standards/blob/8b085c0ec4fb64797b9965741f7138f8f66a6b44/.editorconfig

[JimBobSquarePants]

You chose..... Wisely.

[JimBobSquarePants]

Super happy to update the Standards one with these defaults.

[JimBobSquarePants]

It's a damn shame the tooling is behind here. What happens when you update a package through the manager?

[JimBobSquarePants]

TIL

[JimBobSquarePants]

What's all this stuff? Feels like a relic from the past.

[tannergooding]

These are all files that you currently have checked directly into the repository, namely under tests\Images.

[JimBobSquarePants]

Can we wildcard this?

[tannergooding]

I don't believe solution files support that, and from a quick test locally it doesn't work.

I also don't feel particularly strongly about this being in here, I've just found that having all files accessible from the solution is convenient.

[JimBobSquarePants]

What's happening?

[tocsoft]

this looks great, thanks a lot @tannergooding all the changes get a big thumbs up from me 👍

[JimBobSquarePants]

Let's flip this switch! Thanks @tannergooding for persevering and teaching me some new stuff!