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

Migration: handle project.json "shared" key for internal types access #7514

Closed
jgoshi opened this issue Jan 13, 2017 · 13 comments
Closed

Migration: handle project.json "shared" key for internal types access #7514

jgoshi opened this issue Jan 13, 2017 · 13 comments
Assignees
Milestone

Comments

@jgoshi
Copy link
Contributor

jgoshi commented Jan 13, 2017

No description provided.

@livarcocc
Copy link
Contributor

Can you elaborate on this issue and add repro steps?

@jgoshi
Copy link
Contributor Author

jgoshi commented Jan 14, 2017

Open the attached project. In the project.json world, the "shared": "*.cs" line in the TestLibrary project.json file allows it's internal class to be used in TestApp. After migration we cannot access the internal Helper class and the build fails.
internal.zip

@DustinCampbell
Copy link
Member

That's the first I've heard of that feature. 😄 Is this supported in a .csproj world yet?

@DustinCampbell
Copy link
Member

I guess we'd link them?

@jgoshi
Copy link
Contributor Author

jgoshi commented Jan 17, 2017

There's InternalsVisibleToAttribute, but that's only to specify which friend assemblies can reference internals. I don't know of a way to say all internals are visible to all other assemblies which is what this PJ statement seems to say. I think we should just not support this feature :)

@DustinCampbell
Copy link
Member

DustinCampbell commented Jan 17, 2017

I suspect it's probably the right to just link the files in MSBuild. In fact, that's probably what "shared" is doing. It's akin to Shared Projects in VS. Something like...

<Compile Include="..\TestLibrary\Helper.cs">
    <Link>Helper.cs</Link>
</Compile>

@NTaylorMullen
Copy link
Contributor

Hmm, linking them wont be sufficient. In the old world NuGet or dotnet cli (not sure which one was responsible) would special case the packing of a project that had the "shared" property and build a nupkg that had content files that would then be copied into users projects when restored. If we simply link the cs files it doesn't fix the ability to pack and re-use the shared project.

Support for "shared" was dropped in favor of NuGet/Home#3683. If there's a way we can transition the existing "shared" project to utilize the new NuGet feature then we'd be feature complete; otherwise, I'd recommend failing dotnet migrate when a "shared" property is present.

@DustinCampbell
Copy link
Member

Hmmm... If I I understand you correctly it sounds like the files would also have to be linked in order to compile a project that referenced and used "shared" files from another project, correct? That is, there's two things we'd need to do in order to fully support the shared property: both linking files and doing some other work to support this new NuGet feature. Is that right?

@NTaylorMullen
Copy link
Contributor

@DustinCampbell correct.

  1. Link files so P2P references work.
  2. Transition the project to use the new contentFiles support in nuget generated targets file NuGet/Home#3683 feature so its packed representation was functionally compatible with what was there in project.json.

@krwq krwq self-assigned this Jan 26, 2017
@livarcocc
Copy link
Contributor

Ok, I want to clarify something here.

If I have two projects: project A and Project B and project B has shared: **/*.cs in its project json.

When we migrate these projects, does that mean that we need to translate that shared property in projectB into a bunch of Links in projectA? If so, I am not sure we should take this fix for RTM.

This would mean that migration would require a project's dependencies project.json to still be around, so that we could inspect it and we actually removed that requirement because this can't happen in VS, since VS handles its own backup and it moves the files to backup as each project gets migrated.

@NTaylorMullen
Copy link
Contributor

What does the old project.json based dotnet do?

@livarcocc
Copy link
Contributor

@krwq We are not going to take this for RTM. This would require the dependencies for a project to ling around so that we can look at them and see if we need to include their files. We currently can't do that because of VS, since it moves the PJ to backup individually as it migrates.

We could introduce a parameter for VS to pass to us, but it does not meet the bar right now.

@livarcocc
Copy link
Contributor

This issue was moved to dotnet/cli-migrate#35

@msftgits msftgits transferred this issue from dotnet/cli Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
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

6 participants