[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

misc: adopt minimal license headers #15456

Merged
merged 11 commits into from
Sep 15, 2023
Merged

misc: adopt minimal license headers #15456

merged 11 commits into from
Sep 15, 2023

Conversation

paulirish
Copy link
Member

We're allowed to have more minimal license headers.

So, let's!

Also... in #10469 we added an AUTHORS file and changed to 'The Lighthouse Authors'. The policy says:

However for projects that accept a great deal of external code (such as Chromium), you may wish to optionally update the copyright statements in your source code to "Copyright 2014 The [Project] Authors" to acknowledge the community's efforts.

We haven't added a name to this since the file was established. So I'd prefer we just stick to the basic model (drop the AUTHORS file, tweak copyright assignment). This PR also includes that change.

FYI The diff page is pretty rough on the browser. Spoiler alert.. It's a lot of diffs like this:

diff --git a/core/index.js b/core/index.js
index a6d77f3c5..26864ef42 100644
--- a/core/index.js
+++ b/core/index.js
@@ -1,7 +1,7 @@
 /**
- * @license Copyright 2016 The Lighthouse Authors. All Rights Reserved.
- * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
- * 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
+ * Copyright 2016 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
  */
 
 import Trace from './gather/gatherers/trace.js';

@paulirish paulirish requested a review from a team as a code owner September 12, 2023 23:59
@paulirish paulirish requested review from adamraine and removed request for a team September 12, 2023 23:59
@adamraine
Copy link
Member

Looks like smokes are broke, did decreasing the size of the license affect our transfer size expectations?

connorjclark
connorjclark reviewed Sep 13, 2023
View reviewed changes
build/build-bundle.js
* 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
* Copyright 2018 Google LLC
* SPDX-License-Identifier: Apache-2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing the source file comments is fine, but the bundle is a form of distribution so it should retain the full reference to the license IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

based on my read of the above docs, the minimal header would be fine for a bundle. but... i don't care too much. i'm happy to keep the full text version for this case. :)

Copy link
Collaborator
@connorjclark connorjclark Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose the "SPDX-License-Identifier" thing there is pretty cut and dry in terms of declaring the license that applies (tho, it's for a robot, not a human). Still, I think it's nice to give the whole shebang for the bundle

Copy link
Collaborator

Choose a reason for hiding this comment

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

lol we both thought this was the bundle comment header, but that's something else and less verbose. nvm

@paulirish
Copy link
Member Author

did decreasing the size of the license affect our transfer size expectations?

yup. 😢

@connorjclark
Copy link
Collaborator
connorjclark commented Sep 13, 2023 •
edited
Loading

So I'd prefer we just stick to the basic model (drop the AUTHORS file, tweak copyright assignment)

nitting on your wording: this PR changes the acknowledgement of the copyright found in source headers, but that's a meaningless change wrt. to actual copyright assignment. Contributors have and will continue to retain their copyrights... and we are just choosing a different valid approach for legalese comment cruft that is within the guidelines set by our employer

@devtools-bot devtools-bot merged commit a615ea8 into main Sep 15, 2023
29 checks passed
@devtools-bot devtools-bot deleted the short-license-header branch September 15, 2023 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@connorjclark connorjclark connorjclark left review comments

@adamraine adamraine adamraine approved these changes

Assignees

@adamraine adamraine

Labels
waiting4reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@paulirish @adamraine @connorjclark @devtools-bot