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

Prevent double-import warning for Microsoft.Common.props #5747

Closed
wants to merge 1 commit into from

Conversation

MattGal
Copy link
Member

@MattGal MattGal commented Jul 6, 2020

Noted from the conversation here

@MattGal MattGal requested a review from alexperovich July 6, 2020 16:54
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to do the same thing 😺

@alexperovich
Copy link
Member

alexperovich commented Jul 6, 2020

This.... shouldn't really be needed at all. A helix sdk project doesn't use the 'Microsoft.NET.Sdk' and projects made with the helix sdk shouldn't ever import it.

@dougbu
Copy link
Member

dougbu commented Jul 6, 2020

@alexperovich without this conditionality, features such as automated .NET SDK installation on Helix agents are pretty much useless. We need to know $(NETCoreSdkVersion) when setting $(DotNetCliVersion) and using $(IncludeDotNetCli). That means we need to import all of the .NET SDK's SDK.props, not the small part of it the Helix SDK imports.

@MattGal
Copy link
Member Author

MattGal commented Jul 6, 2020

@alexperovich without this conditionality, features such as automated .NET SDK installation on Helix agents are pretty much useless. We need to know $(NETCoreSdkVersion) when setting $(DotNetCliVersion) and using $(IncludeDotNetCli). That means we need to import all of the .NET SDK's SDK.props, not the small part of it the Helix SDK imports.

I think @alexperovich is suggesting full deletion of the line. I prefer to not find out that we did actually need it later on, but I'll chat w/ Alex offline and get this done one way or the other.

@alexperovich
Copy link
Member

I am not suggesting full deletion of the line. The helix sdk requires microsoft.common.props and targets to work. It is not designed to work alongside another SDK that also imports them. We can add this condition, but there are no guarantees that anything will work in that scenario.

@dougbu
Copy link
Member

dougbu commented Jul 6, 2020

What alternative do you suggest @alexperovich to make $(IncludeDotNetCli) useful (without ugly hacks and manual changes to multiple files when the SDK version changes)❔

@MattGal
Copy link
Member Author

MattGal commented Jul 6, 2020

After discussion with @alexperovich it seems that it's preferable for teams needing to do this to suppress the warning, abandoning.

@MattGal MattGal closed this Jul 6, 2020
@dougbu
Copy link
Member

dougbu commented Jul 6, 2020

Why is it preferable to suppress the warning❔ I thought @alexperovich was saying what we need to do won't work and we might as well give up on $(IncludeDotNetCli) until something else changes.

@alexperovich
Copy link
Member

It looks like we can grab that version without importing the entire sdk. I will look into it.

@dougbu
Copy link
Member

dougbu commented Jul 6, 2020

I will look into it.

Much appreciated❕

@alexperovich
Copy link
Member

#5748

@riarenas riarenas deleted the suppress-double-import-warning branch August 21, 2020 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants