Skip to content
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

Merging of multiple Directory.Build.props #2456

Closed
grokky1 opened this issue Aug 21, 2017 · 17 comments
Closed

Merging of multiple Directory.Build.props #2456

grokky1 opened this issue Aug 21, 2017 · 17 comments

Comments

@grokky1
Copy link

grokky1 commented Aug 21, 2017

I'm trying to understand whether how/if this file is merged with others in the solution tree, and at what point in the build process.

From the guide it's not clear to me whether merging occurs? The section "Search scope" explains that it "stops" once it finds the file. However I'm under the impression that my file is merged with the default stuff for msbuild, so some kind of merging does occur.

Suppose I have this

\
  MySolution.sln
  Directory.Build.props    // I'd like this to apply to all child projects (1)
  \src
    Directory.Build.props  // I'd like this to apply only to child src projects (2-src)
    \Project1
    \Project2
  \test
    Directory.Build.props  // I'd like this to apply only to child test projects (2-test)
    \Project1Tests
    \Project2Tests

Then for any given project, does msbuild

  • find the innermost one (2-src / 2-test) and "stop",
  • OR will it merge with the one at the solution level (1) ...which is what I'm hoping for?

If it doesn't merge like I've described, is it possible to do somehow?

@rainersigwald
Copy link
Member

It finds the innermost one and stops. However, it's possible to manually opt into continuing: just add

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props'))" />

to your "innermost" Directory.Build.props. That mimics the logic Microsoft.Common.props uses to import the first d.b.props, but it can be simpler because you don't need an "off switch" since you already know that you want to load it, and that it exists.

@grokky1
Copy link
Author

grokky1 commented Aug 21, 2017

@rainersigwald Thank you for that!

Does it "stop" after it merges into the solution-level file, or will it continue till it gets to /? (Which is of course undesirable.)

I ask because I'm not sure I understand your "off switch" comment, and I've looked at the source but I don't really understand how msbuild works behind the scenes...

@rainersigwald
Copy link
Member

It stops at the first Directory.Build.props it finds while recursing upward. If you want to ensure that you don't include non-repo state, put a d.b.props in your repo root.

For "off switch" what I meant is that the common targets allow two customization points for d.b.props:

You don't need either of those to continue the search, because you already know that you're actively importing Directory.Build.props.

@grokky1
Copy link
Author

grokky1 commented Aug 21, 2017

Okay so for anyone else like me who is a msbuild amateur, I summarize all your explanations thus:

  • for any given project, msbuild finds the first Directory.Build.props upward in the solution structure, merges it, and stops scanning for more
  • if you want multiple levels to be found and merged, then <Import...> (shown above) the "outer" file from the "inner" file
  • if the "outer" file does not itself also import something above it, then scanning stops there
  • to control the scanning/merging process, use $(DirectoryBuildPropsPath) and $(ImportDirectoryBuildProps) (shown above)

...or more simply: the first Directory.Build.props which doesn't import anything, is where msbuild stops!

@rainersigwald
Copy link
Member

An excellent summary! Would you like to open a PR for the docs with that information?

@grokky1
Copy link
Author

grokky1 commented Aug 24, 2017

Turns out that

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props'))" />

doesn't work - I get an error that the import causes a circular dependency.

However when I hardcode it then it works:

<Import Project="../Directory.Build.props" />

I think it has something to do with the starting directory, which doesn't correspond to what I would assume. If the csproj file automatically imports the dbprops above it (the "inner" file), which in turn imports the dbprops above it (the "outer" file), then the import command essentially points to the inner file, i.e. itself, which causes the circular dependency. So I tried this:

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '../'))" />

But that doesn't help.


UPDATE:
Looks like the correct way to avoid a circular dependency is:

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />

I've created a new PR for that.

@dasMulli
Copy link
Contributor

@grokky1
Copy link
Author

grokky1 commented Aug 31, 2017

@dasMulli Yep I put a link in the SO question to this issue, was sure you saw it... I thought I'd get a quick resolution, and I did 😉

Thanks again BTW. The PR has your proposed change.

@grokky1 grokky1 closed this as completed Sep 21, 2017
@ViktorHofer
Copy link
Member

Turns out that <Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props'))" /> doesn't work - I get an error that the import causes a circular dependency

@rainersigwald that sounds like a bug. GetPathOfFileAbove should not attempt to load itself again. I'm also hitting this in corefx.

@dasMulli
Copy link
Contributor

dasMulli commented Mar 1, 2019

@ViktorHofer unfortunately, this would be a breaking change.

While unfortunate for nested msbuild files, the current behavior semantically aligns with most other behaviors of MSBuild - all relative paths start at the project direcotry, project directory is the CWD for all code being executed by default etc. So GetPathOfFileAbove() could also fetch me a customConfig.json from a consumer solution out of targets i could distribute as MSBuild SDK.

@ViktorHofer
Copy link
Member

I'm not proposing to change the default directory GetPathOfFileAbove starts to search from. What I tried to say is that GetPathOfFileAbove should not return the path of the calling script as that doesn't make any sense.

@dasMulli
Copy link
Contributor

dasMulli commented Mar 1, 2019

Depends on how you look at it.. if you see it from the context of the project file, it's okay.
For this issue to occur, three things have to happen:

  1. You're using GetPathOfFileAbove() to locate props/targets files. There are use cases outside of that (config files for instance)
  2. You're looking for a file named like the msbuild file evaluating the function.
  3. The result is used for an <Import>.

I think it would be weird for a "find me a file in the project directory hierarchy named X" function to have a special case ".. but not if it the path is the same as the currently executing msbuild file". Seems like a hardcoded workaround for the combination of the 3 conditions listed above.

Personally, i'd prefer an additional property on <Import> to silently skip files already imported - this would help in all sorts of other scenarios as well.

@rainersigwald
Copy link
Member

I agree with @dasMulli, except for this part:

Personally, i'd prefer an additional property on <Import> to silently skip files already imported - this would help in all sorts of other scenarios as well.

I don't think it'd help here, because it wouldn't follow the user intent ("recurse upward")--it'd just silently end the search.

Another possible improvement would be to put this logic directly in Import; something like

<Import Project="Directory.Build.props" Search="Above" />

(undoubtedly better wording is possible). Then the case of "continue D.B.props search" would be really concisely expressed. But I'm not sure it's worth the additional complexity.

@dasMulli
Copy link
Contributor

dasMulli commented Mar 1, 2019

true!

for some usages one could also use a function that returns an ordered list of project files in the tree and make use of the list import that was introduced during 15.*. But that would remove the option to explicitly block traversal.

@ViktorHofer
Copy link
Member

Okay, let me summarize my ask. What I'm missing is a functionality to import a file - with the same name as the calling script - somewhere in the directory structure above. A search attribute on the import element itself would be very convenient.

@springy76
Copy link

I don't get the syntax of GetPathOfFileAbove

This property function has the following syntax: $([MSBuild]::GetPathOfFileAbove(dir.props))

So where does this second parameter come from?

@danmoseley
Copy link
Member

@rainersigwald one way to enable @ViktorHofer 's scenario could be to special case GetPathOfFileAbove in the case when the current file has the same name that's being requested.

Right now that's failing the build, so this would not be a breaking change. It would be taking syntax that right now has no valid meaning, and making it useful. It would allow us and others using GetDirectoryNameOfFileAbove to find same-named files above to be able to move to the simpler syntax. Given the function has the word "above" in it I don't think it's reasonable that in this case it starts looking "above".

thoughts? It's a bit of a shame this doesn't work since it was explicitly the use case I described in my original proposal for GetPathOfFileAbove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants