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

added toolchain #166

Merged
merged 3 commits into from
May 2, 2022
Merged

added toolchain #166

merged 3 commits into from
May 2, 2022

Conversation

nargeslein
Copy link
Contributor

Fixes #issue.

added rust-toolchain file in order to run cargo build outside of docker and use the right version as the docker image would

Test Plan

run cargo buildand rustup showto view that the same settings as by the docker image are used

Release notes

Optionally add notes, suggestions and warnings for the releaser. They will be included in the release changelog.

@nargeslein nargeslein requested a review from a team as a code owner April 24, 2022 06:38
@mrq1911
Copy link

mrq1911 commented Apr 24, 2022

spend 30 minutes rebuilding until I stumbled upon right version 😿

Copy link
Contributor

@vkgnosis vkgnosis left a comment

Choose a reason for hiding this comment

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

https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file is the documentation for the toolchain file. I'm not sure about this PR because we aren't "pinned to a specific version". We use whatever is the current stable release of Rust like many other projects. We might want to document this in the readme.

@nlordell
Copy link
Contributor

We use whatever is the current stable release of Rust

I like the addition of the toolchain file, as it can express exactly this. We can make the file it specify that the project uses the stable channel, so others know they should have an up-to-date stable Rust.

rust-toolchain Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Apr 27, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@nargeslein
Copy link
Contributor Author

@vkgnosis the docker file does tie it to a specific version which I had tried to mimic in the toolchain file. We spend some time compiling the project during the hackathon until we found out which Rust version needs to be used. I have updated the toolchain file to point to the stable version as @nlordell suggestes :-),

@nargeslein
Copy link
Contributor Author

have read the CLA Document and I hereby sign the CLA

@nlordell
Copy link
Contributor

the docker file does tie it to a specific version which I had tried to mimic in the toolchain file

Good point. I see rust:1 which should resolve to the latest stable 1.x version according to the Dockerhub reference. This should be the same as stable channel until Rust 2.x comes out.

@vkgnosis
Copy link
Contributor

We spend some time compiling the project during the hackathon until we found out which Rust version needs to be used.

This surprised me because that is what I assume by default when I build a Rust project: it builds with the current stable unless mentioned otherwise. I'm also used to always having the current stable installed and up to date so it hadn't occured to me that someone would try other versions. I'm probably biased by what I'm used to and I can see it might be more confusing for others so I'm fine with the PR.

@nargeslein
Copy link
Contributor Author

This surprised me because that is what I assume by default when I build a Rust project: it builds with the current stable unless mentioned otherwise. I'm also used to always having the current stable installed and up to date so it hadn't occured to me that someone would try other versions. I'm probably biased by what I'm used to and I can see it might be more confusing for others so I'm fine with the PR.

@vkgnosis many thanks for sharing these information. I am relatively new to Rust and do not use it very often (yet!), this is why I appreciate these information. Unfortunately my laptop is very slow and the first compilation took a while. Do you have any advice on a hardware setup for me? E.g. what are you using if I may ask. Thanks.

@vkgnosis
Copy link
Contributor

Unfortunately this Rust project is slow to build and there is no single solution. Rust in general is slow to build but we also have a large number of dependencies.
There are some things you can do speed up compilation locally that you can find through internet searches or by asking in some of the rust communities . Things like https://endler.dev/2020/rust-compile-times/ and https://nnethercote.github.io/perf-book/compile-times.html . Although these might also be confusing to a rust beginner.
To get large speed ups we as the developers of the project probably have to invest more time reducing dependencies and slow to compile code etc.

@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2022

Codecov Report

Merging #166 (22f97ad) into main (3011d69) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
+ Coverage   64.55%   64.57%   +0.02%     
==========================================
  Files         185      185              
  Lines       38602    38602              
==========================================
+ Hits        24920    24928       +8     
+ Misses      13682    13674       -8     

@nlordell
Copy link
Contributor

nlordell commented Apr 29, 2022

Hmm... Looks like there are CLA issues. Any chance you could rebase/squash this commit using a user.email that matches your GitHub user? I think the issue is:

Narges seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

Sorry for the inconvenience!

@nargeslein
Copy link
Contributor Author

Hmm... Looks like there are CLA issues. Any chance you could rebase/squash this commit using a user.email that matches your GitHub user? I think the issue is:

Narges seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

Sorry for the inconvenience!

Hello @nlordell many thanks for following up! I had tried to fix it without success. I will try rebase/squash. Thanks for the support.

Update rust-toolchain

using stable version and components

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

reverted changes
@nargeslein
Copy link
Contributor Author

Hello @nlordell, @vkgnosis I think I managed to clean up the commits by rebasing/squashing after some fiddling around. I hope it's fine now :-)

@mrq1911
Copy link

mrq1911 commented May 1, 2022

We spend some time compiling the project during the hackathon until we found out which Rust version needs to be used.

This surprised me because that is what I assume by default when I build a Rust project: it builds with the current stable unless mentioned otherwise. I'm also used to always having the current stable installed and up to date so it hadn't occured to me that someone would try other versions. I'm probably biased by what I'm used to and I can see it might be more confusing for others so I'm fine with the PR.

this always depends on what you have configured as local default toolchain. i am using for most of my projects some recent nightly build, so if your project don't have toolchain defined, cargo will just my default and fail, but if you define your toolchain version, cargo just automatically uses that and it works for every environment (including ci :)

@nlordell
Copy link
Contributor

nlordell commented May 2, 2022

I think I managed to clean up the commits by rebasing/squashing after some fiddling around.

Looks like it worked! Thanks for the contribution and for bearing with us through our brand new CLA process 😄

@nlordell nlordell enabled auto-merge (squash) May 2, 2022 07:24
@nlordell nlordell merged commit d0eed4b into cowprotocol:main May 2, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants