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

Implementing arm support with #66 #82

Merged
merged 1 commit into from
Jun 18, 2020

Conversation

step21
Copy link
Contributor

@step21 step21 commented Jun 17, 2020

What I did

Implemented arch check and arm compile as per

Related issue: #
#66

How I did it

Described in issue above. Install_arm and install_osx both call compile_solc with osx checking the ox parameter and arm not checking anything so far.

How to verify it

Checklist

  • I have confirmed that my PR passes all linting checks
  • I have included test cases
  • I have updated the documentation (README.md)
  • I have added an entry to the changelog

No test cases as I honestly wasn't sure what to test, and looking for examples there were very few tests related to install. Rest of checklist will follow.

@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #82 into master will decrease coverage by 0.29%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
- Coverage   77.15%   76.85%   -0.30%     
==========================================
  Files           8        8              
  Lines         591      605      +14     
  Branches      137      140       +3     
==========================================
+ Hits          456      465       +9     
- Misses        105      108       +3     
- Partials       30       32       +2     
Impacted Files Coverage Δ
solcx/install.py 72.72% <68.75%> (-0.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58f7bd1...d6bbef3. Read the comment docs.

@iamdefinitelyahuman
Copy link
Contributor

Thanks for your contribution 🙏

Installation testing is mostly handled indirectly, as the entire suite will fail if solc can't install. Unfortunately this only really covers linux and windows - the compile time is so slow it timed out the CI when I tried. The test suite should include some level of mocking to make sure that the compile functions are at least called properly - but it doesn't today, so I can't rightly ask you to be the one to add those tests :) a new major release is on the nearish horizon, it will definitely be handled by then.

Copy link
Contributor

@iamdefinitelyahuman iamdefinitelyahuman left a comment

Choose a reason for hiding this comment

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

Looking good, just need to address one issue.

solcx/install.py Outdated
LOGGER.info("solc {} successfully installed at: {}".format(version, binary_path))
finally:
lock.release()
if arch == "arm":
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be more fitting as the first if statement in the sequence of if/elif starting at line 244. It's important that it happens within the context of try/finally so that the process lock is released when it completes.

Added arm check and support with reordered install_solc elifs
@step21
Copy link
Contributor Author

step21 commented Jun 18, 2020

Also, install_deps currently give a warning on raspbian (Ubuntu Aarch64 however should be fine) but it still works and I also submitted a pathc to Solidity that it treats Raspbian as Debian.

@step21
Copy link
Contributor Author

step21 commented Jun 18, 2020

ethereum/solidity#9221

@iamdefinitelyahuman
Copy link
Contributor

Also, install_deps currently give a warning on raspbian (Ubuntu Aarch64 however should be fine) but it still works and I also submitted a pathc to Solidity that it treats Raspbian as Debian.

Good call!

There is also a push to expand the range of pre-compiled solc binaries being served, that might be of interest to you - ethereum/solidity#9226

@iamdefinitelyahuman iamdefinitelyahuman merged commit b257618 into ApeWorX:master Jun 18, 2020
@ghost ghost mentioned this pull request Jul 8, 2022
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.

2 participants