Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

test.sh: use cargo --target for platforms other than linux, win or mac #9650

Merged
merged 4 commits into from
Oct 1, 2018

Conversation

gabreal
Copy link
Contributor

@gabreal gabreal commented Sep 25, 2018

test runs need cargo --target option for cross-compilation and should then build only but not run the tests.

@parity-cla-bot
Copy link

It looks like @gabreal signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@gabreal gabreal changed the title test.sh: use cargo --target for platforms other than linux, win or mac WIP test.sh: use cargo --target for platforms other than linux, win or mac Sep 25, 2018
@gabreal gabreal force-pushed the ci-tests-target branch 2 times, most recently from 16ecefc to 8e1b3bf Compare September 25, 2018 19:18
@gabreal gabreal changed the title WIP test.sh: use cargo --target for platforms other than linux, win or mac test.sh: use cargo --target for platforms other than linux, win or mac Sep 25, 2018
Copy link
Contributor

@5chdn 5chdn left a comment

Choose a reason for hiding this comment

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

does this mean we don't run tests for anything not linux, windows, macos?

@@ -4,6 +4,7 @@
FEATURES="json-tests,ci-skip-issue"
OPTIONS="--release"
VALIDATE=1
THREADS=8
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to hardcode the number of threads?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just set it as a variable here and did not want to change the behaviour in this pr. But generally I think it's okay to have a fixed number - unless we decide not to run jobs concurrently and then dynamically set it to the number of cores available on a runner node.

Copy link
Contributor

Choose a reason for hiding this comment

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

Theoretically Cargo is supposed to determine the ideal number of threads. In practice I'm not sure it's better than hardcoding it.

Copy link
Contributor

Choose a reason for hiding this comment

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

cargo help test
If you want to control the
number of simultaneous running test cases, pass the --test-threads option
to the test binaries:

cargo test -- --test-threads=1

MB
if darwin -> sysctl -n hw.ncpu
if windows -> echo %NUMBER_OF_PROCESSORS%
if linux -> $(nproc)

test.sh Outdated
echo "________Validate build________"
time cargo check --no-default-features
time cargo check --manifest-path util/io/Cargo.toml --no-default-features
time cargo check --manifest-path util/io/Cargo.toml --features "mio"
Copy link
Contributor

Choose a reason for hiding this comment

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

why do they not require a $TARGET?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They use the default target. I don't want to break the script for those who run it locally and probably don't have the CARGO_TARGET variable set because it's not required and specific to the CI.

test.sh Outdated


# Running the C++ example
echo "________Running the C++ example________"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is only run in win,mac, and linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, @tomaka told me that that only makes sense.

test.sh Outdated
time cargo check --target $CARGO_TARGET --manifest-path util/io/Cargo.toml --features "mio"

# Validate chainspecs
echo "________Validate chainspecs________"
Copy link
Contributor

Choose a reason for hiding this comment

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

script duplicates a bit here, can we simplify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can do that.

@5chdn 5chdn added this to the 2.2 milestone Sep 25, 2018
@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M1-ci 🙉 Continuous integration. labels Sep 25, 2018
@gabreal
Copy link
Contributor Author

gabreal commented Sep 25, 2018

does this mean we don't run tests for anything not linux, windows, macos?

exactly, cross-compiled binaries won't be run but only compiled.

test.sh Outdated
case $CARGO_TARGET in
# native builds
(x86_64-unknown-linux-gnu|x86_64-apple-darwin|x86_64-pc-windows-msvc|'')
validate
Copy link
Contributor

Choose a reason for hiding this comment

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

You should pass --target $CARGO_TARGET here as well.

test.sh Outdated
cd ../..

# Running tests
cargo_test $@
Copy link
Contributor

Choose a reason for hiding this comment

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

And --target here.

@@ -4,6 +4,7 @@
FEATURES="json-tests,ci-skip-issue"
OPTIONS="--release"
VALIDATE=1
THREADS=8
Copy link
Contributor

Choose a reason for hiding this comment

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

Theoretically Cargo is supposed to determine the ideal number of threads. In practice I'm not sure it's better than hardcoding it.

@5chdn 5chdn added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Sep 27, 2018
@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 1, 2018
@5chdn 5chdn merged commit f3b806b into master Oct 1, 2018
@5chdn 5chdn deleted the ci-tests-target branch October 1, 2018 10:55
dvdplm added a commit that referenced this pull request Oct 2, 2018
…mon-deps

* origin/master:
  CI: Remove unnecessary pipes (#9681)
  test.sh: use cargo --target for platforms other than linux, win or mac (#9650)
  ci: fix push script (#9679)
  Hardfork the testnets (#9562)
  Calculate sha3 instead of sha256 for push-release. (#9673)
  ethcore-io retries failed work steal (#9651)
  fix(light_fetch): avoid race with BlockNumber::Latest (#9665)
  Test fix for windows cache name... (#9658)
  refactor(fetch) : light use only one `DNS` thread (#9647)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M1-ci 🙉 Continuous integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants