Clean up the mess of basePath in solc by axic · Pull Request #9353 · argotorg/solidity

axic · 2020-07-07T19:38:18Z

The goal of this PR is to make sure that with solc/solc --base-path openzeppelin-contracts/contracts/ openzeppelin-contracts/contracts/token/ERC20/SafeERC20.sol the following imports will work:

in openzeppelin-contracts/contracts/token/ERC20/SafeERC20.sol: import "./IERC20.sol";
in openzeppelin-contracts/contracts/token/ERC20/SafeERC20.sol: import "../../math/SafeMath.sol";

axic · 2020-07-07T19:46:09Z

solc/CommandLineInterface.cpp

 			else
 			{
-				auto infile = boost::filesystem::path(path);
+				auto const infile = m_basePath / path; //boost::filesystem::path(path);


This changes behaviour: if --base-path is specified, that will be prepended to the filename, e.g. there is no need to use a complete path.

Because of this it may be a breaking change.

This sounds a bit weird to me - arguments to shell commands should always be files that can be resolved in the usual way.

Shouldn't we rather do something like:

if (absolute_path(m_basePath).isPrefixOf(absolute_path(path))) path = absolute_path(path).removePrefix(absolute_path(m_basePath)) infile = m_basePath / path;

?

I guess that can be a convenient feature, however couldn't find similar functions in boost to your pseudocode. Do you have any pointers?

@chriseth maybe you missed this, but there are no such features in boost/std::fs. Did you mean to implement these and/or was this just pseudo code for logic?

It was just pseudo code.

axic · 2020-07-07T19:47:15Z

solc/CommandLineInterface.cpp

 				}

-				m_sourceCodes[infile.generic_string()] = readFileAsString(infile.string());
+				m_sourceCodes[path] = readFileAsString(infile.string());


Here we ensure the source mapping in CompilerStack is not tainted with the value of --base-path. This is only a change because above we prepend m_basePath.

axic · 2020-07-07T19:48:52Z

solc/CommandLineInterface.cpp

+				sout() << "allowedpath: " << path << "\n";
 			}
 			m_allowedDirectories.push_back(boost::filesystem::path(path).remove_filename());
+			if (!m_basePath.empty())


Here we also include the base path in the allowed directories.

See line 1115

For some reason it was not working for me without this.

chriseth · 2020-07-08T08:18:14Z

solc/CommandLineInterface.cpp

 			}
 			m_allowedDirectories.push_back(boost::filesystem::path(path).remove_filename());
+			if (!m_basePath.empty())
+				m_allowedDirectories.push_back(boost::filesystem::canonical(m_basePath));


weakly canonical?

Aren't you saying above we don't need this because line 1155 does it too?

axic · 2020-10-30T20:48:46Z

Need to check if this could close #9346.

chriseth · 2020-11-10T10:14:57Z

I am still of the opinion, that the commandline arguments to solc should be proper paths that the shell can resolve. Internally, it would be nice to strip the base path if the given file is inside that directory.

axic · 2020-11-17T23:20:33Z

Just to state the problem, I expect the following to work:

$ solc/solc --base-path openzeppelin-contracts/contracts/ openzeppelin-contracts/contracts/token/ERC20/SafeERC20.sol 
Warning: This is a pre-release compiler version, please do not use it in production.

Warning: 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. Please see https://spdx.org for more information.
--> openzeppelin-contracts/contracts/token/ERC20/SafeERC20.sol

Error: Source "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol" not found: File outside of allowed directories.
 --> openzeppelin-contracts/contracts/token/ERC20/SafeERC20.sol:3:1:
  |
3 | import "./IERC20.sol";
  | ^^^^^^^^^^^^^^^^^^^^^^

Error: Source "openzeppelin-contracts/contracts/math/SafeMath.sol" not found: File outside of allowed directories.
 --> openzeppelin-contracts/contracts/token/ERC20/SafeERC20.sol:4:1:
  |
4 | import "../../math/SafeMath.sol";
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Error: Source "openzeppelin-contracts/contracts/utils/Address.sol" not found: File outside of allowed directories.
 --> openzeppelin-contracts/contracts/token/ERC20/SafeERC20.sol:5:1:
  |
5 | import "../../utils/Address.sol";
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The above output is on develop, not this PR. Do we agree the above should work or not?

(And adding some of those extra paths to --allow-path does not help.)

I agree that file completion is good, and would be nice to have it. Disregarding that, this messy PR in its current form makes this to work:

$ solc/solc --base-path openzeppelin-contracts/contracts/ token/ERC20/SafeERC20.sol
Warning: This is a pre-release compiler version, please do not use it in production.

Warning: 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. Please see https://spdx.org for more information.
--> token/ERC20/SafeERC20.sol

Error: Source file requires different compiler version (current compiler is 0.7.5-develop.2020.11.17+commit.0fe1791f.Darwin.appleclang) - note that nightly builds are considered to be strictly less than the released version
 --> math/SafeMath.sol:1:1:
  |
1 | pragma solidity ^0.6.0;
  | ^^^^^^^^^^^^^^^^^^^^^^^

Error: Source file requires different compiler version (current compiler is 0.7.5-develop.2020.11.17+commit.0fe1791f.Darwin.appleclang) - note that nightly builds are considered to be strictly less than the released version
 --> token/ERC20/IERC20.sol:1:1:
  |
1 | pragma solidity ^0.6.0;
  | ^^^^^^^^^^^^^^^^^^^^^^^

Error: Source file requires different compiler version (current compiler is 0.7.5-develop.2020.11.17+commit.0fe1791f.Darwin.appleclang) - note that nightly builds are considered to be strictly less than the released version
 --> utils/Address.sol:1:1:
  |
1 | pragma solidity ^0.6.0;
  | ^^^^^^^^^^^^^^^^^^^^^^^

chriseth · 2020-11-23T15:19:07Z

Draft or ready for review?

axic · 2020-12-14T14:57:19Z

For some reason now this translate solc test.sol into trying to load /test.sol.

axic · 2020-12-14T14:59:39Z

Described what this should do on the top. Feel free to take over.

chriseth · 2020-12-14T15:14:09Z

I think essentially solc --base-path BP BP/x should behave like cd BP; solc x - the main question is what to do if an input file lies outside the base path.

cameel · 2021-02-22T15:55:21Z

solc/CommandLineInterface.cpp

 {
+	auto basePath = boost::filesystem::canonical(m_basePath);
+	if (!basePath.empty())
+		m_allowedDirectories.push_back(basePath);


Looks like m_allowedDirectories is a vector. I think we should make it a set to avoid duplicates (and the order does not matter, right?).

cameel · 2021-02-22T15:58:49Z

solc/CommandLineInterface.cpp

+				if (isPrefix(basePath.string(), boost::filesystem::canonical(infile).string()))
+					path = removePrefix(basePath.string(), boost::filesystem::canonical(infile).string());


canonical() raises an exception if the path does not exist. I think this might be a problem here because infile might be a relative path that only works on top of basePath (rather than the current working directory).

cameel · 2021-02-22T16:01:16Z

solc/CommandLineInterface.cpp

+				if (m_basePath.empty())
+					infile = path;
+				else
+					infile = m_basePath / path;


I think this needs a check against path being an absolute path.

cameel · 2021-02-22T16:27:20Z

I could take this over.

What do you think about first adding a table in the docs that would illustrate how various combinations of --base-path, --allowed-paths and absolute/relative paths (both in imports and in paths specified on the command-line) interact with each other? This would show in a clear way how corner cases are handled now and we could then easily decide which ones do not match our expectations and change them.

We could also extract the path remapping logic out of CommandLineInterface and use these examples as test cases. Maybe also unify the behavior with standard JSON (do these options have equivalents there?).

chriseth · 2021-02-23T17:01:56Z

Sounds good, but I think a summary of what we want and changing it right away is better than just documenting the status quo.

axic · 2021-03-17T13:23:05Z

Closing this for now. We'll first figure out it in #11036 and #11105, and then come back.

axic commented Jul 7, 2020

View reviewed changes

chriseth reviewed Jul 8, 2020

View reviewed changes

axic force-pushed the base-path branch from 5db5995 to 806f5ba Compare July 27, 2020 20:21

axic force-pushed the base-path branch from 806f5ba to b6afb31 Compare November 10, 2020 14:34

axic force-pushed the base-path branch 2 times, most recently from 98d1a9c to b678720 Compare November 20, 2020 00:59

argotorg deleted a comment from stackenbotten Nov 20, 2020

Properly handle basepath (and include in allowed directories)

3506208

axic force-pushed the base-path branch from b678720 to 3506208 Compare December 11, 2020 15:41

cameel added the takeover Can be taken over by someone other than label giver label Feb 3, 2021

cameel reviewed Feb 22, 2021

View reviewed changes

cameel mentioned this pull request Mar 3, 2021

Buggy import path resolution in corner cases with remappings and base path #11036

Closed

cameel mentioned this pull request Mar 15, 2021

Path resolution docs + proposed changes (path spec v1) #11105

Closed

axic mentioned this pull request Mar 16, 2021

Extracts import file reading & remapping logic out of CommandLineInterface into their own classes. #11113

Merged

axic closed this Mar 17, 2021

axic deleted the base-path branch March 17, 2021 13:23

cameel mentioned this pull request Mar 22, 2021

Restricted import system with named roots (path spec v2) #11138

Closed

cameel mentioned this pull request Apr 15, 2021

Path resolution docs (current state only) #11263

Closed

This was referenced May 19, 2021

Stripping --base-path from CLI paths + CLI path normalization (path spec v3) #11408

Closed

Search path for libraries (path spec v3) #11409

Closed

cameel mentioned this pull request May 26, 2021

Path resolution docs (current state only) v2 #11442

Merged

		if (isPrefix(basePath.string(), boost::filesystem::canonical(infile).string()))
		path = removePrefix(basePath.string(), boost::filesystem::canonical(infile).string());

Conversation

axic commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Oct 30, 2020

Uh oh!

chriseth commented Nov 10, 2020

Uh oh!

axic commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chriseth commented Nov 23, 2020

Uh oh!

axic commented Dec 14, 2020

Uh oh!

axic commented Dec 14, 2020

Uh oh!

chriseth commented Dec 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel commented Feb 22, 2021

Uh oh!

chriseth commented Feb 23, 2021

Uh oh!

axic commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

axic commented Jul 7, 2020 •

edited

Loading

axic commented Nov 17, 2020 •

edited

Loading