Navigation Menu

Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Update license header and scripts #8666

Merged
merged 12 commits into from Jun 4, 2018
Merged

Update license header and scripts #8666

merged 12 commits into from Jun 4, 2018

Conversation

niklasad1
Copy link
Collaborator

@niklasad1 niklasad1 commented May 19, 2018 •
edited

Sorry for a huge PR but most of the changes are caused by updated scripts.

The major changes in this PR are:

  1. Updated scripts/add_license.sh it will now replace the existing header it starts with // Copyright and ends with If not, see <http://www.gnu.org/licenses/>. or just add the header if no header is found
  2. Added a new script that supresses duplicated empty lines

So, the most of important parts of this PR to review are:

  1. scripts/add_license.sh
  2. scripts/remove_duplicate_empty_lines.sh

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels May 19, 2018
sorpaas
sorpaas reviewed May 20, 2018
View reviewed changes
dapps/js-glue/build.rs Outdated
@@ -8,12 +8,11 @@

// Parity is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
Copy link
Collaborator

@sorpaas sorpaas May 20, 2018 •
edited

Choose a reason for hiding this comment

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

Regarding the extra space, I think it's actually intentional? Reading from Wikipedia this is something called "English spacing". And I do remember encounterings of this spacing strategy in my life.

And in particular, the original GPL text uses English spacing. https://www.gnu.org/licenses/gpl.txt

@niklasad1 niklasad1 force-pushed the license-header/update branch 2 times, most recently from d5ec588 to 5d2fb0b Compare May 20, 2018 06:06
5chdn
5chdn reviewed May 20, 2018
View reviewed changes
ethcore/vm/src/return_data.rs Outdated
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

// Parity is free software: you can redistribute it and/or modify
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check what caused this to have two headers.

rpc/src/v1/helpers/network_settings.rs Outdated

// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

// Copyright 2015-2017 Parity Technologies (UK) Ltd.
// This file is part of Parity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Check double header.

@5chdn
Copy link
Contributor

5chdn commented May 20, 2018

This PR kills my browser. I reported this to Github. I'll try to continue reviewing...

5chdn
5chdn previously requested changes May 20, 2018
View reviewed changes
Copy link
Contributor

@5chdn 5chdn left a comment

Choose a reason for hiding this comment

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

More duplicated headers in:

  • util/rlp/benches/rlp.rs
  • util/rlp/src/error.rs
  • util/rlp/src/impls.rs
  • util/rlp/src/lib.rs
  • util/rlp/src/rlpin.rs
  • util/rlp/src/stream.rs
  • util/rlp/src/traits.rs
  • util/rlp/tests/tests.rs
  • util/rlp_compress/src/common.rs
  • util/rlp_compress/src/lib.rs

@5chdn 5chdn added this to the 1.12 milestone May 20, 2018
@dvdplm
Copy link
Collaborator

dvdplm commented May 21, 2018

I noticed we have a few Cargo.toml that are missing the license key. Not sure if now is a good time to add those.

e.g.
ethcore/res/wasm-tests/gen/Cargo.toml
ethcore/wasm/run/Cargo.toml
ethstore/cli/Cargo.toml
util/mem/Cargo.toml
util/stats/Cargo.toml
util/trie-standardmap/Cargo.toml
etc

@niklasad1
Copy link
Collaborator Author

niklasad1 commented May 21, 2018 •
edited

Good point, I found that the following Cargo.tomls were missing license = "xxx"

./evmbin/Cargo.toml
./local-store/Cargo.toml
./ethash/Cargo.toml
./json/Cargo.toml
./util/kvdb/Cargo.toml
./util/rlp_derive/Cargo.toml
./util/using_queue/Cargo.toml
./util/error/Cargo.toml
./util/macros/Cargo.toml
./util/rlp_compress/Cargo.toml
./util/trie-standardmap/Cargo.toml
./util/kvdb-rocksdb/Cargo.toml
./util/mem/Cargo.toml
./util/path/Cargo.toml
./util/kvdb-memorydb/Cargo.toml
./util/dir/Cargo.toml
./util/version/Cargo.toml
./util/stats/Cargo.toml
./util/stop-guard/Cargo.toml
./util/migration-rocksdb/Cargo.toml
./util/unexpected/Cargo.toml
./ethkey/cli/Cargo.toml
./ethkey/Cargo.toml
./chainspec/Cargo.toml
./ethcore/service/Cargo.toml
./ethcore/evm/Cargo.toml
./ethcore/wasm/run/Cargo.toml
./ethcore/wasm/Cargo.toml
./ethcore/types/Cargo.toml
./ethcore/transaction/Cargo.toml
./ethcore/crypto/Cargo.toml
./ethcore/vm/Cargo.toml
./ethcore/res/wasm-tests/gen/Cargo.toml
./machine/Cargo.toml
./ethstore/cli/Cargo.toml
./ethstore/Cargo.toml
./whisper/Cargo.toml

However, I'm not sure if we should just append license = "GPL-3.0" if it's missing after [package] in Cargo.toml

And lastly, should it go into scripts/add_license.sh or in a separate script?

@dvdplm
Copy link
Collaborator

dvdplm commented May 21, 2018

Maybe add a script that warns us about it but does not take any action? I can imagine situations where adding a mismatching or wrong license could ending real bad.
On the other hand, what are the legal implications for adding the wrong license to a the crate meta-data? I.e. if the LICENSE is a MIT license, but the crate accidentally got published as GPL3, then which is it? IANAL, but surely the actual LICENSE file must be what counts?

@jamesray1 jamesray1 mentioned this pull request May 22, 2018
Open
@5chdn
Copy link
Contributor

5chdn commented May 22, 2018

I don't want to review this again. Can we maybe split the changes to the script from the changes to the actual headers?

@5chdn 5chdn dismissed their stale review May 22, 2018 11:33

obsolete

@niklasad1
Copy link
Collaborator Author

@5chdn It is only two scripts that are changed:

  1. scripts/add_license.sh
  2. scripts/remove_duplicate_empty_lines.sh

Do you want me to revert the changes in them?

Otherwise if I think this PR should be good to go!

@5chdn
Copy link
Contributor

5chdn commented May 22, 2018

No, sorry, thanks for pointing them out.

dvdplm
dvdplm approved these changes May 31, 2018
View reviewed changes
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm

@5chdn 5chdn added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 31, 2018
@5chdn
Copy link
Contributor

5chdn commented May 31, 2018

Let's merge this.

@niklasad1
Copy link
Collaborator Author

Rebased!

@5chdn
Copy link
Contributor

5chdn commented May 31, 2018

Something is wrong with the CI. Why is this job not linked @General-Beck ?

https://gitlab.parity.io/parity/parity/-/jobs/88542

@niklasad1 niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Jun 2, 2018
@niklasad1
Copy link
Collaborator Author

niklasad1 commented Jun 2, 2018 •
edited

@5chdn it's good to go now 🥇

@debris
Copy link
Collaborator

debris commented Jun 4, 2018 •
edited

merging before there are more conflicts ;)

@debris debris merged commit 98b7c07 into openethereum:master Jun 4, 2018
@5chdn
Copy link
Contributor

5chdn commented Jun 4, 2018

Thanks, Niklas :)

@niklasad1 niklasad1 deleted the license-header/update branch June 4, 2018 11:36
ordian added a commit to ordian/parity that referenced this pull request Jun 4, 2018
@ordian
Merge branch 'master' of https://github.com/paritytech/parity into wa…
a1e42a3 
…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity:
  Remove Result wrapper from AccountProvider in RPC impls (openethereum#8763)
  Update `license header` and `scripts` (openethereum#8666)
  Remove HostTrait altogether (openethereum#8681)
  ethcore-sync: fix connection to peers behind chain fork block (openethereum#8710)
  Remove public node settings from cli (openethereum#8758)
dvdplm added a commit that referenced this pull request Jun 7, 2018
@dvdplm
Merge remote-tracking branch 'origin/master' into refactor/hashdb-gen…
4fd7990 
…eric

* origin/master:
  ethcore: fix ancient block error msg handling (#8832)
  CI: Fix docker tags (#8822)
  parity: fix indentation in sync logging (#8794)
  Removed obsolete IpcMode enum (#8819)
  Remove UI related settings from CLI (#8783)
  Remove windows tray and installer (#8778)
  docs: add changelogs for 1.10.6 and 1.11.3 (#8810)
  Fix ancient blocks queue deadlock (#8751)
  Disallow unsigned transactions in case EIP-86 is disabled (#8802)
  Fix evmbin compilation (#8795)
  Have space between feature cfg flag (#8791)
  rpc: fix address formatting in TransactionRequest Display (#8786)
  Conditionally compile ethcore public test helpers (#8743)
  Remove Result wrapper from AccountProvider in RPC impls (#8763)
  Update `license header` and `scripts` (#8666)
@ordian ordian mentioned this pull request Jun 26, 2018
Merged
@5chdn 5chdn mentioned this pull request Jan 5, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sorpaas sorpaas sorpaas left review comments

@dvdplm dvdplm dvdplm approved these changes

@debris debris debris approved these changes

@5chdn 5chdn 5chdn approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.0 Ethereum
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@niklasad1 @5chdn @dvdplm @debris @sorpaas