Directory build project import might be too greedy

[jeffkl]

In pull request #751/commit da283a7 we added functionality in Microsoft.Common.props and Microsoft.Common.targets to walk up the directory tree looking for Directory.build.props and Directory.Build.targets to import.

This functionality may be too greedy. For example, if a user accidentally places one of the files at the root of their drive, MSBuild could start importing it for all projects built on that drive (if their directory structure didn't contain a Directory.Build.* file).

The logic is similar to how dotnet searches for a global.json and how NuGet searches for a NuGet.config.

The primary suggestion here would be to stop traversing up if a particular file system marker is found. For example, if a .git folder is found, the traversal could stop. However, this could also slow down the search and maintaining the list could also be problematic.

We'll use this issue to track feedback around this issue and see what customers think.

[AArnott]

I consider this a feature. A build agent for example might define this file in the root of the enlistment drive to customize certain properties that the cloned repo builds with.

I wouldn't change this unless folks start complaining that it's a problem.

[rainersigwald]

Copied from comment by @jaredpar at #2008 (comment).

That seems like it is introducing unnecessary fragility into my build environment. Now the output, and possibly success or failure, of my build is dependent upon where on the machine a developer clones the repo. Looking at a repo on Github is no longer enough to understand how a build works, have to consider every single directory structure the code is cloned into.

If there is a stray Directory.Build.props file in their home directory then builds will suddenly, and quite silently, start changing. Will be quite difficult to track down.

This can be done both by simple developer accident or by misunderstood design. For the latter consider the act of including a .NET project via a submodule into your repo. If the outer repo has a root Directory.Build.props then there's really no way to safely submodule in another repo. Unless that repo has explicitly forbidden the use of any Directory.Build.props file.

Consider other tools which have a similar design of searching parent directories like editorconfig. They have a mechanism to stop the madness. Can put root=true to stop the searching. That mean you can at least add an .editorconfig to the root of a repo, set root=true and regain the ability to understand how the repo functions.

Is there such a feature here? Or do we just have to disable it entirely?

[jaredpar]

I consider this a feature. A build agent for example might define this file in the root of the enlistment drive to customize certain properties that the cloned repo builds with.

Which property do you imagine being changed here that would just work with anything but the most trivial of repos? I don't see how this is a realistic goal unless the repos involved specifically opted into being manipulated this way. There is way to much explicit setting of properties, hard coding of paths, etc ... for it to ever be change reliably in such a silent manner.

Consider the SDK as a concrete example here. Once you get past the structure change it's really just swapping out the core props / targets files you import. But it also makes very different decisions about defaults and overrides for a number of MSBulid properties. Recently been working on porting Roslyn to the SDK. The result wasn't a blissful silent change to our build system that absorbed all the new properties but rather a long week of digging through diagnostic logs and /pp output to understand what exactly changed.

This is definitely not a feature from our perspective, it's a liability. It introduces chance into a build we designed to be highly deterministic.

[AArnott]

To prevent this (and "fix" this issue) we'd need MSBuild to have an idea of the root of a repo. It doesn't have such a thing. And if we gave MSBuild the syntax support required such that the .targets could be hard-coded it to stop searching when it hit a .git folder, that would only solve the problem for git scenarios, and would fail even for them when on CI servers that don't create those folders, or folks who download the .zip of the source and build that. Then of course there is SVN, TFVC, and other SCC systems that leave no file artifacts on disk whatsoever. So this would be hard to solve, IMO.

@jaredpar, you brought up the difficulty of deterministic builds if this feature remains as-is. The build, I submit, is deterministic with this feature, in that it produces the same thing when built twice. It may not be the same as it would be in a different environment, but this is hardly the first or largest problem that makes that difficult. MSBuild has this gaping hole that I'm sure you're aware of in this area that environment variables are automatically inherited as MSBuild properties. Then there are toolsets and MSBuild extensions brought in with wildcard Imports and can vary across machines as well.

As opposed to all the foregoing problems with the environment a project builds within, this Directory.Build.props import is one of the most obvious. While environment variables are mostly "invisible" and MSBuild extensions also are, Directory.Build.props files are only going to be imported if you (or someone else on that machine) explicitly put them there. So it's highly unlikely that it will be by accident.

I might add a package reference in one such Directory.Build.props file on a build machine, for example, to make sure that all PDB files built get indexed onto http://symweb. That would be pretty sweet as it would make sure all real-signed builds are indexed as required, without having to make sure each individual repo owner added the reference that does this.
Another scenario is one of git submodules. I might host a submodule within a super-project and want my own Directory.Build.props to apply to the submodule. There is a coupled relationship here and it certainly is conceivable that this could be a Good Thing, and even by design for the submodule. Now, in the case of a git submodule, there is no .git folder in between these two modules, so in fact the "look for a .git folder and stop searching" approach wouldn't block this scenario anyway. And if folks think this is something we'd want to block, that reemphasizes my main objection to this GitHub issue: that it would be very difficult to implement.

I'm not too passionate about retaining the ability to import Directory.Build.props from outside a git repo. Honestly, I could take it or leave it fairly readily. But IMO it's a non-issue, and I am very concerned that trying to "fix" this would be more likely to stop searching ancestor directories too soon and break folks than it would be to actually avoid some problem of a user who left a stray Directory.Build.props file in their root directory for some reason other than that they wanted to do exactly this.

[jaredpar]

this is hardly the first or largest problem that makes that difficult

Agreed. But seems suspect to say "this is already a problem we've made difficult to solve so what's the harm in making it even harder?"

So it's highly unlikely that it will be by accident.

Sorry, disagree. In general it will be deliberate but there will be cases that arise which are accidental and extremely frustrating.

Another scenario is one of git submodules. I might host a submodule within a super-project and want my own Directory.Build.props to apply to the submodule

Sure. I still contend strongly this is never something that will work without the submodules explicit understanding. Builds are simply too fragile (see top of this comment) for silent changes to just work. There is always explicit buy off on where the cooperation needs to occur. Hence the idea of silently importing seems very suspect.

I'm not too passionate about retaining the ability to import Directory.Build.props from outside a git repo.

My main frustration is around the documentation of the feature. Specifically where it looks for the files, how many files it looks for, and how to stop it. Having it stop at the first file and not continue to arbitrarily import parent files is reasonable and sufficiently mollifies my other concerns.

[AArnott]

Having it stop at the first file and not continue to arbitrarily import parent files is reasonable and sufficiently mollifies my other concerns.

It does stop at the first file it finds. MSBuild's ability to import files from ancestor directories is only recursive till it finds the first match then it stops. If you want to keep importing more Directory.Build.props files as you go up, you have to add an Import of your own to the file you define.

So for that matter, if you want to protect your repo from importing files from outside its repo, just define a Directory.Build.props file at the root of your repo and that will cut it off.

[michael-hawker]

Wanted to revive this thread, as I've just been bitten by this.

I went into a sub-directory and did a file new WPF Core project (from the commandline or VS the outcome was the same), it wouldn't build:

There ended up being a Directory.build.props file farther up my folder structure from the parent project which was doing some globs and interfering with my build.

Of course moving the project out of the directory worked, but caused more confusion as the project was exactly the same.

It'd be really amazing if msbuild could emit a warning to call out that it has picked up Directory.build.* settings files from beyond the folder where it is run/the project or solution exists, E.g.:

c;\code\Graph-Controls\Samples\XAML Islands>dotnet run XAMLIslands.csproj
Warning: msbuild is using config c:\code\Graph-Controls\Directory.build.props from parent directory.
...

This would have at least given me some indication at the top of my VS error window that I should look somewhere else for the interference. As otherwise, I had no idea what was going on.

[nguerrera]

It would be exceptionally noisy to emit a warning whenever directory.build.props is used. It’s a very commonly used feature.

[michael-hawker]

@nguerrera I'm not saying to put it whenever it's used. Just to emit a warning if it's been pulled from above the working directory of the project/solution. There needs to be a heuristic that realizes that this may be unintentional.

It could even only emit the warning in the end if there were errors on the build. There just needs to be some indicator to the developer about what's being used to build their project, it can't just be a black box.

[nguerrera]

Just to emit a warning if it's been pulled from above the working directory of the project/solution.

To me, this is not very different from when used. It is extremely common to have it in parent directory for sharing between projects. That’s the main reason it exists. I would guess there’s far more of that than in the same directory as project.

[AArnott]

Also note, @michael-hawker, that the project doesn't (typically) know anything about the solution or solution directory. So emitting warnings based on the solution directory from a project's build isn't really feasible.

[michael-hawker]

@AArnott sure, but VS does and you can also build from the solution with the command line tools, so there are instances where it should be known.

In either case, there's a definite lack of transparency in process here that can cause issues. It'd just be great in the case the build fails to indicate to the developer more information about their configuration and how the project was built so that they can make better decisions to resolve their issue.

In my case above, I'm getting error messages and suggestions which are red-herrings and not the right course of action for me to take. But since they're the only indicators that get emitted, I don't know there's another influence that I should be investigating as the root of my issue.

Maybe the solution is that all new projects should have Directory.build.* files? IMHO file-new in an empty directory should always compile regardless (similar problems occur with MAX_PATH for certain other project types created in a deep directory, but you'd hope that wouldn't be the case by now).

[AArnott]

I really don't hear of many (or any, other than on this thread) customers hitting this problem of stray Directory.Build.props files causing problems. So for the few that do, the msbuild /pp:out.xml switch is an excellent diagnostic tool to understand what files are imported and where from. I see @jaredpar has already suggested that as well. IMO given the number of customers that do this regularly with success, it's best to equip the customers with tools to diagnose the problem when they hit it without introducing undue build warnings to the many customers that are using this properly.

[nguerrera]

msbuildlog.com is an invaluable resource when a build goes wrong for a reason that isn’t obvious. It does preprocessing (as /pp does) and much, much more. In terms of transparency of the full build process, it’s there for that when you need it.

[michael-hawker]

Thank you @AArnott and @nguerrera for the resources, they'll be very helpful in the future.

I agree that it's great to educate folks on how to help themselves. But it's also something that's not easily discoverable to know that these extra logging parameters are there and that there are advanced 3rd-party tools to analyze them (as you also need to then know how to analyze the log).

It's the type of thing where there needs to be an actionable thing to click off these build generated errors in VS that can open up that detailed build analysis to at least get the developer the extra info they require easily (or text at the end of an error-ed msbuild run that says to run again with logging to a human-readable format).

[livarcocc]

I agree that there are things we could do to make things more diagnosable. However, I also believe in the current design for Directory.Build.props/targets and that we are at a point now where we can close this issue and keep the design as is.