dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.68k stars 1.06k forks source link

Implement CleanFull target #3067

Closed jzabroski closed 4 years ago

jzabroski commented 5 years ago

In my .NET Core projects, I define a "cleanfull" target in all my .NET Core projects that does the following:

The normal MSBuild Clean is a pain in the ass to deal with when upgrading .NET framework versions and making sure metapackages resolve consistently between development environment and build server.

The only alternative way to solve this problem cleanly is to run your entire build pipeline, from local dev environment to build server, through a isolated process via docker container or similar.

peterhuene commented 5 years ago

Hi @jzabroski. Thank you for the suggestion!

I'm not entirely convinced that we need a "full clean" target in the SDK. Most of what you listed could typically be accomplished with a command to clean the files not under source control from a repository (e.g. git clean).

As for cleaning the global packages, there is the dotnet nuget locals command that can be used to clear the global packages, if desired.

That said, I'll add this to the backlog for now and request comments from @KathleenDollard and the rest of @dotnet/dotnet-cli.

jzabroski commented 5 years ago

These are all really good points, and my only rebuttal would be that about ~11 years ago when I was graduating college, I remember the word "simple" being used over and over again at a Microsoft Build conference, because there was this "critical moment" in High Technology where everyone agreed we were building things correctly but in complicated ways that were hard to learn, hard to discover, and ultimately hard to use.

Microsoft has combated a lot of this "accidental complexity" over the years, but there are times when I make what I think are pragmatic (Dear Webster, please add 'controversial' to your thesaurus for 'pragmatic'!) suggestions. These ideas often get blocked by vendors I worked with, because there is a minimum viable product that "does the job" even if it doesn't score high in the learn/discover/use metrics.

CleanFull is what I would describe as easy to discover, easy to understand what it should do, and ultimately provide a canonical way regardless of what version control system you are using to describe in Microsoft Docs how to clean a project after doing a major upgrade.

As I said, there are other, pragmatic, controversial ways, too: Using Docker for everything also does the job, but not everyone uses Docker. I'm no slouch technologically, and with LCOW I'm really just jumping on the Docker boat (hah) in 2019.

jzabroski commented 5 years ago

One last thought: It would be awesome and a testament to this feature's success if the canonical answer on Stack Overflow for how to fix build issues after a major upgrade is "Oh, you just need to run CleanFull". That's what I would define as a Perfect 10 in learn/discover/use metrics.

KathleenDollard commented 5 years ago

@jzabroski It's an interesting thought. Would you expect the syntax to be something like:

> dotnet clean --full

Questions:

Thanks!

jzabroski commented 5 years ago

@KathleenDollard I have not forgotten about your questions, but I am wondering if there are higher priority causes for me to be involved in.

  • @peterhuene suggested git clean. This would clean obj/bin, and more. Do you think it's desirable to leave any git files other than obj/bin? To put a different way do you think more "just clean everything" (which would require git) or "just clean up the stuff you do on build"?

I do not think the dotnet.exe SDK should be tightly coupled to git, even if Microsoft acquired a company with the word git in its name. Principally, Microsoft should be delivering software that allows the widest spectrum of developers.

In terms of rationale for why developers would not just use git clean, the same logic applies: If you are not using git, then every developer will have a different command to run. This puts us back into the dark ages of some developers using xcopy, some developers using robocopy etc to package their dlls instead of the land of milk and honey we know as nuget today. Will @peterhuene solution work? Sort of, in a bubble gum and duct tape way.

  • Do you have any concerns on the fact that other projects would have to update their dependencies in the shared cache?

I probably don't understand the question fully - I probably don't fully understand how dotnet SDK does its magic and if you can explain some of that to me, I can probably weigh in. Many Java projects would have a special "prepare" task that would create the directory structure for project assets like the WEB-INF folder. A clean in Java task basically takes all the directories creates in the prepare task and undoes them.

  • What directory behavior would you expect? You describe it as run from the project directory I think. What if it was run from a directory with a solution file? What if was run from a repository root with no project of solution files? Would you anticipate this command being dependent on the directory context?

In my implementation, there is a property item that is called SolutionOrProjectFile that searches the path using the root directory and current directory. It probably can be made more robust, but it has not bitten me yet.

As for syntax, I want:

> dotnet cleanfull

The reason is auto-complete. I use PowerShell in all my projects and my msbuild/dotnet.exe wrapper to "b" so that all I have to do is type b and then "c" and TAB and I get "b clean" TAB again and I get "b cleanfull". "bb" rebuilds all.

KathleenDollard commented 5 years ago

I think this is an interesting idea we can have in our backlog.

Thanks for clarifying those points.

jzabroski commented 4 years ago

@KathleenDollard Ran into a situation again today where this would have been helpful migrating a .NET 4.6.1 "old" project type to the new project type format.

It seems the new Common Project System depends upon either the bin or obj directories for some of its inference passes, and that when you upgrade a project from one version to the next, those old bin/obj files cause bad inferences to occur, so you get invalid errors complaining about TargetFrameworks property needing 4.8.

I guess the alternative is to continue improving the inferencing, but I just think CleanFull is such an obvious and effective way to do this. It also demonstrates WHY I would not run git clean: Because I am in the middle of upgrading projects, and I might flip back and forth between project types as I try to upgrade.

jzabroski commented 4 years ago

May've hit another situation today, but need a small repro to confirm. I'm mainly logging this with notes. I think if I hit this need a couple more times, I'll be motivated to demonstrate the general usefulness of CleanFull.

Scenario: You're writing an application, and then need to re-target it to be deployed into a strong-name context. You now need to transitively sign all dependencies. So you read online and hear about StrongNamer package on nuget.org. You add it, but it doesn't do as promised, because there are old bin/obj files laying around: StrongNamer operates on packages which in turn get copied locally to those bin/obj file folders.

I'm a little skeptical this simply isn't my own user error, so forgive me for using this thread as a psuedo-diary.

jzabroski commented 4 years ago

I may've just hit another one. In the same day. Although, this may be a Visual Studio caching bug moreso than a SDK issue. But, I guess my question is, where does that cached value come from, and why doesn't re-loading the project fix the issue?

Again, upgrading .NET Framework 4.6.1 to .NET Framwork 4.8, the Visual Studio Project Properties dialog showed that the projects Platform Target was x86 (see screenshot) . Except, when I deployed it, it deployed as x64, because I didn't specify the following:

  <PropertyGroup>
    <PlatformTarget>x86</PlatformTarget>
  </PropertyGroup>

After going into Visual Studio and "kicking the value" to x64 and then back to x86, it magically fixed the problem. In this case, I'm really not sure whether this was caused by a bug in Visual Studio or the SDK, and if CleanFull would have helped.

image

nguerrera commented 4 years ago

I'm not keen on an adding aggressive clean option. I think that is best left to source control that knows which files are part of the repo. git clean is the next level from dotnet clean already. The problem with any wholesale, recursive directory deletion is that it is prone to errors. I have seen msbuild authoring bugs delete most of a machine with <Delete Files=_$(SpelllingError)\**\*.*" />, but even if the feature itself does not have such a bug, the more intrinsic risk is that the project has misconfigured IntermediateOutputPath or OutputPath and then the "full" clean would lead to data loss. I really don't want to be in this business.

jzabroski commented 4 years ago

@nguerrera That's a killer argument. Effectively, what you're saying is MSBuild can be used to execute a Confused Deputy attack: Because MSBuild has the user's permissions, and the user's permissions include all files the user owns, and many users run Visual Studio as administrator for profiling and debugging purposes. I guess the only way to support CleanFull safely then would be in a container. Which now seems like another notch as to why I should be building everything inside a container instead of my desktop.

jzabroski commented 4 years ago

@dplaisted Did you mean to reference this issue in that commit?

Background Noise: There are additional scenarios where bin and obj folders hanging around (effectively cached) cause development issues. See this comment here: https://github.com/dotnet/corert/issues/7200#issuecomment-566957554

I think I may evolve my original CleanFull proposal after more closely studying Maven Goals and how these goals neatly simplifies many build tasks. In other words, if the SDK had a Maven POM and Clean Goals, then CleanFull could be achieved by invoking all Clean Goals in the POM. This design might overcome the technical flaws mentioned by @nguerrera but would not be resilient to all Confused Deputy attacks due to deleting files outside the solution space.

nguerrera commented 4 years ago

Fixing tag. @dsplaisted