-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[release/6.0] Update THIRD-PARTY-NOTICES.TXT #60092
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
[release/6.0] Update THIRD-PARTY-NOTICES.TXT #60092
Conversation
Question... Visual Studio has a massive TPN. In the past it's something that's been updated whenever an external component like .NET is added to VS. jQuery and Bootstrap comes to mind for ASP.NET. Should we include the changes to our TPN in the VS TPN as well? |
Updated the TPN to incorporate formatting/sorting changes due to updates in infra: a945962 Output of the tool:
|
|
||
------------------------------------------------ | ||
|
||
The MIT License (MIT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Which one is this associated with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming from dotnet/aspnetcore/main/THIRD-PARTY-NOTICES.txt line 194
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/dotnet/aspnetcore/blob/main/THIRD-PARTY-NOTICES.txt line 194 is blank and I suspect this should be under the License notice for corefx
title above. See https://github.com/dotnet/runtime/pull/60092/files#diff-1f7876cafb24ce314004bb439a08320edba033f8a8a43d1961009f1088bd43d3R1350
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing.
|
||
Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
|
||
License notice for StyleCop Analyzers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 While I have no objection to seeing this here, does it apply for cases where the referenced package is only a development dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 While I have no objection to seeing this here, does it apply for cases where the referenced package is only a development dependency?
Good question - I'm not sure, but it would be easy to remove this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richlander do we need to include dev-time dependencies in our shipping TPN? Presumably not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the comment from Sam, where it's not clear what the last license is for.
@NikolaMilosavljevic is this mergeable? |
Quite a few reviews are missing, but otherwise it's mergeable. |
@NikolaMilosavljevic can you tag the reviewers again ? (which are required for the final merge) |
@mkArtakMSFT , @dougbu - we are missing aspnetcore sign-off - can you review new entries coming from dotnet/aspnetcore? |
aspnetcore is the only one missing - I've added and tagged more reviewers for that repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know exactly how or why some of these items are used and probably shouldn't be the approver. @mkArtakMSFT who else needs to look at this❔
|
||
------------------------------------------------ | ||
|
||
The MIT License (MIT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/dotnet/aspnetcore/blob/main/THIRD-PARTY-NOTICES.txt line 194 is blank and I suspect this should be under the License notice for corefx
title above. See https://github.com/dotnet/runtime/pull/60092/files#diff-1f7876cafb24ce314004bb439a08320edba033f8a8a43d1961009f1088bd43d3R1350
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
SOFTWARE. | ||
|
||
License notice for musl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may also be a dev / build dependency. We build (an installer?) for Linux MUSL on an Alpine image but I'm not sure exactly how that uses MUSL libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the empty line and moved under corefx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NikolaMilosavljevic was this a response to https://github.com/dotnet/runtime/pull/60092#discussion_r728315386❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NikolaMilosavljevic was this a response to https://github.com/dotnet/runtime/pull/60092#discussion_r728315386❔
Yes, that is correct - fixing the issue above. There was no way to add a comment right underneath that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
musl gets compiled into every WebAssembly app via emscripten. We should probably mention emscripten too /cc @lewing
|
||
License notice for Angular v8.0 | ||
-------------------------------------------- | ||
-------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a blank line after this for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming from dotnet/aspnetcore - we could fix it here, but it should also be fixed in source location to not regress in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
limitations under the License. | ||
|
||
License notice for Angular v8.0 | ||
-------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorten and add a blank line after this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed - also removed duplicate 'Angular 8.0' notice.
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
THE SOFTWARE. | ||
|
||
License notice for corefx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a dangling header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed - removed.
Side question: Why do you need ASP.NET sign-off on a Runtime PR❔ Is this TPN.txt file going to be used for every published asset from any repo❔ |
Yes, this is a merged TPN, that is installed under dotnet folder - it covers everything we install there, including ASP.NET. |
@NikolaMilosavljevic, given that's the case, what about all the other dependencies that ASP.NET Core has? Shouldn't these also be listed here? |
All ASP.NET dependencies are here, in the shared file. We avoid duplication and only include a single instance of the same dependency. It's likely that many of those dependencies are also present in other repos and included in a different section of this unified file. Is there a specific dependency of ASP.NET that should be included but it is not present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine for aspnetcore. @dougbu is OOF today, so don't feel you have to wait for his sign-off.
Unless I hear objections in the the 15 minutes, I'm going to merge this PR. @Pilchie , @mkArtakMSFT your questions have replies, can you help resolve those concerns (conversations)? |
Update third-party-notices file for .NET 6.
Infra used to generate this file is in another PR: #60091
Here's the output of TPN regeneration process, specifying which repo contributes each new notice: