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

[CI] Update rust format version #2550

Merged
merged 2 commits into from
Feb 2, 2019
Merged

[CI] Update rust format version #2550

merged 2 commits into from
Feb 2, 2019

Conversation

tqchen
Copy link
Member

@tqchen tqchen commented Feb 2, 2019

cc @nhynes @ehsanmok

The update the rust docker creates outrage in the CI because of version mismatch. Next time, we might need to consider a safe transition rule(e.g. remove test or relax version for now)

@ehsanmok
Copy link
Contributor

ehsanmok commented Feb 2, 2019

You are right! Thanks for the update. The docker install rustfmt version must match exactly with the version used in .rustfmt.

@nhynes
Copy link
Member

nhynes commented Feb 2, 2019

This is a giant problem with rustfmt. Fortunately we've finally made it to stable nightly (whatever that means) so it'll be quite a while before we update again. At most in 3 years when rust releases another edition.

@tqchen
Copy link
Member Author

tqchen commented Feb 2, 2019

@nhynes can u comment on http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/PR-2550/3/pipeline? If there is a quick fix to this problem

@ehsanmok
Copy link
Contributor

ehsanmok commented Feb 2, 2019

@tqchen That's a new rustfmt rule. Please change make those #![allow(...)] in rust/src/lib.rs in one line:

#![allow(non_camel_case_types, non_snake_case, non_upper_case_globals, unused)]

I verified it locally on rustc 1.34.0-nightly (852701ad6 2019-02-01). However, cargo build gives 5 warnings including

use of deprecated item 'std::sync::atomic::ATOMIC_USIZE_INIT': the `new` function is now preferred`

that should be fixed in frontend merge.

@ehsanmok
Copy link
Contributor

ehsanmok commented Feb 2, 2019

Hopefully as Nick said, we won't get headache after this. The lesson was such changes should be verified independent from current docker setup then commit the changes all at once in docker and rustfmt config.

@tqchen tqchen merged commit 18b2eba into apache:master Feb 2, 2019
@tqchen
Copy link
Member Author

tqchen commented Feb 2, 2019

OK, I temporarily disabled the rust test in tests/task_rust.sh, please send in another PR to fix the files and re-enable the tests(or update the frontend test)

libing4752 pushed a commit to libing4752/tvm that referenced this pull request Feb 18, 2019
merrymercy pushed a commit to merrymercy/tvm that referenced this pull request Feb 18, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
@yzhliu yzhliu mentioned this pull request Mar 2, 2019
28 tasks
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.

3 participants