trufflesuite / truffle-compile

Compiler helper and artifact manager
22 stars 46 forks source link

Skip warnings and fix imports in truffle publish #48

Closed vladfr closed 6 years ago

vladfr commented 6 years ago

This does 2 changes to fix publish errors in certain cases.

  1. It skips the compiler warnings causing truffle publish to fail.
  2. It implements import path resolution for installed_contracts. This part is a bit hard-coded. I'll improve it, but I wanted to submit this, so we can discuss the solution.

See https://github.com/trufflesuite/truffle/issues/744

cgewecke commented 6 years ago

@vladfr Thanks for doing this - is there any chance you could link to the code you used to set up a reproduction case for this bug? I'd really like to add tests to this module and it might be helpful in evaluating this PR.

vladfr commented 6 years ago

Hey, it's from here: https://github.com/trufflesuite/truffle/issues/744 I'll check the tests as soon as I get a chance. I'll also refactor the hardcoded values.

The import issue is a bit weird. I'll look into a better way to resolve the missing deps too.

vladfr commented 6 years ago

@cgewecke I thought of using truffle-resolver as a more robust way of sending imports to the compiler. Resolver.resolve would work fine, but it uses a callback to return the body of the file it finds. I don't know how to make it work with solc.compileStandard, since this too uses a callback.

The async nature of the truffle-resolve classes defeated me. Any input?

cgewecke commented 6 years ago

@vladfr Thanks! I'll look at this in the next couple of days and see what we can do. . .

vladfr commented 6 years ago

@cgewecke - hey! did you get a chance to review?

cgewecke commented 6 years ago

@vladfr I'm so sorry - we were off-site all of last week and just catching up now. . . . Will look at this ASAP.

vladfr commented 6 years ago

No problem.

On Wed, Mar 21, 2018, 22:16 c-g-e-w-e-k-e-> notifications@github.com wrote:

@vladfr https://github.com/vladfr I'm so sorry - we were off-site all of last week and just catching up now. . . . Will look at this ASAP.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/trufflesuite/truffle-compile/pull/48#issuecomment-375081258, or mute the thread https://github.com/notifications/unsubscribe-auth/AAM-h3Dk4qK3XNV2rqTjgHQtuJTl1lP4ks5tgrUtgaJpZM4SkIxi .

cgewecke commented 6 years ago

@vladfr This looks good, thank you. Apologies again for how long it took to review this.

Rebooted Travis and the tests pass - we have a flaky EthPM test and the previous failure was nothing related to the work you've done here. Also ran this against PLCRVoting and it gets past the gathering contracts section successfully. Great work. Code LGTM.

Going to confer with @gnidan tomorrow AM (PST) and see if he has any further recommendations. . . (fair warning - he might.)

However, there's still a problem publishing PLCR (of course!) The ethpm lockfile spec differs from the ethpm documentation for package names and requires that they that are lower-cased. Resulting error looks like:

screen shot 2018-03-21 at 8 46 42 pm

@skmgoldin FWIW the lockfile implementation is consistent with NPM's rules for package names. They're also not allowed to have capital letters. Additionally there's some Mac vs Linux weirdness about filename capitalization. Do you have a view about this?