Skip to content

Conversation

@cameel
Copy link
Collaborator

@cameel cameel commented Feb 3, 2022

Related to #12583.

This should fix breakage in t_ems_compile_ext_colony and b_bytecode_ems that came up now that we merged argotorg/solc-js#596.

Our publishing process changed so that now the content of dist/ is the actual package. S

  • I adjusted the bytecode compare script so that it now installs from the tarball and gets whatever structure we actually package. This will make it work no matter whether the published content is at the root, in dist/ or anywhere else.
    • EDIT: This breaks bytecode comparison because npm run build:tarball includes the command to download the release binary. Perhaps we should change it so that downloading the binary is a separate step. For now I switched to the solution with hard-coding dist/ subdir.
  • In external tests I'm hard-coding dist/. Using the tarball would complicate them too much IMO. This will unfortunately break once we change the structure of solc-js again but the adjustment will be simple.
  • The PR check script for solc-bin also needed some adjustments (tested with Test for the bytecode compare job with my solc-js fix solc-bin#109):
    • I added the dist/ subdir.
    • I changed it so that installs the package instead of using the repo checkout directly (prepare-report.js now requires this).

@cameel cameel self-assigned this Feb 3, 2022
@cameel cameel changed the title Adjust solc js installation Adjust solc-js installation to match the new release process Feb 3, 2022
@cameel cameel force-pushed the adjust-solc-js-installation branch 2 times, most recently from ee5eff6 to 293e30b Compare February 3, 2022 15:30
@cameel cameel force-pushed the adjust-solc-js-installation branch 3 times, most recently from 6683e30 to 93839c3 Compare February 3, 2022 17:45
@cameel cameel marked this pull request as ready for review February 3, 2022 17:46
@cameel cameel force-pushed the adjust-solc-js-installation branch from 93839c3 to f5b3455 Compare February 3, 2022 17:47
Copy link
Collaborator

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

I haven't followed the latest solc-js, typescript and dist/ developments, but this PR still does not look like the current state of solc-js is particularly backwards compatible :-).

I'll approve and merge it nonetheless to fix CI for now...

@ekpyron ekpyron merged commit afc3d72 into develop Feb 4, 2022
@ekpyron ekpyron deleted the adjust-solc-js-installation branch February 4, 2022 11:26
@cameel
Copy link
Collaborator Author

cameel commented Feb 4, 2022

The thing is, it's compatible only as a published package - because only the content of the dist/ directory gets published. And that's only the kind of compatibility that counts for us. It's not compatible if you're using the repo directly.

The fixes here are basically a mix of switching from using the repo to using the package and adding dist/ in places where we can't easily do that.

These are actually changes that I would expect to be needed already in #12583 and I don't fully understand why they weren't. The switch to dist/ already happended there. My theory is that having "files": ["dist/abi.js", "dist/index.js", ...] in package.json was what allowed the module loader to find these files despite this. Now when argotorg/solc-js#596 replaced it with "files": ["abi.js", "index.js", ...] we got the breakage.

@ekpyron
Copy link
Collaborator

ekpyron commented Feb 4, 2022

I wonder if the separation into a dedicated build directory (i.e. dist/) is really worth all the hassle it's causing :-).

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.

3 participants