Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimal changes to use the same version of emscripten on Travis and Circleci. #4486

Merged
merged 2 commits into from
Nov 21, 2018

Conversation

ekpyron
Copy link
Member

@ekpyron ekpyron commented Jul 11, 2018

Experiments with minimal changes to achieve the goal of #3491.
Closes #3491.

This results in:

  • Travis: emscripten changed (1.37.21, boost changed (1.67)
  • Circleci: emscripten unchanged (1.37.21), boost changed (1.67)

@ekpyron ekpyron self-assigned this Jul 11, 2018
@ekpyron ekpyron force-pushed the travisEmscriptenMinimal branch from 644f2aa to fce3307 Compare July 11, 2018 23:01
@ekpyron ekpyron changed the title [WIP] Minimal changes to use emscripten 1.37.21 on Travis (same as on Circleci). [WIP] Minimal changes to use the same version of emscripten on Travis and Circleci. Jul 11, 2018
@ekpyron ekpyron force-pushed the travisEmscriptenMinimal branch 4 times, most recently from 8b06cef to 2cf0ab6 Compare July 12, 2018 10:25
@ekpyron
Copy link
Member Author

ekpyron commented Jul 12, 2018

@axic So this is the minimal set of changes I got running so far... However, the soljson.js size is even a bit larger than with newer emscripten versions (although still less than 10% larger than the original).

If we want to still merge this, note that we need to reenable the travis and circleci caches first.

@ekpyron ekpyron force-pushed the travisEmscriptenMinimal branch from 2cf0ab6 to dc37b11 Compare July 12, 2018 13:42
@ekpyron ekpyron requested a review from axic July 12, 2018 18:49
@ekpyron ekpyron changed the title [WIP] Minimal changes to use the same version of emscripten on Travis and Circleci. Minimal changes to use the same version of emscripten on Travis and Circleci. Jul 12, 2018
chriseth
chriseth previously approved these changes Jul 12, 2018
@@ -90,8 +90,9 @@ matrix:
before_install:
- nvm install 8
- nvm use 8
- docker pull trzeci/emscripten:sdk-tag-1.35.4-64bit
- docker pull trzeci/emscripten:sdk-tag-1.37.21-64bit
Copy link
Member

Choose a reason for hiding this comment

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

So does it needs an emscripten update then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - only updating both emscripten and boost currently works - updating either one alone still results in errors that we don't have a solution for so far. However, this only updates the emscripten version on travis to the same version used on circleci and does not update any further than that.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, in that case I'm not sure we should merge without running some benchmarks. It is kind of weird, because on circle 1.37.21 compiles with both versions of boost, while on travis 1.37.21 needs the boost upgrade.

Well, we could use the circle output to benchmark against he nightlies, since both versions were created.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think on circle everything is compiled in docker containers, whereas on travis parts are built in a docker container and parts are not - my conjecture would be that that's causing the differences. I'd have to recheck, though.

@codecov-io
Copy link

codecov-io commented Aug 1, 2018

Codecov Report

Merging #4486 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4486   +/-   ##
========================================
  Coverage    88.09%   88.09%           
========================================
  Files          308      308           
  Lines        31247    31247           
  Branches      3751     3751           
========================================
  Hits         27528    27528           
  Misses        2465     2465           
  Partials      1254     1254
Flag Coverage Δ
#all 88.09% <ø> (ø) ⬆️
#syntax 28.98% <ø> (ø) ⬆️

@chriseth
Copy link
Contributor

chriseth commented Aug 6, 2018

Would postpone this to 0.5.1

@ekpyron
Copy link
Member Author

ekpyron commented Aug 6, 2018

@chriseth Agreed.

@ekpyron ekpyron mentioned this pull request Aug 14, 2018
@ekpyron ekpyron force-pushed the travisEmscriptenMinimal branch 3 times, most recently from 323a4c6 to 81c62e5 Compare November 14, 2018 11:21
@axic
Copy link
Member

axic commented Nov 21, 2018

I think this is ready for merging.

@axic
Copy link
Member

axic commented Nov 21, 2018

Oh, this was blocked by Remix having 0.5.0 support. That is available as of today on https://remix-alpha.ethereum.org.

@ekpyron ekpyron force-pushed the travisEmscriptenMinimal branch from 81c62e5 to 119aa10 Compare November 21, 2018 14:24
@ekpyron
Copy link
Member Author

ekpyron commented Nov 21, 2018

Rebased.

chriseth
chriseth previously approved these changes Nov 21, 2018
@axic
Copy link
Member

axic commented Nov 21, 2018

We should mention this in the changelog... but lets wait for travis to finish first.

-DBoost_PROGRAM_OPTIONS_LIBRARY_RELEASE="$WORKSPACE"/boost_1_67_0/libboost_program_options.a \
-DBoost_REGEX_LIBRARY_RELEASE="$WORKSPACE"/boost_1_67_0/libboost_regex.a \
-DBoost_SYSTEM_LIBRARY_RELEASE="$WORKSPACE"/boost_1_67_0/libboost_system.a \
-DBoost_UNIT_TEST_FRAMEWORK_LIBRARY_RELEASE="$WORKSPACE"/boost_1_67_0/libboost_unit_test_framework.a \
Copy link
Member

Choose a reason for hiding this comment

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

@christianparpart can you review this file?

@axic
Copy link
Member

axic commented Nov 21, 2018

Previous travis run was successful: https://travis-ci.org/ethereum/solidity/builds/457964218

@axic axic merged commit 10a2e5d into develop Nov 21, 2018
@axic axic deleted the travisEmscriptenMinimal branch November 21, 2018 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants