EIPs icon indicating copy to clipboard operation
EIPs copied to clipboard

CI: Add License Checker

Open Pandapip1 opened this issue 3 years ago • 29 comments

Makes sure all solidity files are CC0.

Closes https://github.com/ethereum/EIPs/pull/5231 (Supersedes) Closes https://github.com/ethereum/EIPs/pull/5232 (Supersedes) Closes https://github.com/ethereum/EIPs/pull/5227 (Supersedes)

Pandapip1 avatar Jul 30 '22 23:07 Pandapip1

File EIPS/eip-1.md

Requires 2 more reviewers from @axic, @gcolvin, @lightclient, @samwilsn

File .github/workflows/ci.yml

Requires 1 more reviewers from @axic, @gcolvin, @lightclient, @samwilsn

File .licenserc.json

Requires 1 more reviewers from @axic, @gcolvin, @lightclient, @samwilsn

eth-bot avatar Jul 30 '22 23:07 eth-bot

The commit 51a20de10c91d549492d89caa245a028a5c02cdf (as a parent of 67d67c49442dfae9cb9a2851661a05cc9d2a7875) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Jul 31 '22 00:07 github-actions[bot]

Testing...

Pandapip1 avatar Jul 31 '22 00:07 Pandapip1

Well uh, glad I tested 😆

Pandapip1 avatar Jul 31 '22 00:07 Pandapip1

The commit 12f811389067e1762df415c836ee8103ed8019c4 (as a parent of 13d8664da340b5456b904e9c7df8ab9278a3f95a) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Jul 31 '22 00:07 github-actions[bot]

Well, I guess I'm about to be adding some identifiers to a few files. This will now need a manual merge.

Pandapip1 avatar Jul 31 '22 00:07 Pandapip1

Actually, on second thought, I'll just exempt those files.

Pandapip1 avatar Jul 31 '22 00:07 Pandapip1

The commit 12f811389067e1762df415c836ee8103ed8019c4 (as a parent of 272a89a8377bc3632d7e897bb86ba4b317cc79d4) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Jul 31 '22 00:07 github-actions[bot]

The commit 533d083c303e9f6b828791d63bcd6e62647b7ffb (as a parent of 0aebd8fd7c7aa30951f05a6fb0f9db180e10404b) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Jul 31 '22 00:07 github-actions[bot]

image Can confirm it works as intended... I guess I'll wait for @MicahZoltu's judgment.

Pandapip1 avatar Jul 31 '22 00:07 Pandapip1

Does this not error if files are missing a license line at the top of the file entirely? I have always found license at top of file quite silly and would like to continue supporting people just leaving the line out entirely.

No, it does, but IIRC the solidity compiler warns you if you don't have a license file, as if it's deployed to Ethereum and the contract source verified, the license should be obvious from the file alone.

That's my case for .sol files. I am okay with removing the req for .ts and .js though.

Pandapip1 avatar Jul 31 '22 12:07 Pandapip1

Is it possible to make it so it checks to make sure licenses are CC0 if present, but ignore the file if they aren't present? That would be my preference if it is possible.

MicahZoltu avatar Jul 31 '22 15:07 MicahZoltu

Is it possible to make it so it checks to make sure licenses are CC0 if present, but ignore the file if they aren't present?

Again, that's explicitly not allowed by the solidity style guide. I'd be glad to make an issue if it's a blocker, but I would much rather force people to do the right thing and make it obvious that their code can be used for whatever purpose.

Pandapip1 avatar Jul 31 '22 16:07 Pandapip1

that's explicitly not allowed by the solidity style guide

We are not beholden to following other people's bad decisions just because they write them down in a style guide. 😄 The argument that the license should be embedded in the contract so that it shows up in various tools that deterministically build based on the source file alone (like Etherscan) is a much more compelling argument in favor of including a license in the header, so I recommend you pursue that angle rather than "because other people do it".

I would much rather force people to do the right thing

Aside from the Ethesrcan argument above, I'm very much unconvinced this practice is "the right thing" and in almost all other contexts I think it is "the wrong thing".


All the above being said, I think I'm compelled enough by the Etherscan issue to allow this for .sol files, though I would still like licenses to not be required for other file types as I consider it to be an ~~ridiculous~~ outdated pattern.

MicahZoltu avatar Aug 01 '22 09:08 MicahZoltu

I can get behind not requiring licenses in other files (after all, most of the issues with using the wrong license are in .sol files due to most developers automatically putting in the MIT license header in their .sol files). I have already made that change.

Pandapip1 avatar Aug 01 '22 14:08 Pandapip1

The commit d261332bb3ee297b3a95ffb9413e67a902ac5149 (as a parent of 125c7d5bb97a6ddb2d18b49f986fa5edfb47968f) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 01 '22 15:08 github-actions[bot]

The commit f045a7a2d039d6de7b1a32b599f98cc13cbdf784 (as a parent of 3b5a6ea53e5da2780966d7390263af681fc27150) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 01 '22 15:08 github-actions[bot]

The commit 51dcb8137b79a353b33c265ce368a54f11024098 (as a parent of c2269de3e3cff94552ad6b1f699b54d1e773e1c9) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 01 '22 15:08 github-actions[bot]

Modifies EIP-1 now, so should be safe to mark as ready for review.

Pandapip1 avatar Aug 01 '22 16:08 Pandapip1

The commit 7f275ace81b1e867c9a876342f98193f42a8b88a (as a parent of 0a7c7892f5f7be8d22c3b62c3b9a9bafa98a7d18) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 01 '22 16:08 github-actions[bot]

@MicahZoltu @axic @lightclient @SamWilsn @gcolvin any objections to this?

  • This only forces an SPDX identifier of CC0-1.0 for .sol files. All other file types are unaffected.
  • The current system only looks at changed files. No exceptions are needed, nor are any unnecessary file changes.
  • Adds a short snippet to EIP-1 reminding EIP authors that assets must be cc0 if they can be.

Pandapip1 avatar Aug 01 '22 17:08 Pandapip1

The commit b56fac89dee68a09db45bf8d1c4f2cb1385dba33 (as a parent of c722f9302ef8ede0fd2fde90e3ca9cac747b9f03) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 03 '22 17:08 github-actions[bot]

The commit 0f41e2937cd5b669339d32f6a67750c9fe4cf332 (as a parent of 18ef9c82cca9a1947673436db9faaab63ee9b19a) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 04 '22 12:08 github-actions[bot]

The commit 0f41e2937cd5b669339d32f6a67750c9fe4cf332 (as a parent of d55aa5896f0aa25640833f4b79c8e585874ec0f6) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 08 '22 20:08 github-actions[bot]

The commit 43e30667a85a3798f5036d69a1604ce30b5626dd (as a parent of c38e31136b9e916cd45c74c0d97b2443bcc7f672) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 11 '22 17:08 github-actions[bot]

The commit fa04c9cbcb1622404da913e29d1c80723c266e70 (as a parent of 6de2efa36a2ed81b81cfa5bd9bcd698074b3bcea) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 12 '22 01:08 github-actions[bot]

The commit d342bfb11e6e85d37bb0953b8bdc8679993265dd (as a parent of 7464dcb2a167026c33679f5692142d405123c56a) contains errors. Please inspect the Run Summary for details.

github-actions[bot] avatar Aug 12 '22 13:08 github-actions[bot]

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

github-actions[bot] avatar Sep 14 '22 00:09 github-actions[bot]

@SamWilsn I'm pretty sure during meeting 63 we agreed to merge this.

Pandapip1 avatar Sep 14 '22 12:09 Pandapip1

Somehow the stale bot hasn't been reprompting me.

I still think this is a good idea. This doesn't need to be a required check, but it makes my job of scanning for aberrant licenses easier.

Pandapip1 avatar Jan 18 '23 04:01 Pandapip1