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

Clean-up sourcelink repo and remove workarounds #1003

Closed
wants to merge 1 commit into from

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Apr 18, 2023

  1. Remove license headers from non-shipping assets (project files) Remove unnecessary license header in msbuild #1014
  2. Use the Microsoft.Build.NoTargets SDK for content-only packages That avoids invoking the compiler without defining custom targets and makes the repository use the same path as other repositories in the stack.
  3. Delete runtimeconfig.template.json in favor of the in-built "<RollForward>...</RollForward>" switch.
  4. Remove not needed sourcebuild target.
  5. Group dependencies in Versions.props by repositories. Remove the now unneeded Microsoft.NET.Test.Sdk entry.
  6. Define target frameworks for source build centrally in one place.
  7. Remove unnecessary package dependencies from projects.
  8. Apply code styling:
    • "empty line after Project tag and before closing Project tag"
    • "empty line between groups (property, item, target)"
    • "TargetFramework(s) should be the first property in the project"
  9. Remove unnecessary PrivateAssets="all" attributes from four ProjectReference items.

1. Remove license headers from non-shipping assets (project files)
2. Use the Microsoft.Build.NoTargets SDK for content-only packages
   That avoids invoking the compiler without defining custom targets
   and makes the repository use the same path as other repositories
   in the stack.
3. Delete runtimeconfig.template.json in favor of the in-built
   "<RollForward>...</RollForward>"  switch.
4. Remove not needed sourcebuild target.
5. Group dependencies in Versions.props by repositories. Remove the
   now unneeded Microsoft.NET.Test.Sdk entry.
6. Define target frameworks for source build centrally in one place.
7. Remove unnecessary package dependencies from projects.
8. Apply code styling:
   - "empty line after Project tag and before closing Project tag"
   - "empty line between groups (property, item, target)"
   - "TargetFramework(s) should be the first property in the project"
9. Remove unnecessary PrivateAssets="all" attributes from four
   ProjectReference items.
@KalleOlaviNiemitalo
Copy link

3. Delete runtimeconfig.template.json in favor of the in-built "..." switch.

Does "..." refer to the RollForward property here?

<RollForward>Major</RollForward>

@ViktorHofer
Copy link
Member Author

Yes. Markdown didn't display that correctly on GH. Fixed the comment, thanks.

Comment on lines -11 to -16
<Target Name="GetRepoSourceBuildCommandConfiguration"
BeforeTargets="GetSourceBuildCommandConfiguration">
<PropertyGroup>
<InnerBuildArgs>$(InnerBuildArgs) /p:Pack=true</InnerBuildArgs>
</PropertyGroup>
</Target>
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 verified that this is no longer needed. Sourcebuild automatically creates packages.

Comment on lines +5 to +8
<!-- TODO: Remove when Arcade offers an in-built way to filter out anything other than NetCurrent. -->
<PropertyGroup>
<TargetFrameworks Condition="'$(TargetFrameworks)' != '' and '$(DotNetBuildFromSource)' == 'true'">$(NetCurrent)</TargetFrameworks>
</PropertyGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

@tmat
Copy link
Member

tmat commented Apr 19, 2023

We actually want to build this source package to validate that the code is correct.


Refers to: src/SourceLink.Tools/Microsoft.SourceLink.Tools.Package.csproj:1 in 772a4ff. [](commit_id = 772a4ff, deletion_comment = False)

@tmat
Copy link
Member

tmat commented Apr 19, 2023

Are you sure this is still valid XML without <?xml?


Refers to: src/SourceLink.GitLab/buildMultiTargeting/Microsoft.SourceLink.GitLab.targets:1 in 772a4ff. [](commit_id = 772a4ff, deletion_comment = True)

@ViktorHofer
Copy link
Member Author

Are you sure this is still valid XML without <?xml?

Yes, those entries aren't required by msbuild anymore: dotnet/corefx#35686 (comment)

We removed them from corefx (now dotnet/runtime) 4 years ago and didn't encounter any issues with tooling. Our samples don't include them either:

dotnet new buildprops

dotnet new console

dotnet new classlib

@oleksandr-didyk
Copy link
Contributor

@tmat soft ping since this PR has been open for around a week and is required for the pre-built detection work. Would be great if you could take a look at it / approve it whenever you have time. Thanks!

@ViktorHofer
Copy link
Member Author

@tmat and I chatted offline and he wants me to move the formatting changes into a separate commit or PR. Will do soon.

@ViktorHofer
Copy link
Member Author

Closing in favor of #1015

@ViktorHofer ViktorHofer deleted the SourceLinkCleanup branch April 27, 2023 08:18
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.

4 participants