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

Add file_header_template to editorconfig #30132

Closed
wants to merge 52 commits into from

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Feb 12, 2021

  • This ensures all files have the proper header template in CI.
  • Also when adding new files from Solution Explorer, it will get pre-populated with the header template.

@Youssef1313
Copy link
Member Author

Before moving forward with applying a FixAll in more solution filters, can someone confirm you want this change?

@Youssef1313 Youssef1313 marked this pull request as ready for review February 12, 2021 09:35
@Youssef1313 Youssef1313 requested a review from a team February 12, 2021 09:35
Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

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

This is super valuable, but likely to be a bit tricky. It might be easier to move directly to MIT, since parts of the repo are already licensed that way, and that's where we want to end up.

eng/tools/RepoTasks/DownloadFile.cs Outdated Show resolved Hide resolved
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect, since it's copied source from something that was MIT.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Pilchie Is there a list of solution filters that I shouldn't touch their license headers?

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
// Copyright (c) .NET Foundation. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

This code in Caching moved from dotnet/extensions which was MIT.

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
// Copyright (c) .NET Foundation. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

The HttpClientFactory stuff also came from extensions.

@Pilchie Pilchie self-assigned this Feb 12, 2021
@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 12, 2021
@Pilchie
Copy link
Member

Pilchie commented Feb 12, 2021

Related to #18873.

@BrennanConroy BrennanConroy added the community-contribution Indicates that the PR has been added by a community member label Mar 29, 2021
@Youssef1313
Copy link
Member Author

Using EnforceCodeStyleInBuild caused the following error because the analyzer assemblies come from the .NET SDK are for CodeAnalysis 3.10 packages. But, the compiler version is pinned to a version that doesn't support 3.10.

CSC(0,0): error CS8032: (NETCORE_ENGINEERING_TELEMETRY=Build) An instance of analyzer Microsoft.CodeAnalysis.UseExplicitTupleName.UseExplicitTupleNameDiagnosticAnalyzer cannot be created from /home/vsts/work/1/s/.dotnet/sdk/6.0.100-preview.3.21168.19/Sdks/Microsoft.NET.Sdk/codestyle/cs/Microsoft.CodeAnalysis.CodeStyle.dll : Could not load file or assembly 'Microsoft.CodeAnalysis, Version=3.10.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'. The system cannot find the file specified..

Upgrading the compiler version introduced some nullable errors. I think I'll wait on this PR until you upgrade the compiler version and fix these errors. I'll convert to a draft until you move to the newer compiler version or I get to do this in a separate PR.

@BrennanConroy
Copy link
Member

@dougbu Can you comment on #30132 (comment)?

@dougbu
Copy link
Member

dougbu commented Mar 30, 2021

@dougbu Can you comment on #30132 (comment)?

Yes 😺

@dougbu
Copy link
Member

dougbu commented Mar 30, 2021

The Build team doesn't treat the compiler version as infrastructure unless (and, yes, this has happened) a new version causes problems. We pinned the version to keep aspnetcore ahead of Arcade IIRC.

I have no general concerns about moving from 3.7.x to 3.10.x. Suggest working with @JamesNK and @pranavkm, both of whom have done a large amount of nullability-related changes. @Youssef1313 where were most of the new warnings or errors❔

@Youssef1313
Copy link
Member Author

@dougbu src/Http/Headers/src/BaseHeaderParser, src/Http/Headers/src/HttpHeaderParser, and src/Components/Components/src/RenderTree/RenderTreeEdit

@BrennanConroy
Copy link
Member

I think I'll wait on this PR until you upgrade the compiler version and fix these errors

Done #31403

@Youssef1313
Copy link
Member Author

@Pilchie The size of the PR is growing crazy. Is that okay?

@BrennanConroy
Copy link
Member

Is it possible to only apply this to part of the repo at a time?

@Youssef1313
Copy link
Member Author

Is it possible to only apply this to part of the repo at a time?

Yes, I can cherry-pick the commits here to multiple PRs. Would you be able to let me know which directories are owned by the same team to open as a separate PRs?

@BrennanConroy
Copy link
Member

There are probably 5 main areas:
Mvc/
Components/
SignalR/
Servers/
Everything else/

@Tratcher
Copy link
Member

Tratcher commented Apr 1, 2021

Sorry @Youssef1313, we ran into a bureaucratic hurdle. We'll get it sorted out and get back to you here soon.

@Tratcher Tratcher removed their request for review June 18, 2021 21:07
@pranavkm
Copy link
Contributor

Closing this in lieu of #34573. Thanks for your time and effort though!

@pranavkm pranavkm closed this Jul 21, 2021
@Youssef1313 Youssef1313 deleted the patch-2 branch July 22, 2021 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants