Check for SPDX license identifiers. #8907

chriseth · 2020-05-12T09:56:49Z

Closes #7738.

axic · 2020-05-12T10:03:27Z

libsolidity/parsing/Parser.cpp

+optional<string> Parser::findLicenseString()
+{
+	// We circumvent the scanner here, because it skips non-docstring comments.
+	static regex const licenseRegex("SPDX-License-Identifier:\\s+([^\\n\\r]+)");


This might be the first-ever compliant contract: axic/eth2-deposit-contract@3aaf6f0

(Pushed yesterday)

I wonder what prompted me to start this...

axic · 2020-05-12T10:04:10Z

libsolidity/parsing/Parser.cpp

+		{-1, -1, m_scanner->charStream()},
+		"SPDX license identifier not provided in source file. "
+		"Before publishing, consider adding a comment containing "
+		"\"SPDX-License-Identifier: <SPDX-License>\" to each source file. "


It is still unclear what to use for closed source:
a) "Proprietary" perhaps?
b) avoiding the field?

Provide the field with UNLICENSED or CLOSED-SOURCE or PROPRIETARY - do we need to specify that?

I think we should provide a recommendation given SPDX does not provide one.

npm proposes UNLICENSED: https://docs.npmjs.com/files/package.json#license

Sounds good, we should document this and link to npm as well.

chriseth · 2020-05-12T11:02:42Z

Once we merge this, we should maybe start adding the field to common libraries?

hrkrshnn · 2020-05-12T11:08:00Z

We could also implement a feature for solidity-upgrade to do add a license.

chriseth · 2020-05-12T11:28:08Z

@hrkrshnn good idea!

chriseth · 2020-05-12T17:10:04Z

libsolidity/parsing/Parser.cpp

+optional<string> Parser::findLicenseString()
+{
+	// We circumvent the scanner here, because it skips non-docstring comments.
+	static regex const licenseRegex("SPDX-License-Identifier:\\s+([^\\n\\r]+)");


As it is now, this can also be found in strings. Do we want to extend this to ^// SPDX...? Then it would not work for /*-comments, though.

chriseth · 2020-05-12T19:19:55Z

Taken over by @aarlt.
We would like to get this finished quickly, so please report your progress so that others can take over because of timezones.

stackenbotten · 2020-05-13T01:59:46Z

There was an error when running chk_coding_style for commit 021444e697d84e928661cadb443ac0b0ae77068b:

Coding style error:
 libsolidity/parsing/Parser.cpp:1997: for (auto& node : _nodes)
 libsolidity/parsing/Parser.cpp:1999: for (auto& remove : toBeRemoved)

Please check that your changes are working as intended.

libsolidity/parsing/Parser.cpp

aarlt · 2020-05-13T02:06:34Z

@chriseth I mainly added testing support. Sadly I didn't saw the current errors locally. These errors need to be fixed.

libsolidity/parsing/Parser.cpp

chriseth · 2020-05-13T08:17:34Z

libsolidity/parsing/Parser.cpp

-
+	static regex const licenseRegex("SPDX-License-Identifier:\\s*([^\\n\\r\\s]+)");
+
+	// Remove all parts of the source that where referenced by different AST node locations.


That's a good idea, but we could also find all places that match the regex and see if we ended up inside one of the nodes.

libsolidity/parsing/Parser.cpp

chriseth · 2020-05-13T09:51:03Z

test/libsolidity/AnalysisFramework.cpp

@@ -46,11 +46,13 @@ AnalysisFramework::parseAnalyseAndReturnError(
 	bool _reportWarnings,
 	bool _insertVersionPragma,
 	bool _allowMultipleErrors,
-	bool _allowRecoveryErrors
+	bool _allowRecoveryErrors,
+	bool _insertLicensePragma


Is there a situation where we would insert the version but not the license pragma?

axic · 2020-05-13T10:53:07Z

libsolidity/parsing/Parser.cpp

+		);
+	else
+		parserError(
+			0000_error,


ErrorIds... Why are the above two the same errors?

axic · 2020-05-13T10:54:44Z

libsolidity/parsing/Parser.cpp

+			"SPDX license identifier not provided in source file. "
+			"Before publishing, consider adding a comment containing "
+			"\"SPDX-License-Identifier: <SPDX-License>\" to each source file. "
+			"Use \"SPDX-License-Identifier: UNLICENSED\" for non-open-source code. "


Nit: Is non-open-source the best term? Why not just closed-source?

"open source", "source accessible", "proprietary", "closed-source" - all different terms...

We could also say "free software" as that is even a smaller subset of open source and we advocate for that 😉

axic · 2020-05-13T10:57:02Z

test/libsolidity/ASTJSON/long_type_name_binary_operation.json

@@ -8,6 +8,7 @@
    ]
  },
  "id": 12,
+  "license": null,


Didn't we had some PR/issue discussing that we shouldn't have null fields? Maybe it is out of scope here.

there is an open pr about it.

axic · 2020-05-13T10:57:24Z

test/libsolidity/GasTest.cpp

@@ -101,6 +101,7 @@ void GasTest::printUpdatedExpectations(ostream& _stream, string const& _linePref
 TestCase::TestResult GasTest::run(ostream& _stream, string const& _linePrefix, bool _formatted)
 {
 	string const versionPragma = "pragma solidity >=0.0;\n";
+	string const license = "// SPDX-License-Identifier: GPL-3.0\n";


Nit: why not preamble as in the other files?

axic

Should add a parser test for MIT OR Apache-2.0 and GPL-2.0 AND GPL-3.0. I know we do not deal with expression, but as we instruct people to use in a warning, we should make sure they are parsed correctly.

axic · 2020-05-13T11:00:16Z

Will the documentation changes be part of a different PR?

Can we also have parser tests for different comment styles (if we support them):

//SPDX-License-Identifier:
///SPDX-License-Identifier:
// SPDX-License-Identifier:
/// SPDX-License-Identifier:
/* SPDX-License-Identifier:
 * SPDX-License-Identifier:

chriseth · 2020-05-13T11:49:38Z

/// will not work, it is a doxygen comment.

chriseth · 2020-05-13T11:50:11Z

will add a test for /// and also multpile licenses.

chriseth · 2020-05-13T14:54:38Z

Added documentation.

axic · 2020-05-13T15:12:42Z

docs/layout-of-source-files.rst

+it does include the supplied string in the `bytecode metadata <metadata>`_.
+
+If you do not want to specify a license or if the source code is
+not open-source, please use the special value ``UNLICENSED``.


Should we mention this is also the suggested way by npm?

I don't think this is needed.

I just saw that there is a spdx license identifier UNLICENSE. I'm wondering whether UNLICENSE vs UNLICENSED can create confusions.

{ "reference": "./Unlicense.html", "isDeprecatedLicenseId": false, "isFsfLibre": true, "detailsUrl": "http://spdx.org/licenses/Unlicense.json", "referenceNumber": "179", "name": "The Unlicense", "licenseId": "Unlicense", "seeAlso": [ "https://unlicense.org/" ], "isOsiApproved": false }

This is also recommended by npm, I don't think we should just make an arbitrary new suggestion.

axic

Looks good.

chriseth · 2020-05-13T16:10:55Z

Fingers crossed...

chriseth · 2020-05-13T16:57:15Z

Ah! The joy of external tests!

chriseth · 2020-05-13T17:03:01Z

The failure seems to be an SMT failure. Re-running...

aarlt · 2020-05-13T19:42:30Z

docs/layout-of-source-files.rst

+Supplying this comment of course does not free you from other
+obligations related to licensing like having to mention
+a specific license header in each source file or the
+original copyrigh holder.


typo: copyright

axic reviewed May 12, 2020

View reviewed changes

chriseth commented May 12, 2020

View reviewed changes

aarlt force-pushed the licenseIdentifier branch from 43252b5 to 021444e Compare May 13, 2020 01:58

stackenbotten reviewed May 13, 2020

View reviewed changes

libsolidity/parsing/Parser.cpp Outdated Show resolved Hide resolved

stackenbotten reviewed May 13, 2020

View reviewed changes

libsolidity/parsing/Parser.cpp Outdated Show resolved Hide resolved

aarlt force-pushed the licenseIdentifier branch from 021444e to 92ab0c5 Compare May 13, 2020 02:01

chriseth commented May 13, 2020

View reviewed changes

libsolidity/parsing/Parser.cpp Outdated Show resolved Hide resolved

chriseth commented May 13, 2020

View reviewed changes

libsolidity/parsing/Parser.cpp Outdated Show resolved Hide resolved

chriseth commented May 13, 2020

View reviewed changes

chriseth force-pushed the licenseIdentifier branch 4 times, most recently from 578fc47 to 16ad880 Compare May 13, 2020 10:49

chriseth marked this pull request as ready for review May 13, 2020 10:49

axic reviewed May 13, 2020

View reviewed changes

libsolidity/parsing/Parser.cpp Outdated

);

else

parserError(

0000_error,

Copy link

Member

axic May 13, 2020

Choose a reason for hiding this comment

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

ErrorIds... Why are the above two the same errors?

axic reviewed May 13, 2020

View reviewed changes

axic requested changes May 13, 2020

View reviewed changes

chriseth force-pushed the licenseIdentifier branch from b98a695 to 01df7a1 Compare May 13, 2020 15:00

chriseth requested a review from axic May 13, 2020 15:02

axic reviewed May 13, 2020

View reviewed changes

axic previously approved these changes May 13, 2020

View reviewed changes

chriseth dismissed axic’s stale review via f6936ac May 13, 2020 15:48

chriseth force-pushed the licenseIdentifier branch 3 times, most recently from 40a5318 to 69fa2f1 Compare May 13, 2020 16:09

chriseth requested a review from axic May 13, 2020 16:12

chriseth and others added 3 commits May 13, 2020 18:16

Add support for SPDX license identifiers.

3872a1f

Update existing tests.

52b9a92

Add new test.

af44c05

chriseth force-pushed the licenseIdentifier branch from 69fa2f1 to fdb26c8 Compare May 13, 2020 16:17

chriseth force-pushed the licenseIdentifier branch from dcd35b3 to fdb26c8 Compare May 13, 2020 19:19

aarlt reviewed May 13, 2020

View reviewed changes

chriseth added 3 commits May 13, 2020 22:16

Documentation changes.

0148525

Update test extraction script to recognize license identifier.

bcc4bbc

Add license tags to documentation snippets.

d33b67b

chriseth force-pushed the licenseIdentifier branch from fdb26c8 to d33b67b Compare May 13, 2020 20:16

axic approved these changes May 13, 2020

View reviewed changes

chriseth merged commit 504b8d2 into develop May 13, 2020

chriseth deleted the licenseIdentifier branch May 13, 2020 23:21

This was referenced May 14, 2020

SPDX license identifiers missing OpenZeppelin/openzeppelin-contracts#2234

Closed

SPDX license identifiers missing trufflesuite/truffle#3051

Closed

SPDX license identifiers missing truffle-box/bare-box#23

Open

naszam mentioned this pull request May 16, 2020

SPDX license identifiers missing opengsn/gsn#375

Closed


		static regex const licenseRegex("SPDX-License-Identifier:\\s*([^\\n\\r\\s]+)");

		// Remove all parts of the source that where referenced by different AST node locations.

Check for SPDX license identifiers. #8907

Check for SPDX license identifiers. #8907

Conversation

chriseth commented May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented May 12, 2020

hrkrshnn commented May 12, 2020

chriseth commented May 12, 2020

Choose a reason for hiding this comment

chriseth commented May 12, 2020

stackenbotten commented May 13, 2020

aarlt commented May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic left a comment

Choose a reason for hiding this comment

axic commented May 13, 2020

chriseth commented May 13, 2020

chriseth commented May 13, 2020

chriseth commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic left a comment

Choose a reason for hiding this comment

chriseth commented May 13, 2020

chriseth commented May 13, 2020

chriseth commented May 13, 2020

Choose a reason for hiding this comment

chriseth commented May 12, 2020 •

edited

Loading

aarlt commented May 13, 2020 •

edited

Loading