Too many defaults that are opt-out, and they are different from what MSbuild and the .NET Core SDK chose as defaults

[jcagme]

Aspnet has a defined set of properties which are different from the default values in Arcade so they had to put workarounds. i.e.

• https://github.com/aspnet/Extensions/blob/master/eng/Workarounds.BeforeArcade.props
• https://github.com/aspnet/Extensions/blob/master/eng/Workarounds.AfterArcade.targets
• https://github.com/aspnet/Extensions/blob/master/eng/Workarounds.AfterArcade.props

Full list of properties they had to work around were:

• Xliff tasks for resx generation.
• DebugType == embedded
• Strong name key == MicrosoftShared
• Test framework == xunit 2.4.1-pre.build.4059. (We were on 2.3 and there are breaking changes between 2.3 and 2.4)
• Using ref assemblies from a NuGet package for .NET Framework projects (Microsoft.NETFramework.ReferenceAssemblies). This breaks some ASP.NET scenarios
• MSBuild warnings as errors. Sometimes warnings are inevitable and we don’t have a way to fix the warnings.
• Projects, by default, do not produce packages when calling dotnet pack. (This is the opposite default that Microsoft.NET.Sdk chose.)
• Any project named “Tests” gets automatic references to xunit. We have some tests that use NUnit.
• The obj/ and bin/ folders are moved to the repo root. (I actually like this but…) it’s not the default MSBuild chose, and we rely on some tools which don’t work unless bin/ and obj/ are in the default MSbuild location.

Is the idea for customers to have these types of workarounds in place defining the values of the properties or we should have a better overriding story in place?

[tmat]

Xliff tasks for resx generation.

What tools are used for localization?

<UsingToolXliff>false</UsingToolXliff>

This should be in Versions.props file. This is the intended way how repos should opt-out of Xliff localization or other optional Arcade SDK features. So I'm not sure what the problem is here.

Using ref assemblies from a NuGet package for .NET Framework projects (Microsoft.NETFramework.ReferenceAssemblies). This breaks some ASP.NET scenarios

true

Ditto. Also move to Versions.props. It would be useful to know what the problem is.

DebugType == embedded

What's the problem with embedding symbols in CI builds? The reason why we do so is that in CI builds symbols are not published to symbol servers. So when investigating a crash dump that was captured during CI build the symbols are not available unless embedded to he DLLs.

Strong name key == MicrosoftShared

This is just default. It's definitely expected that repos that need different key override the default. Why is the default bad?

Test framework == xunit 2.4.1-pre.build.4059. (We were on 2.3 and there are breaking changes between 2.3 and 2.4)

Again, I don't understand what the problem is. If the default doesn't work for you setting <XUnitVersion> in Versions.props is the intended way to override.

Projects, by default, do not produce packages when calling dotnet pack. (This is the opposite default that Microsoft.NET.Sdk chose.)

I'm open for discussion about this one. I believe turning this on by default in the SDK was a mistake, because usually one needs to specify a bunch of properties (like PackageDescription, tags, etc.) in order to have a reasonable content of the package. So I think expressing the intent to produce a package is better than accidentally producing and publishing bad/useless packages (like packages for utilities, etc.). Note that msbuild MySolution.sln /t:pack runs Pack target on all projects. Unless they specify IsPackable false they produce a package which would get published. @livarcocc @nguerrera

MSBuild warnings as errors. Sometimes warnings are inevitable and we don’t have a way to fix the warnings.

Any problem with listing the warnings that should be suppressed in <NoWarn>?

Any project named “Tests” gets automatic references to xunit. We have some tests that use NUnit.

There are two knobs you can use:

• repo that doesn't use xunit at all can set UsingToolXUnit=false in Versions.props.
• if some projects use xunit and others nunit then the ones that use nunit can specify <TestRunnerName>NUnit<TestRunnerName> in the project file. This will stop XUnit references from being added. We need to add support for NUnit runner to Arcade SDK though, so that /t:Test works for NUnit. This should be fairly easy to do, the targets implemented in https://github.com/dotnet/arcade/tree/master/src/Microsoft.DotNet.Arcade.Sdk/tools/XUnit just need to be reimplemented for NUnit.

I need to document this better.

The obj/ and bin/ folders are moved to the repo root. (I actually like this but…) it’s not the default MSBuild chose, and we rely on some tools which don’t work unless bin/ and obj/ are in the default MSbuild location.

This is a proposed product feature: https://github.com/dotnet/sdk/issues/867. What tools are those?

[jcagme]

@natemcmaster @dougbu

[natemcmaster]

The main point of this feedback is not the specifics. We could debate those separately. I listed those as examples given to illustrate the larger problem. Microsoft.DotNet.Arcade.Sdk is very heavy handed about what it does with projects, and in my opinion, deviates too much from how Microsoft.NET.Sdk and MSBuild are designed. We're having trouble adopting Arcade in aspnet repos because Microsoft.DotNet.Arcade.Sdk, the YAML templates, BAR manifest generation, publishing, and more are tightly coupled. It has not been reasonable or cheap for my team to adopt Arcade. I have basically had to read every line of Microsoft.DotNet.Arcade.Sdk to make sure using it will change important aspects of the product we produce. It has been so difficult getting just a few of our repos that I am considering just not doing it at all for our larger repos, like aspnet/AspNetCore.

[dougbu]

Any project named “Tests” gets automatic references to xunit.

This doesn't help anything very much if it means tests only run in Arcade. For aspnet/Extensions, aspnet/AspNetcore, and so on, we want to be able to run tests in local builds. Will that be possible?

Also, (especially in aspnet/AspNetCore) many projects with "Tests" in the name are testassets and most of our test projects are named "….Test".

[nguerrera]

Who said tests only run in arcade? We have losts of repos with tests that run in arcade, dotnet test, VS test explorer and build -test?

[nguerrera]

Projects, by default, do not produce packages when calling dotnet pack. (This is the opposite default that Microsoft.NET.Sdk chose.)

I don't remember if this was really discussed before v1. Pack target is implemented entirely in nuget so Microsoft.NET.Sdk didn't make the choice here. cc @rrelyea I don't have a strong opinion for or against how arcade chose to do it. I think the scenario of being able to pack a sln without getting cruft is compelling, and avoiding cruft is usually easier with opt-in than opt-out.

[nguerrera]

Moving between different infrastructure is always hard.

Arcade being opinionated is a big part of its appeal to me. There are a number of things that are requirements for our infrastructure that don't apply to file -> new project. I don't think we can make it a requirement that all default behaviors are the same between those. I think having certain important things done for me by default if I follow opinionated conventions is a good approach. Historically, when we have relied on each team to do it their own way, we have duplicated effort at best and had gaps in which parts of the stack were meeting certain requirements at worst. We finally have things like source linking and symbol publishing just working without having to maintain it ourselves.

I get that if you already built the equivalent with a different approach, then migrating is hard. The dividends will still come in the future when we can share improvements across more teams with less duplicated effort.

[markwilkie]

Now that we've shipped preview 2, I'll circle back around and work towards getting this area of 'defaults' more crisp. More soon.....

[tmat]

What @nguerrera says. It is the goal of Arcade SDK to set certain standards and enforce some amount of uniformity. There is indeed a balance - we need to give repos freedom to customize certain things but not others. We can discuss each particular requirement, convention and default. But if this issue is asking for Arcade SDK to not do that then it can be closed as "won't fix, by design".

[dougbu]

Who said tests only run in arcade?

Nobody. My statement included an "if" and I asked for confirmation.

[nguerrera]

Sorry, confirming that it is totally possible.

[natemcmaster]

The original list of things @jcagme posted were given in response to the question, "what did aspnet have to workaround?" and "what were the pain points in adopting Arcade?". You have my feedback. You're welcome to dismiss it. All I wanted to let you know is that adopting Arcade is not trivial, the things it does are not always right, and in my opinion may of its behaviors are magic, clever, and fancy and a pain to demystify.

From https://github.com/dotnet/arcade/blob/master/Documentation/Overview.md

Overview / Introduction

We need well-understood and consistent mechanisms to consume, update, and share engineering infrastructure across the .NET Core team.

The primary concept is to break the infrastructure into “pay for play” components, such that one piece of functionality can be consumed, with minimal dependencies. The idea is to not force unnecessary dependencies, thus making it more reasonable to consume only what is needed.

This approach publishes what amounts to “public surface area” for the shared engineering infrastructure. These “contracts” then allow for the product teams to reason about how (or if) they participate and manage their engagement with the common infra over time. In short, the product teams “pull” what is needed, when it’s needed.

...

Principles

• Updates and changes should always be done with all the repos (not just yours) in mind. This implies compromise by all to achieve a better common goal.
• Simplicity and austerity are our friends. We want to avoid clever, magic, or fancy features.

[tmat]

"what did aspnet have to workaround?" and "what were the pain points in adopting Arcade?". You have my feedback. You're welcome to dismiss it

I am not dismissing any specific feedback. I welcome feedback on any particular setting/convention and am ready to explain the reasons for them. What I am saying above is that we need to constrain repos to some degree and disallow some customizations to get a sane system.

behaviors are magic, clever, and fancy and a pain to demystify.

Perhaps. It depends on what you consider magic and fancy. Again, this is not actionable unless we talk about specific feature.

[natemcmaster]

Would you rather I open an issue for each default that I think should be opt-in? Or have a long discussion on this thread?

[natemcmaster]

typo: default that should be made opt-in

[tmat]

We can start by this comment: https://github.com/dotnet/arcade/issues/1912#issuecomment-459086378 I have some questions there that would be useful to have answers for. Thanks!

[markwilkie]

I'm going to speak with @natemcmaster in person, and then we should circle back around on this (and the other "defaults") issues.

[nguerrera]

For any default we decide to revert, we need a plan to roll out without breaking everyone. Silently stopping localization for example could go unnoticed. I'm not against that default being the other way (also just an example), we just have to be prepared to go through and opt everyone who needs it back in.

[natemcmaster]

Yes, let's chat @markwilkie. I'm currently feeling sad about this issue because I wrote down a list about a year ago on this subject (see https://gist.github.com/natemcmaster/0ef23512c011b7ec09f25ad13a1d65e2 and https://github.com/dotnet/arcade/issues/88) and this was apparently not given consideration when selecting Arcade SDK defaults. I agree with @nguerrera that we should come up with a sensible migration plan if defaults are changed. I wish different defaults had been selected from the beginning, but oh well...

[tmat]

@natemcmaster Your feedback has been useful and taken into account. Arcade SDK has been evolving. There wasn't a single point where we designed upfront what all the defaults will be. Some were inherited from the previous implementation (RepoToolset). As I said many times over and over, we can indeed consider changing the defaults, if there is a good reason for doing so. But we just can't do everything everybody's asking for. The main reason is that we want some uniformity across repos. Also, breaking changes mean work across many repos as @nguerrera pointed out.

So, I'll repeat one more time that I'm open for discussion on any particular default value or requirement. Please, provide answers for my questions here: https://github.com/dotnet/arcade/issues/1912#issuecomment-459086378

I can also comment on the items listed in https://gist.github.com/natemcmaster/0ef23512c011b7ec09f25ad13a1d65e2 if you'd like me to.

[natemcmaster]

Chatted with Mark about how we might approach this. Posting here for additional feedback.

Differences between repos can be put into a few categories. For the purpose of this thread, those categories are:

• Category 1 - differences which affect the product in an important way.
• Category 2 - differences which are exceptions to something that is otherwise a good default
• Category 3 - differences which are the result of team/individual preferences and are important to reconcile
• Category 4 - differences which are the result or team/individual preferences and are NOT important to reconcile

Examples:

• Category 1 - the strong name key. We should not expect corefx or aspnet to change the assembly identity - this is an essential aspect of the product
• Category 2 - using .NET Framework references as a package instead of global machine install. Normally good as is it compensates for a gap in .NET SDK xplat support. There are exceptions, like aspnet MVC, where this may break the product and its not trivial to fix
• Category 3 - the layout of the artifacts/ folder. This is important to reconcile because it impacts how Arcade works.
• Category 4 - tabs vs spaces (let's just not even go there)

My proposal for a strategy to handling differences:

• Category 1 - when we identify these kinds of differences, there should be no default in the Arcade SDK. Each repo should opt-in to the settings required.
• Category 2 - it is okay for the Arcade SDK to provide a default. We should document how to opt-out and (when reasonable) add functional tests to make sure the opt-out flags work. We should minimize as much as possible to reduce the risk that changes to Arcade break repos which opt-out of these kinds of defaults.
• Category 3 - try to eliminate these. Easier said than done, but there is value in consistency. It is hard to provide specific guidance on how we come to agreement, but some useful principles are
  • We prefer defaults that match the defaults used by 'vanilla' Visual Studio/C# code formatters/MSBuild/.NET Core SDK.
  • Weigh the costs of adopting different alternatives by looking at impact on existing Arcade repos.
  • Give consideration to how this affects new repos onboarding to Arcade.
  • We should also avoid blocking people from having differences, but still strongly encourage consistency.
• Category 4 - we allow room to agree to disagree and still be friends.

[markwilkie]

The next step here is probably to "crisp up" what qualifies as a category 1...

[tmat]

@natemcmaster That's a great write up. Thanks for doing this! I agree with the categorization. I'd perhaps add another category - company policies. Such as "all artifacts must be signed", "all shipping binaries must be localized", "all packages must have a license file", etc. These should imo be enforced by Arcade.

Regarding the strong name key example - should Arcade remove the default and instead report an error saying that strong name key file must be specified? The specific key will differ across repos but all assemblies are required to be strong name signed.

[ericstj]

I agree that some things are "must" but they need not be "must do this way". We have to be careful about forcing folks to do things a single way. If we have a safe default then make a default but still permit customization. If we don't have a safe default but still want to provide enforcement we can, but don't make that enforcement push folks down a single path of implementation when many can satisfy the requirement.

[tmat]

@ericstj If there are N ways of achieving some requirement then either 1) We can agree on one that is optimal 2) We do not agree on the best because they are pretty much the same (all have some pros, some cons that balance out). a) No one has any particular preference b) Different repo owners have different preferences for their very good reasons based on specific requirements of the repo/product

Then [1] We use the optimal one as default and strongly recommend using that approach. Repos should not implement another one since we know why that won't be optimal. If a better solution is found in future, update Arcade to use that solution, so everyone benefits from it. [2a] We choose one and strongly recommend using that approach. Repos should not implement another one since that's just arbitrary difference for no good reason, since we agreed that they are pretty much the same. Arbitrary differences just add overhead. Again, if a clearly superior solution is found in future, goto [1]. [2b] Provide default with first class customization.

My 2 cents.

[markwilkie]

Ok - here's a DRAFT https://github.com/dotnet/arcade/pull/1997

[markwilkie]

Closing given the published doc: https://github.com/dotnet/arcade/blob/master/Documentation/DefaultsGuidance.md