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 missing Copyright headers #754

Merged
merged 13 commits into from
May 25, 2021
Merged

Add missing Copyright headers #754

merged 13 commits into from
May 25, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented May 14, 2021

A quick crude PR to add copyright header in srcs and hdrs using the script from here: https://github.com/atapas/add-copyright

After running the script, manually removed copyright from below files ( thrift gen, traceloggingdynamics, nostd::absl, nostd::mpark:)

/home/labhas/ot/labhas/opentelemetry-cpp$ grep -riL "OpenTelemetry Authors"  | grep "\.h$"
exporters/jaeger/thrift-gen/agent_types.h
exporters/jaeger/thrift-gen/zipkincore_constants.h
exporters/jaeger/thrift-gen/zipkincore_types.h
exporters/jaeger/thrift-gen/ZipkinCollector.h
exporters/jaeger/thrift-gen/Agent.h
exporters/jaeger/thrift-gen/Collector.h
exporters/jaeger/thrift-gen/jaeger_types.h
exporters/etw/include/opentelemetry/exporters/etw/TraceLoggingDynamic.h
exporters/etw/include/opentelemetry/exporters/etw/etw_traceloggingdynamic.h
api/include/opentelemetry/nostd/absl/types/internal/variant.h
api/include/opentelemetry/nostd/absl/types/variant.h
api/include/opentelemetry/nostd/absl/types/bad_variant_access.h
api/include/opentelemetry/nostd/absl/meta/type_traits.h
api/include/opentelemetry/nostd/absl/utility/utility.h
api/include/opentelemetry/nostd/absl/base/attributes.h
api/include/opentelemetry/nostd/absl/base/policy_checks.h
api/include/opentelemetry/nostd/absl/base/internal/identity.h
api/include/opentelemetry/nostd/absl/base/internal/inline_variable.h
api/include/opentelemetry/nostd/absl/base/internal/invoke.h
api/include/opentelemetry/nostd/absl/base/config.h
api/include/opentelemetry/nostd/absl/base/port.h
api/include/opentelemetry/nostd/absl/base/options.h
api/include/opentelemetry/nostd/absl/base/optimization.h
api/include/opentelemetry/nostd/absl/base/macros.h
api/include/opentelemetry/nostd/detail/functional.h
api/include/opentelemetry/nostd/detail/dependent_type.h
api/include/opentelemetry/nostd/detail/variant_fwd.h
api/include/opentelemetry/nostd/detail/void.h
api/include/opentelemetry/nostd/detail/decay.h
api/include/opentelemetry/nostd/detail/variant_size.h
api/include/opentelemetry/nostd/detail/variant_alternative.h
api/include/opentelemetry/nostd/detail/all.h
api/include/opentelemetry/nostd/detail/trait.h
api/include/opentelemetry/nostd/detail/invoke.h
api/include/opentelemetry/nostd/detail/valueless.h
api/include/opentelemetry/nostd/detail/type_pack_element.h

/home/labhas/ot/labhas/opentelemetry-cpp$ grep -riL "OpenTelemetry Authors"  | grep "\.cpp$"
exporters/jaeger/thrift-gen/Collector_server.skeleton.cpp
exporters/jaeger/thrift-gen/zipkincore_types.cpp
exporters/jaeger/thrift-gen/ZipkinCollector_server.skeleton.cpp
exporters/jaeger/thrift-gen/zipkincore_constants.cpp
exporters/jaeger/thrift-gen/Agent.cpp
exporters/jaeger/thrift-gen/Agent_server.skeleton.cpp
exporters/jaeger/thrift-gen/jaeger_types.cpp
exporters/jaeger/thrift-gen/Collector.cpp
exporters/jaeger/thrift-gen/ZipkinCollector.cpp
/home/labhas/ot/labhas/opentelemetry-cpp$

Sorry for the big PR. Will take care of sh/ps1/cmd files separately.

@lalitb lalitb requested a review from a team May 14, 2021 19:29
@ThomsonTan
Copy link
Contributor

Wondering whether it is required and necessary to add copyright header for the auto-generated files, like exporters/jaeger/thrift-gen/zipkincore_types.cpp?

@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #754 (566cce0) into main (02f9efc) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
- Coverage   96.02%   96.00%   -0.02%     
==========================================
  Files         176      176              
  Lines        7186     7186              
==========================================
- Hits         6900     6899       -1     
- Misses        286      287       +1     
Impacted Files Coverage Δ
api/include/opentelemetry/baggage/baggage.h 97.29% <ø> (ø)
.../include/opentelemetry/common/key_value_iterable.h 100.00% <ø> (ø)
...ude/opentelemetry/common/key_value_iterable_view.h 88.88% <ø> (ø)
api/include/opentelemetry/common/kv_properties.h 98.88% <ø> (ø)
api/include/opentelemetry/common/spin_lock_mutex.h 31.25% <ø> (ø)
api/include/opentelemetry/common/string_util.h 100.00% <ø> (ø)
api/include/opentelemetry/common/timestamp.h 100.00% <ø> (ø)
api/include/opentelemetry/context/context.h 100.00% <ø> (ø)
...lemetry/context/propagation/composite_propagator.h 100.00% <ø> (ø)
...ntelemetry/context/propagation/global_propagator.h 100.00% <ø> (ø)
... and 158 more

Copy link
Contributor

@maxgolov maxgolov left a comment

Choose a reason for hiding this comment

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

Us being "grandchildren" of Linux Foundation project - I would strongly encourage the usage of SPDX

// Copyright (c) 2021 OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

SPDX is part of Linux Foundation Collaborative Project initiative. This is more modern way of doing it. It is also shorter. You visually get to the essence of the code faster within 1 screen without having to scroll thru the license. Your license becomes just 2 lines. I can share a script that appends it. Latest spec:
https://www.linuxfoundation.org/en/blog/spdx-2-2-specification-released/

@maxgolov
Copy link
Contributor

maxgolov commented May 14, 2021

Technically and Legally - you do not have to have it at all, since LICENSE is in the root folder already covers it. I would strongly encourage us using SPDX. We went to greater length to adopt C++20-alike features on API surface, so let's be progressive in a way how we add our copyrights! 😄

@lalitb
Copy link
Member Author

lalitb commented May 15, 2021

Technically and Legally - you do not have to have it at all, since LICENSE is in the root folder already covers it. I would strongly encourage us using SPDX. We went to greater length to adopt C++20-alike features on API surface, so let's be progressive in a way how we add our copyrights! 😄

Agree we don't need the license in all files, as long as it's there in the root folder, and apache clearly specifies that too:
https://infra.apache.org/apply-license.html#copy-per-file

It's more useful if someone is using or viewing files in isolation, or we are having separate source distributions for api, sdk, etc.
But I don't have strong opinion to include the licenses in every file, in fact it's more of hassle to add these license later in each file if got missed out :)

We can close this PR in that case if we all agree :)

@maxgolov
Copy link
Contributor

maxgolov commented May 19, 2021

@lalitb - I can find the script I used to add SPDX headers in the other public repo that I'm maintaining. If you don't mind, I can try to apply SPDX. But right now - maybe we can keep in on a back-burner for now. We are good with the LICENSE file in the root folder.

@lalitb
Copy link
Member Author

lalitb commented May 19, 2021

@lalitb - I can find the script I used to add SPDX headers in the other public repo that I'm maintaining. If you don't mind, I can try to apply SPDX. But right now - maybe we can keep in on a back-burner for now. We are good with the LICENSE file in the root folder.

Sure, let me see if I can use the script I used for this PR for SPDX. If it's not straightforward, I will leave it for you for later :)

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang
Copy link
Member

reyang commented May 21, 2021

I wonder if:

  1. We could remove the exact year (e.g. 2020/2021). For example, OTel .NET doesn't have the exact year and OpenTelemetry Python also doesn't have that.
  2. We could use CI enforcement. For example this is what OpenTelemetry .NET did https://github.com/open-telemetry/opentelemetry-dotnet/blob/7af79470d92a5f7430e10b93cd785ec60f612249/build/stylecop.json#L6.

@reyang
Copy link
Member

reyang commented May 21, 2021

Technically and Legally - you do not have to have it at all, since LICENSE is in the root folder already covers it. I would strongly encourage us using SPDX. We went to greater length to adopt C++20-alike features on API surface, so let's be progressive in a way how we add our copyrights! 😄

Agree we don't need the license in all files, as long as it's there in the root folder, and apache clearly specifies that too:
https://infra.apache.org/apply-license.html#copy-per-file

It's more useful if someone is using or viewing files in isolation, or we are having separate source distributions for api, sdk, etc.
But I don't have strong opinion to include the licenses in every file, in fact it's more of hassle to add these license later in each file if got missed out :)

We can close this PR in that case if we all agree :)

I personally would agree, we might want to discuss this in the Maintainers Meeting.

@lalitb
Copy link
Member Author

lalitb commented May 24, 2021

Technically and Legally - you do not have to have it at all, since LICENSE is in the root folder already covers it. I would strongly encourage us using SPDX. We went to greater length to adopt C++20-alike features on API surface, so let's be progressive in a way how we add our copyrights! 😄

Agree we don't need the license in all files, as long as it's there in the root folder, and apache clearly specifies that too:
https://infra.apache.org/apply-license.html#copy-per-file
It's more useful if someone is using or viewing files in isolation, or we are having separate source distributions for api, sdk, etc.
But I don't have strong opinion to include the licenses in every file, in fact it's more of hassle to add these license later in each file if got missed out :)
We can close this PR in that case if we all agree :)

I personally would agree, we might want to discuss this in the Maintainers Meeting.

As discussed in today's Maintainers meeting, and also specified here: https://github.com/open-telemetry/community/blob/main/CONTRIBUTING.md#code-attribution , we should have short license/copyright notice in each source file. Currently, we have below header as part of this PR which is similar to what opentelemetry-java has:

// Copyright (c) 2021 OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

Will resolve the conflicts in this PR as of now. We need more clarity on thrift/protobuf generated files, and they would be left for now.

@reyang
Copy link
Member

reyang commented May 24, 2021

// Copyright (c) 2021 OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

Will resolve the conflicts in this PR as of now. We need more clarity on thrift/protobuf generated files, and they would be left for now.

Could we remove 2021?

@lalitb
Copy link
Member Author

lalitb commented May 24, 2021

// Copyright (c) 2021 OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0
Will resolve the conflicts in this PR as of now. We need more clarity on thrift/protobuf generated files, and they would be left for now.

Could we remove 2021?

Correct it's good to remove it. Will do.

@lalitb
Copy link
Member Author

lalitb commented May 24, 2021

2. We could use CI enforcement. For example this is what OpenTelemetry .NET did https://github.com/open-telemetry/opentelemetry-dotnet/blob/7af79470d92a5f7430e10b93cd785ec60f612249/build/stylecop.json#L6.

Yes, this would be good to have. Will see if we can incorporate something similar in our CI pipeline.

Copy link
Member

@alolita alolita left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks for removing the year from the Copyright notice.

// Copyright (c) OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

@ThomsonTan
Copy link
Contributor

Could we add the grep -riL to a CI task to check against the presence of license header?

@lalitb
Copy link
Member Author

lalitb commented May 25, 2021

We still need to add copyright information in BUILD, CMakeLists and scripts. Will add that in separate PR as this is too big now.

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