Imports can't always be resolved

muellerberndt commented 5 years ago

Sabre isn't always able to resolve imports. Apparently, imports can be either relative to the path of the importing file or the current working directory. Also, solc has an "allow-paths" argument which further complicates things.

For example, in this project, there is no way of getting imports working:

$ sabre agreements/ActiveAgreement.sol 
✔ Compiled with solc v0.4.25 successfully
ActiveAgreement.sol:3:1: ParserError: Source "commons-collections/DataStorage.sol" not found: File import callback not supported
import "commons-collections/DataStorage.sol";
^-------------------------------------------^

ActiveAgreement.sol:4:1: ParserError: Source "commons-collections/AddressScopes.sol" not found: File import callback not supported
import "commons-collections/AddressScopes.sol";
^---------------------------------------------^

ActiveAgreement.sol:5:1: ParserError: Source "commons-events/EventEmitter.sol" not found: File import callback not supported
import "commons-events/EventEmitter.sol";
^---------------------------------------^

ActiveAgreement.sol:6:1: ParserError: Source "documents-commons/Signable.sol" not found: File import callback not supported
import "documents-commons/Signable.sol";
^--------------------------------------^

ActiveAgreement.sol:7:1: ParserError: Source "commons-management/VersionedArtifact.sol" not found: File import callback not supported
import "commons-management/VersionedArtifact.sol";
^------------------------------------------------^

ActiveAgreement.sol:9:1: ParserError: Source "agreements/Agreements.sol" not found: File import callback not supported
import "agreements/Agreements.sol";
^---------------------------------^

This can only be addressed comprehensively by researching the exact imports resolution logic used by solc and setting up a number of test scenarios to verify that all cases are accounted for.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 1.4 ETH (228.23 USD @ $163.02/ETH) attached to it.

If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
- Want to chip in? Add your own contribution here.
- Questions? Checkout Gitcoin Help or the Gitcoin Slack
- $97,023.70 more funded OSS Work available on the Gitcoin Issue Explorer

kuhnchris commented 5 years ago

Hey there! Actually this sounds like it should work, but I assume it may be a problem with from where the build is being started, as solc will take the current directory as "." and if it's on "blackstone" level then agreements/... would be blackstone/agreements/ which does naturally not exist. There is however some way to include the parameters in your batch/Makefile/build pipeline.

I suggest reading here: https://solidity.readthedocs.io/en/v0.5.7/layout-of-source-files.html#use-in-actual-compilers

As workaround I guess a mapping áka solc ".=$PWD/contratcs/src" would work.

muellerberndt commented 5 years ago

@kuhnchris I think Sabre is doing it wrong at the moment.. it's using the directory of the contract as the base directory instead of the current working directory.

kuhnchris commented 5 years ago

Well, i checked Sabre, I think the problem is that there is no indication on where the relative path should be.

parseImports > rel: ..\blackstone\contracts\src\agreements\commons-collections / ..\blackstone\contracts\src\agreements\commons-collections\DataStorage.sol
parseImports > rel: ..\blackstone\contracts\src\agreements\commons-collections / ..\blackstone\contracts\src\agreements\commons-collections\AddressScopes.sol
parseImports > rel: ..\blackstone\contracts\src\agreements\commons-events / ..\blackstone\contracts\src\agreements\commons-events\EventEmitter.sol
parseImports > rel: ..\blackstone\contracts\src\agreements\documents-commons / ..\blackstone\contracts\src\agreements\documents-commons\Signable.sol
parseImports > rel: ..\blackstone\contracts\src\agreements\commons-management / ..\blackstone\contracts\src\agreements\commons-management\VersionedArtifact.sol
parseImports > rel: ..\blackstone\contracts\src\agreements\agreements / ..\blackstone\contracts\src\agreements\agreements\Agreements.sol
sourcelist: ActiveAgreement.sol
import_paths: commons-collections/DataStorage.sol,commons-collections/AddressScopes.sol,commons-events/EventEmitter.sol,documents-commons/Signable.sol,commons-management/VersionedArtifact.sol,agreements/Agreements.sol

And the source is

pragma solidity ^0.4.25;

import "commons-collections/DataStorage.sol";
import "commons-collections/AddressScopes.sol";
import "commons-events/EventEmitter.sol";
import "documents-commons/Signable.sol";
import "commons-management/VersionedArtifact.sol";

import "agreements/Agreements.sol";

so by just providing the .sol to sabre it won't look there, even I would need to look up where those files actually are (under /src, while the Agreements live in src/agreements).

The only thing i COULD think of is to add a parameter to sabre to specifiy a 'root' directory. You think that's a route to go?

kuhnchris commented 5 years ago

Sorry for the double post:

I took the freedom of implementing this anyways. So there is now a new parameter "--rootdir" which takes a directory that would be used as base for the import resolution. With that, the blackstone contract "compiles".

PS C:\Users\kuhnc\OneDrive\Documents\GitHub\sabre> node .\sabre.js ..\blackstone\contracts\src\agreements\ActiveAgreement.sol --rootdir=..\blackstone\contracts\src\
found match: import "commons-collections/DataStorage.sol";,import,,commons-collections/DataStorage.sol
found match: import "commons-collections/AddressScopes.sol";,import,,commons-collections/AddressScopes.sol
found match: import "commons-events/EventEmitter.sol";,import,,commons-events/EventEmitter.sol
found match: import "documents-commons/Signable.sol";,import,,documents-commons/Signable.sol
found match: import "commons-management/VersionedArtifact.sol";,import,,commons-management/VersionedArtifact.sol
found match: import "agreements/Agreements.sol";,import,,agreements/Agreements.sol
import_paths: commons-collections/DataStorage.sol,commons-collections/AddressScopes.sol,commons-events/EventEmitter.sol,documents-commons/Signable.sol,commons-management/VersionedArtifact.sol,agreements/Agreements.sol
parseImports > rel: ..\blackstone\contracts\src\commons-collections / ..\blackstone\contracts\src\commons-collections\DataStorage.sol
parseImports > rel: ..\blackstone\contracts\src\commons-collections / ..\blackstone\contracts\src\commons-collections\AddressScopes.sol
found match: import "commons-collections/DataStorage.sol";,import,,commons-collections/DataStorage.sol
parseImports > rel: ..\blackstone\contracts\src\commons-collections / ..\blackstone\contracts\src\commons-collections\DataStorage.sol
parseImports > rel: ..\blackstone\contracts\src\commons-events / ..\blackstone\contracts\src\commons-events\EventEmitter.sol
parseImports > rel: ..\blackstone\contracts\src\documents-commons / ..\blackstone\contracts\src\documents-commons\Signable.sol
parseImports > rel: ..\blackstone\contracts\src\commons-management / ..\blackstone\contracts\src\commons-management\VersionedArtifact.sol
found match: import "commons-standards/ERC165.sol";,import,,commons-standards/ERC165.sol
parseImports > rel: ..\blackstone\contracts\src\commons-standards / ..\blackstone\contracts\src\commons-standards\ERC165.sol
parseImports > rel: ..\blackstone\contracts\src\agreements / ..\blackstone\contracts\src\agreements\Agreements.sol
sourcelist: commons-collections/DataStorage.sol,commons-collections/AddressScopes.sol,commons-events/EventEmitter.sol,documents-commons/Signable.sol,commons-standards/ERC165.sol,commons-management/VersionedArtifact.sol,agreements/Agreements.sol,ActiveAgreement.sol
import_paths: commons-collections/DataStorage.sol,commons-collections/AddressScopes.sol,commons-events/EventEmitter.sol,documents-commons/Signable.sol,commons-management/VersionedArtifact.sol,agreements/Agreements.sol
√ Compiled with solc v0.4.25 successfully
✖ Compiling the Solidity code did not return any bytecode. Note that abstract contracts cannot be analyzed.

Want me to send in a PR?

THanks, Chris

P.S. may I also ask why sabre isn't using a "proper" command line parser like "command-line-args"

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Workers have applied to start work.

These users each claimed they can complete the work by 12 months from now. Please review their action plans below:

1) eswarasai has applied to start work _(Funders only: approve worker | reject worker)_.

Will add a new argument option similar to --allow-paths of solc to allow absolute paths for contract imports

Learn more on the Gitcoin Issue Details page.

2) kuhnchris has applied to start work _(Funders only: approve worker | reject worker)_.

As noted in the github thread, I'll send in the PR for the new command line option to fix sabre (this issue was previously reserved hence I didn't apply)

Learn more on the Gitcoin Issue Details page.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 10 months, 3 weeks from now. Please review their action plans below:

1) eswarasai has been approved to start work.

Will add a new argument option similar to --allow-paths of solc to allow absolute paths for contract imports 2) kuhnchris has applied to start work _(Funders only: approve worker | reject worker)_.

As noted in the github thread, I'll send in the PR for the new command line option to fix sabre (this issue was previously reserved hence I didn't apply)

Learn more on the Gitcoin Issue Details page.

muellerberndt commented 5 years ago

Hey @kuhnchris! Sorry, I saw your comments only now.

The Gitcoin bounty was supposed to be reserved for @eswarasai 😅However I saw that you already implemented a solution. Eswara, did you already start working on this and are you OK if we use Khunkris' solution?

kuhnchris commented 5 years ago

Not a problem at all, let's see what eswarasai says, else i'll just add a PR and he can work from that if he wants. ;-)

It's weird that I was able to apply allthou it was reserved... gotta open up a bug report on gitcoin for that I guess.

Thanks

muellerberndt commented 5 years ago

Not sure, maybe I messed up when creating the issue..

kuhnchris commented 5 years ago

Naw i'm pretty sure it's something in the gitcoin system again, worked with them for a while, so it's most likely a bug, i'll try to replicate it on my local test instance, so do not worry about that. And as I said, if eswarasai wants to take that from scratch or from some point of my work on - no problemo, it's fine by me, it was only a quick dirty little fix, and it will surely cause more problems down the line. ;-)

Thanks, Chris

eswarasai commented 5 years ago

@kuhnchris -- Sorry. It's not a bug on Gitcoin front. It's been over 3 days since the issue was reserved to me and I've forgotten to start work on this until now. I'm just following up on the above conversation. Although you're suggestion to use --rootdir solves the problem of the above mentioned scenario, it still isn't an ideal solution to go forward with because it would then fail for resolving imports from node_modules for example ERC20.sol from open-solidity etc.

I don't have a proper solution yet which would resolve all the imports scenario out of the box. But I'm hoping to take care of it once and for all to resolve such issues. Thanks for looking into it anyway :)

kuhnchris commented 5 years ago

Hey there, as I said, no problem at all. :-) Regarding that node_modules issue - maybe we should throw that in as a "good guess default" then, additionally to any other directory the user suggests? Since I assume node_modules isn't that uncommon, especially in javascript/node environments...

muellerberndt commented 5 years ago

That is very kind @kuhnchris, thanks!

I just created a test project to figure out how solc behaves in various situations. Works as follows:

$ git clone https://github.com/b-mueller/solc-imports-test 
$ cd solc-imports-test
$ solc --allow-paths . project_directory/ImportTest.sol --bin

======= InCWD.sol:InCWD =======
Binary: 
6080604052348015600f57600080fd5b50606d8061001e6000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c8063eb26da6314602d575b600080fd5b60336035565b005b6002600114603f57fe5b56fea165627a7a7230582096577a8cb6c96cd247549d02da92d1b068f7a66872b0293df6b78ddb829017840029

======= project_directory/ImportTest.sol:ImportTest =======
Binary: 
608060405234801561001057600080fd5b50610117806100206000396000f3fe6080604052348015600f57600080fd5b5060043610605a5760003560e01c806334ba89a614605f578063925154ca1460675780639f8dd80f14606f578063a3583e45146077578063eb26da6314607f578063f59dd579146087575b600080fd5b6065608f565b005b606d60af565b005b607560bb565b005b607d60c7565b005b608560d3565b005b608d60df565b005b609560d3565b609b60c7565b60a160bb565b60a760df565b60ad60af565b565b600660051460b957fe5b565b600360021460c557fe5b565b600260011460d157fe5b565b600260011460dd57fe5b565b600460031460e957fe5b56fea165627a7a7230582063cc9f85c4b5863c1660bca558824c32ad66b914c579c0b87ded7265b59d5f8b0029

======= project_directory/InProjectDir.sol:InProjectDir =======
Binary: 
6080604052348015600f57600080fd5b50606d8061001e6000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c8063a3583e4514602d575b600080fd5b60336035565b005b6002600114603f57fe5b56fea165627a7a72305820649925ac8449ff323b1f62c3e421db9fd41ca3af4cbdbdd6f8a3e9de8da012610029

======= project_directory/subdir/InProjectSubDir.sol:InProjectSubDir =======
Binary: 
6080604052348015600f57600080fd5b50606d8061001e6000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c80639f8dd80f14602d575b600080fd5b60336035565b005b6003600214603f57fe5b56fea165627a7a7230582066df392f0721d720e6bb175f0f884abd032b83adfa5762c10b22dcabca1c1beb0029

======= somedir/InExternalDir1.sol:InExternalDir1 =======
Binary: 
6080604052348015600f57600080fd5b50606d8061001e6000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c8063f59dd57914602d575b600080fd5b60336035565b005b6004600314603f57fe5b56fea165627a7a72305820236f9db53b5edd2c34396c7cad0e8c7c16513d3084fad6954e23b003a88bdccd0029

======= somedir/InExternalDir2.sol:InExternalDir2 =======
Binary: 
6080604052348015600f57600080fd5b50606d8061001e6000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c8063925154ca14602d575b600080fd5b60336035565b005b6006600514603f57fe5b56fea165627a7a723058204d8a821b4f16f218c1ba7eb30eacb1873be8c4429b8b55c6ec5ad88e82f8c6e60029

Most notably:

When the import is a relative path ("./", "../"), solc uses a path relative to the project directory.
When the import isn't prefixed, solc uses a path relative to the current working directory (the path that solc is being run from).
If the path is outside the project directory it must be added to --allowed-paths.

I'd suggest that we replicate this behavior but add the cwd to the list of allowed paths automatically.

@eswarasai if you find a better solution that also solves node imports you are welcome :) I think Truffle framework might have helper functions that could be used here.

eswarasai commented 5 years ago

@b-mueller -- That's perfect. Thanks a lot for the above testing scenarios. I'll add them to our test cases to see if everything is resolved appropriately or not.

I've already went through the codebase of truffle-compile in and out earlier and I couldn't find much of helper functions, if I remember correctly. Although I'll re-visit the codebase to see if it makes sense to me this time at least 😅

tagomaru commented 5 years ago

I've already went through the codebase of truffle-compile in and out earlier and I couldn't find much of helper functions

idk the detail issue, but if you are resolving import things, i think truffle-resolver is better to refer to.

https://github.com/trufflesuite/truffle/blob/develop/packages/truffle-resolver/index.js

Profiler of truffle-compile may also be helpful for you.

https://github.com/tagomaru/truffle-sca2t/blob/master/packages/truffle-sca2t-mythx/lib/runner.js#L34

gitcoinbot commented 5 years ago

⚡️ A *Bot Kitty* Kudos has been sent to @kuhnchris for this issue from @b-mueller. ⚡️ The sender had the following public comments: > Thanks for the PR! Nice work @kuhnchris! Your Kudos has automatically been sent in the ETH address we have on file.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@eswarasai due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

[x] reminder (3 days)
[x] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 1.4 ETH (323.21 USD @ $230.86/ETH) has been submitted by:

@eswarasai

@b-mueller please take a look at the submitted work:

PR by @eswarasai

Learn more on the Gitcoin Issue Details page
Want to chip in? Add your own contribution here.
Questions? Checkout Gitcoin Help or the Gitcoin Slack
$88,991.19 more funded OSS Work available on the Gitcoin Issue Explorer

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 1.4 ETH (357.16 USD @ $255.11/ETH) attached to this issue has been approved & issued to @eswarasai.

Learn more on the Gitcoin Issue Details page
Questions? Checkout Gitcoin Help or the Gitcoin Slack
$88,539.03 more funded OSS Work available on the Gitcoin Issue Explorer

muellerberndt / sabre

Imports can't always be resolved #52