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

Throw an error if the Sdk attribute contains any expandable characters #1518

Open
jeffkl opened this issue Jan 6, 2017 · 7 comments
Open
Milestone

Comments

@jeffkl
Copy link
Contributor

jeffkl commented Jan 6, 2017

Right now the Sdk value is treated as a literal string with no expansion. If a user specifies something like:

<Project Sdk="$(Property1)" />

Then the value "$(Property1)" is passed to the resolver which would probably return a message stating that it couldn't be found. We should instead check if the value contains expandable characters and throw an invalid project error.

@jeffkl jeffkl added this to the Feature: Sdks milestone Jan 6, 2017
@jeffkl jeffkl self-assigned this Jan 6, 2017
@Nirmal4G
Copy link
Contributor

Nirmal4G commented Sep 14, 2017

What about Import tags' Sdk attribute?

For e.g.:

<Import Sdk="$(DynamicSdk)" />

It'll be useful in an injection scenario!

Does it work? If not, will it be supported?

@jeffkl
Copy link
Contributor Author

jeffkl commented Sep 15, 2017

At this time the Sdk attribute in an <Import /> element is not expanded so using properties in its value will not work as expected. The top implicit import when defining <Project Sdk="Microsoft.NET.SDK />` is evaluated before a user can define any properties so we decided to save the overhead. However, if you change your projects to use explicit imports, then it might make sense to use properties. You would still need to import another central file manually or define a property in every project to make it useful.

<Project>
  <!-- common.props defines $(DynamicSdk) -->
  <Import Project="..\common.props" />
  <Import Project="Sdk\Sdk.props" Sdk="$(DynamicSdk)" />
    ...
  <Import Project="Sdk\Sdk.targets" Sdk="$(DynamicSdk)" />
</Project>

Every time you made a new project you'd have to hand edit it and add the imports.

@Nirmal4G
Copy link
Contributor

You would still need to import another central file manually or define a property in every project to make it useful. Every time you made a new project you'd have to hand edit it and add the imports.

Well, that may be so but not exactly.

  1. I can do this via another SDK meta-package that references all other SDKs.
  2. If I am using a local build then Directory.Build.props would be the best way to import the SDK properties.
  3. Another way is to have default SDK defined but override them through msbuild my.proj /p:DynamicSdk=The.Sdk

These are the ones I'm currently dealing with.
Thanks for the clarification.

@jeffkl
Copy link
Contributor Author

jeffkl commented Sep 15, 2017

I can do this via another SDK meta-package that references all other SDKs.

Can you explain this?

If I am using a local build then Directory.Build.props would be the best way to import the SDK properties.

Directory.Build.props is imported by Microsoft.Common.props which is imported by Sdk.props. Are you suggesting doing something like:

<Project Sdk="Microsoft.NET.Sdk">
  <Import Project="Sdk\Sdk.props" Sdk="$(DynamicSdk)" />
</Project>

If you're adding an import to every project, it would be just as easy to just do:
<Import Project="$(DynamicImport)" />

Another way is to have default SDK defined but override them through msbuild my.proj /p:DynamicSdk=The.Sdk

You would still need to default DynamicSdk in case you didn't set a global property via the command-line. Setting the property before the <Import /> is evaluated would be tricky. The only solution I could think of would be:

<Import Project="Sdk\Sdk.props" Sdk="$([MSBuild]::ValueOrDefault($(DynamicSdk), 'Microsoft.NET.Sdk))" />

That seems less then ideal

@Nirmal4G
Copy link
Contributor

Nirmal4G commented Sep 15, 2017

Directory.Build.props is imported by Microsoft.Common.props which is imported by Sdk.props. Are you suggesting doing something like:

<Project Sdk="Microsoft.NET.Sdk">
  <Import Project="Sdk\Sdk.props" Sdk="$(DynamicSdk)" />
</Project>

Yes. But within the Base Sdk itself. For e.g. say I have two types of Packaging or Bundling targets like nuget. I would do this

<Project Sdk="Custom.NET.Sdk">
 ...
<BundlingSystem>CabBundler</BundlingSystem>
 ...
</Project>

Note: Custom.NET.Sdk imports Core SDK like Microsoft.NET.Sdk!

Then in the Base SDK, I would resolve the above property to SDK name and set the property to that resolved Sdk package. For e.g. In the props and targets of the Custom.NET.Sdk I would do this

<Project>
...
<ResolvedBundlerSdk Condition="'$(BundlingSystem)' == 'CabBundler'">Bundler.Cab.Sdk</ResolvedBundlerSdk>
<Import Project="Sdk\Sdk.props" Sdk="$(ResolvedBundlerSdk)" />
...
</Project>

Little bit of something I'm working on.

@Nirmal4G
Copy link
Contributor

This is not setting different global SDKs but using different SDKs with specific functions within the master SDK without creating a duplicate set of targets as a nuget package.

For e.g.: NuGet team can have one master Sdk that does have all the tasks and targets but can also have a separate targets (Sdk) package that is referenced by the master package. And also by other teams that want the functionality like Packaging but disabling the other features that comes with master Sdk.

dotnet/sdk#889 (comment)
dotnet/sdk#889 (comment)

@Nirmal4G
Copy link
Contributor

I can do this via another SDK meta-package that references all other SDKs.
Can you explain this?

<Project>
...
<Import Project="Sdk\Sdk.targets" Sdk="1.Sdk" />
<Import Project="Sdk\Sdk.targets" Sdk="2.Sdk" />
<Import Project="Sdk\Sdk.targets" Sdk="3.Sdk" />
...
</Project>

OR

<Project>
...
<Sdk Name="1.Sdk" Version="1.0" />
<Sdk Name="2.Sdk" Version="2.1" />
<Sdk Name="3.Sdk" Version="3.2" />
...
</Project>

If this is possible. Even cleaner!

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

No branches or pull requests

2 participants