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

Introduce targeting properties #12161

Merged
merged 2 commits into from
Jan 11, 2023
Merged

Conversation

mmitche
Copy link
Member

@mmitche mmitche commented Jan 10, 2023

Targeting implementation of https://github.com/dotnet/arcade/pull/11903/files. Adds three properties to allow repos to defer some/all of their target framework maintenance to arcade. Arcade already had a file like this for its own purposes. I've removed that and hoisted arcade's targeting property into its Directory.Build.props.

To double check:

Targeting implementation of https://github.com/dotnet/arcade/pull/11903/files.
Adds three properties to allow repos to defer some/all of their target framework maintenance to arcade.
Arcade already had a file like this for its own purposes. I've removed that and hoisted arcade's targeting property into its Directory.Build.props.
@mmitche mmitche requested review from MichaelSimons, tkapin and a team January 10, 2023 22:22
<PropertyGroup>
<PropertyGroup>
<NetCurrent>net8.0</NetCurrent>
<NetSupported>net7.0;net6.0</NetSupported>
Copy link
Member

Choose a reason for hiding this comment

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

We need to split those apart as otherwise repos like dotnet/runtime that use platform tfms couldn't use them, i.e.:

<TargetFrameworks>$(NetCurrent)-windows;$(NetCurrent);$(NetPrevious)-windows;$(NetPrevious);$(NetMinimum)-windows;$(NetMinimum)</TargetFrameworks>

https://github.com/dotnet/runtime/blob/7c265c396e6ef31cb0d588853834c800e40ecdef/src/libraries/System.Windows.Extensions/src/System.Windows.Extensions.csproj#L3

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

- NetSupported - Supported, released versions of .NET
- NetCurrentAndSupported - Supported, released versions of .NET and the current version.

Repos can choose set their TargetFrameworks properties using NetCurrent or NetSupported, potentially in conjunction with other target frameworks a project might
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should avoid defining properties that concatenate multiple TFMs as that makes it really hard to reason about their inner builds and it doesn't work when targeting platforms or RIDs. Today, the TargetFrameworks string tells you the number of inner builds and what it targets. As someone who spends a large amount of time in project files, I would like to keep that metric.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

<?xml version="1.0" encoding="utf-8"?>
<!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. -->
<Project>
<!-- Repositories using the arcade SDK can stay up to date with their target framework more easily using the properties in this file.
Copy link
Member

Choose a reason for hiding this comment

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

We should put this into the guide for onboarding Arcade so that new repos also copy this file and what it does.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that we should put this into the Onboarding Arcade guide but there's nothing to copy as this file is part of the Arcade.Sdk. Repositories will have access to these properties by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I put it into the Arcade Sdk documentation already:

https://github.com/dotnet/arcade/pull/12161/files#diff-ade115018a157765907cf3140eab34463b66acb4dfe276174c78ab870e260709R946-R953

I did not choose the onboarding guide because onboarding isn't that common any longer.

Comment on lines +950 to +953
- NetCurrent - The TFM of the major release of .NET that the Arcade SDK aligns with.
- NetPrevious - The previously released version of .NET (e.g. this would be net7 if NetCurrent is net8)
- NetMinimum - Lowest supported version of .NET the time of the release of NetCurrent. E.g. if NetCurrent is net8, then NetMinimum is net6
- NetFrameworkMinimum - Lowest supported version of .NET Framework the time of the release of NetCurrent. E.g. if NetCurrent is net8, then NetFrameworkMinimum is net462
Copy link
Member

Choose a reason for hiding this comment

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

@jkotas do those property names make sense to you? Their naming inspiration comes from dotnet/runtime.

Copy link
Member

Choose a reason for hiding this comment

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

The names look reasonable to me.

Nit: It is not obvious from the property name that the property value is TFM. It is the usual name verbosity tradeoff. Since this is for our internal use only, I do not see a problem with the current names.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

LGTM but I would like to hear @jkotas opinion before merging.

@mmitche mmitche merged commit 9a4d4f5 into dotnet:main Jan 11, 2023
@mmitche mmitche deleted the common-framework-props branch January 11, 2023 22:40
mmitche added a commit to mmitche/arcade that referenced this pull request Feb 17, 2023
* Introduce targeting properties
Targeting implementation of https://github.com/dotnet/arcade/pull/11903/files.
Adds three properties to allow repos to defer some/all of their target framework maintenance to arcade.
Arcade already had a file like this for its own purposes. I've removed that and hoisted arcade's targeting property into its Directory.Build.props.
mmitche added a commit to mmitche/arcade that referenced this pull request Feb 17, 2023
* Introduce targeting properties
Targeting implementation of https://github.com/dotnet/arcade/pull/11903/files.
Adds three properties to allow repos to defer some/all of their target framework maintenance to arcade.
Arcade already had a file like this for its own purposes. I've removed that and hoisted arcade's targeting property into its Directory.Build.props.
mmitche added a commit to mmitche/arcade that referenced this pull request Feb 17, 2023
* Introduce targeting properties
Targeting implementation of https://github.com/dotnet/arcade/pull/11903/files.
Adds three properties to allow repos to defer some/all of their target framework maintenance to arcade.
Arcade already had a file like this for its own purposes. I've removed that and hoisted arcade's targeting property into its Directory.Build.props.
mmitche added a commit that referenced this pull request Feb 17, 2023
* Introduce targeting properties
Targeting implementation of https://github.com/dotnet/arcade/pull/11903/files.
Adds three properties to allow repos to defer some/all of their target framework maintenance to arcade.
Arcade already had a file like this for its own purposes. I've removed that and hoisted arcade's targeting property into its Directory.Build.props.
mmitche added a commit that referenced this pull request Feb 21, 2023
* Introduce targeting properties (#12161)

* Introduce targeting properties
Targeting implementation of https://github.com/dotnet/arcade/pull/11903/files.
Adds three properties to allow repos to defer some/all of their target framework maintenance to arcade.
Arcade already had a file like this for its own purposes. I've removed that and hoisted arcade's targeting property into its Directory.Build.props.

* Fixup the TFM

* Remove import
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.

5 participants