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.

Sign up for GitHub

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

Jump to bottom

Fix #25 - run rustfmt, cargo build and make #36

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Fix #25 - run rustfmt, cargo build and make #36

wants to merge 23 commits into from

Conversation

abitrolly
Copy link

@abitrolly abitrolly commented Feb 9, 2019 •
edited by axic

:D

Fixes #25.

@gitcoinbot gitcoinbot mentioned this pull request Feb 9, 2019
Open
@abitrolly abitrolly mentioned this pull request Feb 13, 2019
Closed
@axic
Copy link
Member

axic commented Feb 13, 2019 •
edited

You can also try circleci if that is easier to work with (it is in many cases).

@abitrolly
Copy link
Author

@axic another CI won't help with broken project. Ticked #25 is about running CI, and Travis works well. If we can agree that the work done worthy the bounty assigned there, I can port these instruction to CircleCI.

@axic
Copy link
Member

axic commented Feb 14, 2019

another CI won't help with broken project.

If you refer to #37 then please note this project was compiled several times, last 7 months ago, without issues. If the tools changed without mentioning a breaking change, then I do not believe it is the fault of this project.

and Travis works well

In general we prefer CircleCI in this organisation because it works better, but it is not a necessity to
port everything over, unless it makes work easier.

axic
axic reviewed Feb 15, 2019
View reviewed changes
.travis.yml Outdated
# activate Emscripten
- source ./emsdk/emsdk_env.sh
- cargo build
Copy link
Member

Choose a reason for hiding this comment

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

If you look at the makefile, cargo build requires the asmjs target to be specified. You are better off just running make and no cargo build.

Copy link
Author

Choose a reason for hiding this comment

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

@axic why #25 then says to run cargo build and Makefle as two separate steps?

@abitrolly
Copy link
Author

@axic I updated PR to use make, but the compilation is still failing. #39 There is nothing I can do without knowing Rust.

@hugo-dc
Copy link
Contributor

hugo-dc commented Feb 18, 2020

seems to be blocked by this PR: #38

@abitrolly
Fix ethereumjs#25 - run rustfmt, cargo build and make
7eb6d21 
:D
@abitrolly
Cleanup Travis matrix
ff21403
@abitrolly
Use "new" Ubuntu 16.04 image
faaa0a6
@abitrolly
Split matrix in Js and Rust part
e4f37d3
@abitrolly
Remove strange env and run rustfmt properly
c2b5252
@abitrolly
Remove ubuntu-toolchain-r-test, add Rust asm.js
e0888e5
@abitrolly
Remove CXX from JS test and install Emscripten
de9bbcd
@abitrolly
Use incoming version of Emscripten SDK
e3690f6
@abitrolly
Use latest-incoming version of Emscripten SDK
498ca63 
By advice from @kripken
emscripten-core/emscripten#8045 (comment)
@abitrolly
It is latest-upstream, not latest-incoming
48c3233
@abitrolly
Let's try new latest which should be 1.38.27 now
1e677f4 
If it fails, then `./emsdk update-tags` might be needed.
@abitrolly
Use last good Rust/EmSDK
403fd30
@abitrolly
No rustfmt for Rust 1.27.2
111e574 
https://travis-ci.org/ethereumjs/rustbn.js/jobs/493650749#L201
@abitrolly
Be more explicit about sdk version
c0d6934 
Error: No tool or SDK found by name '1.38.21'.
@abitrolly
EMSDK version needs suffix
7916a2c
@abitrolly
cargo build is not needed
07774ce
@abitrolly
Try to compile on Travis with latest Rust and EMSDK
8d75f41
@abitrolly
Switch to Bionic and run cargo fmt as a separate job
9e2aceb
@abitrolly
Fix typo in job language specifier
551dd0f
@abitrolly
Install rustfmt
511f664
@abitrolly
Cleanup comments
de44a9a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axic axic axic left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI should run cargo build
3 participants
@abitrolly @axic @hugo-dc