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

[.NET 6] Files copies into MonoBundle get folder structure flattened #12386

Closed
filipnavara opened this issue Aug 9, 2021 · 14 comments
Closed
Labels
bug If an issue is a bug or a pull request a bug fix dotnet An issue or pull request related to .NET (6) dotnet-pri0 .NET 6: required for stable release
Milestone

Comments

@filipnavara
Copy link
Contributor

Steps to Reproduce

  1. Build example project using dotnet build

Expected Behavior

MailClient.Import.Wab.resources.dll should be located in ./bin/Debug/net6.0-macos/osx-x64/macos-debug.app/Contents/MonoBundle/cs

Actual Behavior

MailClient.Import.Wab.resources.dll is located in ./bin/Debug/net6.0-macos/osx-x64/macos-debug.app/Contents/MonoBundle (path component is lost)

Environment

.NET 6 Preview 6

Build Logs

msbuild.binlog.zip

Example Project (If Possible)

Archive.zip

@rolfbjarne rolfbjarne added bug If an issue is a bug or a pull request a bug fix dotnet An issue or pull request related to .NET (6) dotnet-pri0 .NET 6: required for stable release labels Aug 9, 2021
@rolfbjarne rolfbjarne added this to the .NET 6 milestone Aug 9, 2021
@filipnavara
Copy link
Contributor Author

filipnavara commented Aug 10, 2021

I suppose the problem is somewhere here:

<!-- include .dll, .exe and, for debugging only, .pdb files inside the .app -->
<ResolvedFileToPublish
Update="@(ResolvedFileToPublish)"
RelativePath="$(_AssemblyPublishDir)\%(ResolvedFileToPublish.DestinationSubDirectory)\%(Filename)%(Extension)"
Condition="'%(Extension)' == '.dll' Or ('$(_BundlerDebug)' == 'true' And '%(Extension)' == '.pdb') Or '%(Extension)' == '.exe'" />
<!-- Copy the app.config file to the app bundle -->
<ResolvedFileToPublish
Update="@(ResolvedFileToPublish)"
RelativePath="$(_AssemblyPublishDir)\%(ResolvedFileToPublish.DestinationSubDirectory)\%(ResolvedFileToPublish.TargetPath)"
Condition="'$(AppConfig)' != '' And '%(ResolvedFileToPublish.OriginalItemSpec)' == '$(AppConfig)' And '%(ResolvedFileToPublish.Link)' == 'app.config' And '%(ResolvedFileToPublish.TargetPath)' != ''" />
<!-- .dylib are never needed (nor allowed) for fully AOT'ed applications FIXME https://github.com/xamarin/xamarin-macios/issues/11145 -->
<ResolvedFileToPublish
Update="@(ResolvedFileToPublish)"
RelativePath="$(_DylibPublishDir)\%(Filename)%(Extension)"
Condition="('$(_SdkIsSimulator)' != 'false' Or '$(_PlatformName)' == 'macOS' Or '$(_PlatformName)' == 'MacCatalyst') And '%(Extension)' == '.dylib'" />
<ResolvedFileToPublish
Update="@(ResolvedFileToPublish)"
RelativePath="$(_AssemblyPublishDir)\%(Filename)%(Extension)"
Condition="'$(_PlatformName)' != 'macOS' And '$(InvariantGlobalization)' != 'true' And '%(Filename)%(Extension)' == '$(_GlobalizationDataFile)'" />

The copied files have the .dll extension but they don't have DestinationSubDirectory property (unless it's somehow magically computed?), only TargetPath and RelativePath.

@rolfbjarne
Copy link
Member

This should do as a workaround:

<None Update="cs\*.dll">
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    <DestinationSubDirectory>cs</DestinationSubDirectory>
</None>

@filipnavara
Copy link
Contributor Author

Thanks. It's a bit tricky for us since we use **\*.dll wildcard but it's something we can figure out somehow.

@rolfbjarne
Copy link
Member

Would this work?

<None Update="cs\*.dll">
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    <DestinationSubDirectory>%(None.Directory)</DestinationSubDirectory>
</None>

@filipnavara
Copy link
Contributor Author

I seemed to have luck when changing this:

<ResolvedFileToPublish
Update="@(ResolvedFileToPublish)"
RelativePath="$(_AssemblyPublishDir)\%(ResolvedFileToPublish.DestinationSubDirectory)\%(Filename)%(Extension)"
Condition="'%(Extension)' == '.dll' Or ('$(_BundlerDebug)' == 'true' And '%(Extension)' == '.pdb') Or '%(Extension)' == '.exe'" />

to

			<ResolvedFileToPublish
				Update="@(ResolvedFileToPublish)"
				RelativePath="$(_AssemblyPublishDir)\%(ResolvedFileToPublish.RelativePath)"
				Condition="'%(Extension)' == '.dll' Or ('$(_BundlerDebug)' == 'true' And '%(Extension)' == '.pdb') Or '%(Extension)' == '.exe'" />

filipnavara added a commit to filipnavara/xamarin-macios that referenced this issue Aug 13, 2021
@rolfbjarne
Copy link
Member

This is a bit more complicated than just making sure the relative path is correct, because it touches upon a few important questions:

  • What should we actually copy to the app bundle? Your fix works for *.dll files, but that's just because we've special-cased *.dll files, and has nothing to do with None items that are configured to be copied to the output directory. What about this, should it also copy the file to the output directory?
<None Update="Something.txt">
    <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
  • What's the actual "output directory"? The root directory of the app bundle? Where assemblies are placed? Where resources are supposed to be? Some of these directories are different between iOS and macOS for instance.

For legacy Xamarin, we said to use either Content or BundleResource items to get files copied to the app bundle, which have a specific meaning and allow you to set where in the app bundle the file should be copied.

@filipnavara
Copy link
Contributor Author

filipnavara commented Aug 13, 2021

I'm not sure there is an uniformly correct answer to your questions. The default is the root of the app bundle and ResolvedFileToPublish seem to be relative to it unless the path gets updated by one of the rules linked above.

Since .dll files are special cased already it makes sense not to lose their path which is what my fix does. The follow up question is whether we should do the same for non-.dll files to keep the whole folder structure consistent. That may have some unforeseen consequences though.

The None idiom is not something I invented. It was widely used pattern in the WinForms world where this code comes from. I can change it to Content but I am pretty sure it would suffer from the same bug. The whole idea is to deploy pre-built satellite assemblies to be side-by-side with the application assemblies so the resource loader can find them. We actually consume this as a dependency from separate .csproj file which flows transitively into the application .csproj file.

@filipnavara
Copy link
Contributor Author

I can change it to Content but I am pretty sure it would suffer from the same bug.

Just to answer myself: Well, it does not... kinda... it puts it into Contents/Resources/cs. The relative path is preserved correctly but it's not a location where satellite assemblies are looked up.

@filipnavara
Copy link
Contributor Author

The BDN issue (#12418) is actually caused by using the same <None> idiom but for native libraries: https://github.com/microsoft/perfview/blob/main/src/TraceEvent/Microsoft.Diagnostics.Tracing.TraceEvent.props

@rolfbjarne
Copy link
Member

Since .dll files are special cased already it makes sense not to lose their path which is what my fix does.

I'm not sure I agree, I lean towards changing our current logic to not copy .dll files either (when they come from items with CopyToOutputDirectory set), and then require some other mechanism (such as using Content or BundleResource items instead).

I see three alternatives:


Enforce explicitness

This would be to only put Content and BundleResource files in the app bundle:

  1. Detect any files with CopyToOutputDirectory set and give a warning (telling the developer it won't work, and suggest to use another alternative)
  2. Document the alternatives, in particular how to specify where in the app bundle the files should go.

The downside is of course that nothing that currently uses CopyToOutputDirectory will work. The other solutions will at least work sometimes.


Define "output directory" to something

We could define the output directory for CopyToOutputDirectory to be where the managed assemblies are located. This would work for satellite assemblies, and any other logic that expects files next to the assemblies.

OTOH if you do this for resources, you won't get the behavior you want (and you'll have to use Content/BundleResource instead).


Guess

Come up with rules like:

  • .dll, .exe: next to the assemblies
  • Other files: with the resources.

This raises more questions though (what about MyAssembly.dll.config? MyAssembly.xml? MyAssembly.pdb?), and seems brittle and unnecessarily complex.

@filipnavara
Copy link
Contributor Author

I did a cursory GitHub search and checked few pages of the usages. Then I did the same for our sources (including 3rd-party libraries) and for some StackOverflow answers. Apparently my expectations about the usage were somewhat distorted but there are three main usages that are common:

  1. Copying resource-like assets into the target directory (with expectation to have them next to main assembly)
  2. Deploying .dll files along with application
    • Native libraries
    • Satellite assemblies
  3. Copying something like readme.txt or license.txt that is not consumed by the application directly

Whatever is the final decision I would expect the behavior of net6.0-macos to be as close as possible to net6.0, ie. adding the consumption of macOS bindings should have minimal impact on code that worked on cross-platform app. However wrong it may seem that leads me to believe that the correct option is "define output directory to be where managed assemblies are located". That seems to be the only option that doesn't break either 1) or 2).

@rolfbjarne
Copy link
Member

Another related issue is what to do about frameworks: #12440 (comment) - that issue in particular is about multiple frameworks in the same NuGet, but a related point is where to put frameworks in general. We might end with a mix of "Guess" and "Define 'output directory' to something":

  • For any file of the format *.framework/*, put in the Contents/Frameworks directory
  • Any other file gets put next to the managed assemblies.

@rolfbjarne
Copy link
Member

And this should be considered as well: #11667

@rolfbjarne
Copy link
Member

I believe this to be the same as #12572 (which is a more general issue), so I'm closing this one in favor of that one.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug If an issue is a bug or a pull request a bug fix dotnet An issue or pull request related to .NET (6) dotnet-pri0 .NET 6: required for stable release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants