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

`dotnet <sln> add proj` should have good defaults for ProjectTypeGuid #5131

Closed
TheRealPiotrP opened this Issue Dec 22, 2016 · 8 comments

Comments

5 participants
@TheRealPiotrP
Collaborator

TheRealPiotrP commented Dec 22, 2016

@jgoshi

This comment has been minimized.

Show comment
Hide comment
Collaborator

jgoshi commented Dec 22, 2016

@jgoshi jgoshi self-assigned this Dec 23, 2016

@jgoshi

This comment has been minimized.

Show comment
Hide comment
@jgoshi

jgoshi Dec 23, 2016

Collaborator

Algorithm:

  1. Evaluate the project and get ProjectTypeGuid
  2. Evaluate the project and get DefaultProjectTypeGuid (depends on the work for the sdk mentioned above)
  3. if neither is available, fail with an error message.

Note that ProjectTypeGuid could have multiple guids. Choose the last one.
guid.Split(‘;’).Last()

Collaborator

jgoshi commented Dec 23, 2016

Algorithm:

  1. Evaluate the project and get ProjectTypeGuid
  2. Evaluate the project and get DefaultProjectTypeGuid (depends on the work for the sdk mentioned above)
  3. if neither is available, fail with an error message.

Note that ProjectTypeGuid could have multiple guids. Choose the last one.
guid.Split(‘;’).Last()

@jgoshi

This comment has been minimized.

Show comment
Hide comment
@jgoshi

jgoshi Jan 6, 2017

Collaborator

All the infrastructure on our end is in place (including commented out tests). The current behavior remains unchanged (we just add the CPS project type guid when we cannot figure out the correct type). Once the SDK change goes in to provide us with the DefaultProjectTypeGuid it should be a simple change to throw when ProjectTypeGuid and DefaultProjectTypeGuid is not set.

Collaborator

jgoshi commented Jan 6, 2017

All the infrastructure on our end is in place (including commented out tests). The current behavior remains unchanged (we just add the CPS project type guid when we cannot figure out the correct type). Once the SDK change goes in to provide us with the DefaultProjectTypeGuid it should be a simple change to throw when ProjectTypeGuid and DefaultProjectTypeGuid is not set.

@jgoshi

This comment has been minimized.

Show comment
Hide comment
@jgoshi

jgoshi Mar 9, 2017

Collaborator

In the master branch we have support for C# and VB. F# support is still in progress (see the comments near the end of this PR Microsoft/msbuild#1607)

Once F# support is available, do the following: Search our codebase for "issues/522". It should appear in the following three files.

  1. src\dotnet\ProjectInstanceExtensions.cs - Follow the instructions in the comment. Basically we want to throw the GracefulException if ProjectTypeGuid or DefaultProjectTypeGuid is not defined.

  2. test\dotnet-migrate.Tests\GivenThatIWantToMigrateSolutions.cs - Enable the ProjectTypeGuid check. Note that the code comment is wrong, we don't burn in the C# guid but rather use the appropriate one (C#, VB, or F#).

  3. test\dotnet-sln-add.Tests\GivenDotnetSlnAdd.cs - Enable the two tests (one is a Fact and the other is a Theory) which now will test the project type guid.

Collaborator

jgoshi commented Mar 9, 2017

In the master branch we have support for C# and VB. F# support is still in progress (see the comments near the end of this PR Microsoft/msbuild#1607)

Once F# support is available, do the following: Search our codebase for "issues/522". It should appear in the following three files.

  1. src\dotnet\ProjectInstanceExtensions.cs - Follow the instructions in the comment. Basically we want to throw the GracefulException if ProjectTypeGuid or DefaultProjectTypeGuid is not defined.

  2. test\dotnet-migrate.Tests\GivenThatIWantToMigrateSolutions.cs - Enable the ProjectTypeGuid check. Note that the code comment is wrong, we don't burn in the C# guid but rather use the appropriate one (C#, VB, or F#).

  3. test\dotnet-sln-add.Tests\GivenDotnetSlnAdd.cs - Enable the two tests (one is a Fact and the other is a Theory) which now will test the project type guid.

@jgoshi jgoshi assigned livarcocc and unassigned jgoshi Mar 9, 2017

@livarcocc livarcocc modified the milestones: 2.0.0, 2.1.0 May 19, 2017

@cwe1ss

This comment has been minimized.

Show comment
Hide comment
@cwe1ss

cwe1ss Jun 21, 2017

Will this also cover "Service Fabric Applications" (*.sfproj)? It seems like they are currently also added incorrectly.

cwe1ss commented Jun 21, 2017

Will this also cover "Service Fabric Applications" (*.sfproj)? It seems like they are currently also added incorrectly.

@peterhuene

This comment has been minimized.

Show comment
Hide comment
@peterhuene

peterhuene Mar 14, 2018

Member

I believe this is now addressed for F#: https://github.com/Microsoft/visualfsharp/blob/44fa027b308681a1b78a089e44fa1ab35ff77b41/src/fsharp/FSharp.Build/Microsoft.FSharp.NetSdk.props#L38 due to setting the default project type GUID. I've confirmed that adding an F# project to the solution results in the correct type GUID.

@cwe1ss It should work for any project type that sets DefaultProjectTypeGuid in their SDK.

However, we still need to address @jgoshi's comment above; we should be throwing GracefulException saying the project cannot be added to the solution because it doesn't have a ProjectTypeGuids set (see #7742; related bug reading the wrong property) or DefaultProjectTypeGuid coming from an SDK rather than defaulting to C# (that's just wrong behavior, period).

I'll see about re-enabling any needed tests as well.

@livarcocc, given that this is closely related to #7742, want me to take this issue from you?

Member

peterhuene commented Mar 14, 2018

I believe this is now addressed for F#: https://github.com/Microsoft/visualfsharp/blob/44fa027b308681a1b78a089e44fa1ab35ff77b41/src/fsharp/FSharp.Build/Microsoft.FSharp.NetSdk.props#L38 due to setting the default project type GUID. I've confirmed that adding an F# project to the solution results in the correct type GUID.

@cwe1ss It should work for any project type that sets DefaultProjectTypeGuid in their SDK.

However, we still need to address @jgoshi's comment above; we should be throwing GracefulException saying the project cannot be added to the solution because it doesn't have a ProjectTypeGuids set (see #7742; related bug reading the wrong property) or DefaultProjectTypeGuid coming from an SDK rather than defaulting to C# (that's just wrong behavior, period).

I'll see about re-enabling any needed tests as well.

@livarcocc, given that this is closely related to #7742, want me to take this issue from you?

@livarcocc

This comment has been minimized.

Show comment
Hide comment
@livarcocc

livarcocc Mar 14, 2018

Member

@peterhuene absolutely. It is yours.

Member

livarcocc commented Mar 14, 2018

@peterhuene absolutely. It is yours.

@livarcocc livarcocc assigned peterhuene and unassigned livarcocc Mar 14, 2018

peterhuene added a commit to peterhuene/cli that referenced this issue Mar 15, 2018

Fix project type GUIDs when adding projects to solution files.
This commit ensures the correct property (`ProjectTypeGuids`) is respected when
adding a project to a solution file.

Additionally, we now error if a project type GUID cannot be determined rather
than incorrectly mapping to the C# project type.

Enabled previously disabled tests that were waiting on upstream changes from
MSBuild and F#.

Fixes #5131.
Fixes #7742.

peterhuene added a commit to peterhuene/cli that referenced this issue Mar 15, 2018

Fix project type GUIDs when adding projects to solution files.
This commit ensures the correct property (`ProjectTypeGuids`) is respected when
adding a project to a solution file.

Additionally, we now error if a project type GUID cannot be determined rather
than incorrectly mapping to the C# project type.

Enabled previously disabled tests that were waiting on upstream changes from
MSBuild and F#.

Fixes #5131.
Fixes #7742.

peterhuene added a commit to peterhuene/cli that referenced this issue Mar 15, 2018

Fix project type GUIDs when adding projects to solution files.
This commit ensures the correct property (`ProjectTypeGuids`) is respected when
adding a project to a solution file.

Additionally, we now error if a project type GUID cannot be determined rather
than incorrectly mapping to the C# project type.

Enabled previously disabled tests that were waiting on upstream changes from
MSBuild and F#.

Fixes #5131.
Fixes #7742.

peterhuene added a commit to peterhuene/cli that referenced this issue Mar 20, 2018

Fix project type GUIDs when adding projects to solution files.
This commit ensures the correct property (`ProjectTypeGuids`) is respected when
adding a project to a solution file.

Additionally, we now error if a project type GUID cannot be determined rather
than incorrectly mapping to the C# project type.

Enabled previously disabled tests that were waiting on upstream changes from
MSBuild and F#.

Fixes #5131.
Fixes #7742.

peterhuene added a commit to peterhuene/cli that referenced this issue Mar 20, 2018

Fix project type GUIDs when adding projects to solution files.
This commit ensures the correct property (`ProjectTypeGuids`) is respected when
adding a project to a solution file.

Additionally, we now error if a project type GUID cannot be determined rather
than incorrectly mapping to the C# project type.

Enabled previously disabled tests that were waiting on upstream changes from
MSBuild and F#.

Fixes #5131.
Fixes #7742.

peterhuene added a commit to peterhuene/cli that referenced this issue Mar 20, 2018

Fix project type GUIDs when adding projects to solution files.
This commit ensures the correct property (`ProjectTypeGuids`) is respected when
adding a project to a solution file.

Additionally, we now error if a project type GUID cannot be determined rather
than incorrectly mapping to the C# project type.

Enabled previously disabled tests that were waiting on upstream changes from
MSBuild and F#.

Fixes #5131.
Fixes #7742.

peterhuene added a commit to peterhuene/cli that referenced this issue Mar 21, 2018

Fix project type GUIDs when adding projects to solution files.
This commit ensures the correct property (`ProjectTypeGuids`) is respected when
adding a project to a solution file.

Additionally, we now error if a project type GUID cannot be determined rather
than incorrectly mapping to the C# project type.

Enabled previously disabled tests that were waiting on upstream changes from
MSBuild and F#.

Fixes #5131.
Fixes #7742.

peterhuene added a commit to peterhuene/cli that referenced this issue Mar 23, 2018

Fix project type GUIDs when adding projects to solution files.
This commit ensures the correct property (`ProjectTypeGuids`) is respected when
adding a project to a solution file.

Additionally, we now error if a project type GUID cannot be determined rather
than incorrectly mapping to the C# project type.

Enabled previously disabled tests that were waiting on upstream changes from
MSBuild and F#.

Fixes #5131.
Fixes #7742.
@peterhuene

This comment has been minimized.

Show comment
Hide comment
@peterhuene

peterhuene Mar 23, 2018

Member

Fixed in #8796.

Member

peterhuene commented Mar 23, 2018

Fixed in #8796.

@peterhuene peterhuene closed this Mar 23, 2018

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