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

Update EIP-1: Add License Checker #5379

Closed
wants to merge 18 commits into from
Closed

Conversation

Pandapip1
Copy link
Member

@Pandapip1 Pandapip1 commented Jul 30, 2022

Makes sure all solidity files are CC0.

@eth-bot
Copy link
Collaborator

eth-bot commented Jul 30, 2022

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

@github-actions
Copy link

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

@Pandapip1
Copy link
Member Author

Testing...

@Pandapip1 Pandapip1 closed this Jul 31, 2022
@Pandapip1 Pandapip1 reopened this Jul 31, 2022
@Pandapip1
Copy link
Member Author

Well uh, glad I tested 😆

@github-actions
Copy link

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

@Pandapip1
Copy link
Member Author

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

@Pandapip1
Copy link
Member Author

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

@github-actions
Copy link

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

@github-actions
Copy link

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

@Pandapip1
Copy link
Member Author

Pandapip1 commented Jul 31, 2022

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

.licenserc.json Outdated Show resolved Hide resolved
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

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.

@Pandapip1
Copy link
Member Author

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.

@MicahZoltu
Copy link
Contributor

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.

@Pandapip1
Copy link
Member Author

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.

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Aug 1, 2022

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.

@Pandapip1
Copy link
Member Author

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.

@github-actions
Copy link

github-actions bot commented Aug 1, 2022

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

@Pandapip1
Copy link
Member Author

Re-requesting @SamWilsn

@github-actions github-actions bot removed the w-stale Waiting on activity label Mar 22, 2023
@Pandapip1 Pandapip1 requested review from MicahZoltu and xinbenlv and removed request for MicahZoltu March 22, 2023 22:30
@github-actions
Copy link

github-actions bot commented Apr 7, 2023

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 github-actions bot added the w-stale Waiting on activity label Apr 7, 2023
@Pandapip1
Copy link
Member Author

Re-cc @SamWilsn

@github-actions github-actions bot removed the w-stale Waiting on activity label Apr 8, 2023
@github-actions
Copy link

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 github-actions bot added the w-stale Waiting on activity label Apr 22, 2023
@Pandapip1
Copy link
Member Author

CC @SamWilsn

@github-actions github-actions bot removed the w-stale Waiting on activity label May 12, 2023
@SamWilsn
Copy link
Contributor

@Pandapip1 I assume you're pinging me just to keep the stale bot away, but I won't approve a PR that only allows CC0-1.0 assets.

@SamWilsn
Copy link
Contributor

As discussed on EIPIP today, I've opened a discussion thread about licenses: https://ethereum-magicians.org/t/14329

@Pandapip1
Copy link
Member Author

@Pandapip1 I assume you're pinging me just to keep the stale bot away, but I won't approve a PR that only allows CC0-1.0 assets.

This was something I was not aware of. Would you mind telling me which licenses can be allowed?

@github-actions
Copy link

github-actions bot commented Jun 1, 2023

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 github-actions bot added the w-stale Waiting on activity label Jun 1, 2023
@SamWilsn
Copy link
Contributor

SamWilsn commented Jun 1, 2023

This was something I was not aware of. Would you mind telling me which licenses can be allowed?

I think we can start with CC0-1.0, Unlicense, Apache-2.0, and MIT? Probably best discussed on the FEM thread though.

@github-actions github-actions bot removed the w-stale Waiting on activity label Jun 2, 2023
@Pandapip1
Copy link
Member Author

I didn't know there was a FEM thread. I'll add the non-copyleft ones for now.

@Pandapip1
Copy link
Member Author

Also, I assume you mean Unilicence as opposed to Unlicenced?

@eth-bot eth-bot changed the title CI: Add License Checker Update EIP-1: Add License Checker Jun 2, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jun 2, 2023
@Pandapip1
Copy link
Member Author

The license checker isn't sophisticated enough to do SPDX checking. I'll probably make my own CI tool.

@Pandapip1 Pandapip1 closed this Jun 2, 2023
@github-actions
Copy link

github-actions bot commented Jun 2, 2023

The commit 7568e6d (as a parent of 2b45aaf) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal e-consensus Waiting on editor consensus t-process w-ci Waiting on CI to pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants