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

Cannot include dialog files in a package that contains code generated from a csproj #1201

Closed
garypretty opened this issue Apr 16, 2021 · 9 comments
Assignees
Labels
Documentation P0 Must Fix. Release-blocker
Milestone

Comments

@garypretty
Copy link

Describe the bug

Currently the package manager looks for an 'exported' folder in the root of a NuGet package to find any dialog / declarative files that should be installed into the bot project. However, if a NuGet package is generated from a CSPROJ file, which they often are (including in the community repo), the 'exported' folder will not be at the root, it will be located at 'content/exported'.

Also, without this change, it won't be easy to have a package that contains both dialog files and code without a bunch of additional work for the developer.

Note: I realize that a content only NuGet package can, and arguably should, be created using a nuspec file (so that the unnecessary dlls are not included), but we need to address the above to allow for mixed packages. This will be a blocker for the Teams / Channel Handoff Package currently in development, which will likely contain mixed content.

To Reproduce

Try installing the Bot.Builder.Community.Components.Dialogs.GetWeather package from the Community repo.

Expected behavior

When installing Bot.Builder.Community.Components.Dialogs.GetWeather, we should look for the exported folder within the content folder of the package and not just at the root.

Potentially related to microsoft/BotFramework-Composer#6994

@rvinothrajendran
Copy link

rvinothrajendran commented Apr 16, 2021

Look like I am facing the same issue in "Bot.Builder.Community.Dialogs.CustomAction" which contains CS + schema files

In my pc changed to BotComponent registration( community repro old concept) .. and Install the package .. no information in the bot composer ( package is not installed ) but in visual studio "Managed NuGet package" showing as package has installed.

@rvinothrajendran
Copy link

Raised one more bug in components projects microsoft/botframework-components#913 (but not sure related to this)

@garypretty garypretty transferred this issue from microsoft/BotFramework-Composer Apr 16, 2021
@garypretty
Copy link
Author

Look like I am facing the same issue in "Bot.Builder.Community.Dialogs.CustomAction" which contains CS + schema files

In my pc changed to BotComponent registration( community repro old concept) .. and Install the package .. no information in the bot composer ( package is not installed ) but in visual studio "Managed NuGet package" showing as package has installed.

I think this is a separate issue where packages without schema are not showing as installed, which we are tracking. It's not this issue.

@peterinnesmsft
Copy link

Hi @rvinothrajendran and @garypretty,

I've investigated this issue and, based on the outlined scenario, I've written some basic sample code that shows how to create a NuGet package that contains a combination of custom dlls and declarative content. You can find said code here: https://github.com/peterinnesmsft/CustomComponent (Should be public, let me know if you don't have access)


The basic gist of this is that, when using dotnet pack to create a NuGet package from a .csproj, any Content items in the project are handled in a special manner and by default get copied into the 'content' and 'contentFiles' directories created in the root of the package. It is possible to override this behavior by using the ContentTargetFolders MSBuild property in your .csproj, as described here.

Given that the goal here is really just to copy the exported directory and its contents as-is to the package root, the simplest way to do this is to use the following in your .csproj (see the sample code I included above):

<ItemGroup>
  <None Include="exported/**/*.*" Pack="true" PackagePath="exported" />
</ItemGroup>

This will mark all files under the exported directory as the None item type, meaning they won't default to the behavior applied to Content items when being packaged. The Pack="true" attribute is required to force these items to be included, as by default None items are not included in the generated package. Finally, PackagePath instructs packaging to include the contents of the exported folder in the package root underneath 'exported'.

This should enable you to have both custom dlls packaged to the standard locations while enabling declarative content to be output at the right location in the package.


@rvinothrajendran please take a look at the above and sample code I have included, and try making a similar modification to your own code to see if this gets you moving. If you run into any issues, let me know.

@peterinnesmsft peterinnesmsft self-assigned this Apr 22, 2021
@sgellock sgellock added this to the R13 milestone Apr 22, 2021
@sgellock sgellock added Documentation P1 Painful if we don't fix, won't block releasing labels Apr 22, 2021
@sgellock
Copy link
Member

I think this is going to end up being a documentation issue, meaning how it should be done would need to include @peterinnesmsft guidance. let's see if we get an ACK, and if so, we'll need to make sure @clearab and @Kaiqb are aware as they pull docs together on this topic.

I've also made this a P1, only because I've made the assumption Peter's guidance is correct. If we feel different and want to bump it up, go for it

@clearab
Copy link

clearab commented Apr 22, 2021

I agree - this aligns with how I was planning to document this.

@sgellock sgellock added P0 Must Fix. Release-blocker and removed P1 Painful if we don't fix, won't block releasing labels Apr 22, 2021
@sgellock
Copy link
Member

ok. talked with @garypretty and talked this over with @tomlm and I'm agreeing with the original issue framing. I've added @chrimc62 and changed the priority to P0. we want to get this right before we go GA.

@chrimc62
Copy link
Contributor

I will go ahead and implement checking for content\exported as well. I will not check for contentFiles<framework>\exported since that is a bigger change. content\exported is supported for "old" nuget and gets generated when you mark files as content in csproj so it should solve the immediate problem. If someone thinks we should support framework specific content files, please let me know ASAP.

@chrimc62
Copy link
Contributor

The current behavior is "by design" as the least confusing option and one that makes writing packages a little harder, but consuming them easy and clearer.

The problem with marking files as "content" is that they are considered to be read-only and part of the nuget package. We copy the files and they become part of your project and can be changed. Visual Studio also has magic that makes content files from packages appear like they are "part of your project". This is even more confusing because you have an exported folder which contains things which are not being exported (from packages) next to things which are being exported (if you are building a package.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation P0 Must Fix. Release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants