Skip to content

Conversation

@cameel
Copy link
Collaborator

@cameel cameel commented Jan 25, 2022

This PR adjusts the command we run from solc-js and adds npm run build required to compile the TypeScript code. It should be merged after argotorg/solc-js#566.

@cameel cameel self-assigned this Jan 25, 2022
@cameel cameel marked this pull request as ready for review January 25, 2022 14:34
ekpyron
ekpyron previously approved these changes Jan 25, 2022
@cameel cameel force-pushed the update-solcjs-commands-for-typescript branch from 353f1a5 to be7485c Compare January 25, 2022 14:39
@cameel
Copy link
Collaborator Author

cameel commented Jan 25, 2022

I found two more places where we do not run solcjs but we do npm install on solc-js. I think they now need npm run build too. PR updated.

echo "Prepare solc-js."
cd /root/solc-js
npm install >/dev/null 2>&1
npm run build >/dev/null 2>&1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure about this one and these scripts are not actively tested but I'm adding it just in case.

ekpyron
ekpyron previously approved these changes Jan 25, 2022
@ekpyron
Copy link
Collaborator

ekpyron commented Jan 25, 2022

Tests are still failing.
Also: does the release checklist need adjustment or is it fine?

@cameel
Copy link
Collaborator Author

cameel commented Jan 25, 2022

Also: does the release checklist need adjustment or is it fine?

Right, it does. You cannot run verifyVersion.js directly. You need npx ts-node verifyVersion.ts instead.
But I think we should replace it with npm run updateBinary. It does this but also downloads the binary. I think we should have changed it to that even earlier, back when we made it not download a new binary automatically.

@cameel cameel force-pushed the update-solcjs-commands-for-typescript branch from be7485c to 4e39513 Compare January 25, 2022 15:43
@cameel
Copy link
Collaborator Author

cameel commented Jan 25, 2022

  • Checklist updated.
  • Added changelog entry for TypeScript migration.
  • Bytecode comparison was failing because the JS snippet was importing specific .js files directly instead of doing it via the public interface. I changed it completely so that now the package is installed via npm from a local dir and we can just import solc.
  • solc-js and Colony external tests were failing because now soljson.js is actually under dist/. npm run build copies it there from the old location but only if we put it in that old location before running the command. That was not the case.
    • It's only Colony test that failed because it's the only one that's still on solc-js rather than native binary. I think we should keep one working like that just to have this code path somewhat tested.

@cameel cameel changed the title Update the solcjs commands for TypeScript Compatibility fixes for solc-js TypeScript migration Jan 25, 2022
@ekpyron
Copy link
Collaborator

ekpyron commented Jan 25, 2022

Still something off in the tests apparently...

@cameel
Copy link
Collaborator Author

cameel commented Jan 25, 2022

Pushing a fix for bytecode comparison (turns out I still need to run npm install and npm run build inside the directory I install a local package from).

Still working on the other fail.

@cameel
Copy link
Collaborator Author

cameel commented Jan 25, 2022

The test failure in solc-js external test is actually a bug in solc-js itself. The failing test is buggy but gets skipped due to missing Z3. It does not get skipped here :) We need to merge argotorg/solc-js#592 to fix it.

@cameel
Copy link
Collaborator Author

cameel commented Jan 25, 2022

The problem with Colony is harder. I think we may have actually broken backwards-compatibility with the Typescript change.
It fails with TypeError: soljson.cwrap is not a function in Trufle's Local.load().

Truffle does soljson = originalRequire(compilerPath), where compilerPath is actually a path to the directory containing solc-js checkout (because that's what I'm giving it in the test). In JS that's fine and soljson.compile function is present. Now, however, it's under soljson.default.compile due to default export in our wrapper.js and soljson.compile is undefined.

From what I'm reading, not having default in there was either some kind of hack or it's something that changed by design in TypeScript. In any case, we probably need to change how we export stuff because otherwise JS tools will be able to import it without changes on their side.

@cameel
Copy link
Collaborator Author

cameel commented Jan 25, 2022

@stephensli What do you think about #12583 (comment)? Is that actually a breaking change or is there some easy workaround?

@axic
Copy link
Contributor

axic commented Jan 25, 2022

So maybe we do need to do those manual function exports, so they are named.

@stephen-slm
Copy link

stephen-slm commented Jan 25, 2022

argotorg/solc-js#593

This is the way to get around, it should now function as expected, if you could use my changes to just verify that first, but this should be it.

instead of exporting the default as the object, we rexport the entire content.

… and install it via npm from a local dir instead
@ekpyron ekpyron force-pushed the update-solcjs-commands-for-typescript branch from a06d36d to f46e333 Compare January 26, 2022 10:56
@ekpyron
Copy link
Collaborator

ekpyron commented Jan 26, 2022

Force-pushed to retrigger tests after merging argotorg/solc-js#593
[by the way a bit annoying that circle CI doesn't easily let you retrigger the entire bytecode comparison workflow]

@ekpyron ekpyron merged commit 6f59e22 into develop Jan 26, 2022
@ekpyron ekpyron deleted the update-solcjs-commands-for-typescript branch January 26, 2022 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants