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: Travis support for PowerPC64 #17402

Closed
wants to merge 2 commits into from
Closed

Conversation

@elichai
Copy link
Contributor

elichai commented Nov 7, 2019

Apparently travis has powerpc64 support even though it's undocumented.

I found a couple of caveats though:

  1. Compact glibc support doesn't work (should be solved in #14066 )
  2. it fails building OpneSSL from depends (fanquake tried debugging and it looks like it can't recognize the platform).
  3. Docker doesn't recognize the platform correctly, so I had to use ppc64le/ubuntu:18.04 instead of library/ubuntu:18.04 (this is also maintained by Docker themselves https://github.com/docker-library/official-images#architectures-other-than-amd64)
  4. functional tests are timing out.
elichai added 2 commits Nov 7, 2019
@elichai elichai changed the title Travis support for PowerPC64 ci: Travis support for PowerPC64 Nov 7, 2019
@fanquake fanquake added the Tests label Nov 7, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 7, 2019

Nice find. However, given that it is not documented, it might go away or break any time and we shouldn't expect travis to provide any support for it.

@elichai

This comment has been minimized.

Copy link
Contributor Author

elichai commented Nov 7, 2019

@MarcoFalke I can add allow_failures so the builds won't fail if it doesn't work. (altough that means manually checking if PPC passed)

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 7, 2019

altough that means manually checking if PPC passed

As if anyone would do that

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 7, 2019

Also according to #6466 we were looking for a be build, not le. Travis is already pretty slow these days and adding another build (which is mostly redundant to the other le builds) does not help in that regard.

@elichai

This comment has been minimized.

Copy link
Contributor Author

elichai commented Nov 7, 2019

I agree that if it was big endian it would've been nicer.
But I do think it's valuable to have CI for every platform we're aiming to support, as it's very easy to break support with one the second we use non C++ stuff (i.e. libc, asm etc.)

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 7, 2019

Concept ACK

Very nice find! :)

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 7, 2019

we use non C++ stuff (i.e. libc, asm etc.)

We use the Ubuntu Bionic libc on all builds, no? So at least that shouldn't matter.

@elichai

This comment has been minimized.

Copy link
Contributor Author

elichai commented Nov 7, 2019

You mean when distributing binaries, right?

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 7, 2019

The ci system is running (at least on travis) in ubuntu:bionic docker containers.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 7, 2019

Also according to #6466 we were looking for a be build, not le. Travis is already pretty slow these days and adding another build (which is mostly redundant to the other le builds) does not help in that regard.

I tend to agree. Adding a big-endian build / test (irrespective of platform) would be great. PPC LE is worth testing on intermittently, but don't think it needs to run for every PR and commit (I made a similar argument for RISC-V in #17089).

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 7, 2019

Don't get me wrong, I appreciate the work you put in here and I am going to steal it for my projects (e.g. intermittent tests as suggested by @laanwj), but it might not be the best fit for the Bitcoin Core repo.

@elichai

This comment has been minimized.

Copy link
Contributor Author

elichai commented Nov 8, 2019

I guess I agree, about the times.
for the same reason RISC-V didn't get merged, this i guess shouldn't get merged either.

But this just makes me wonder if travis is hurting us in the long run.
Giving up on testcases because "travis is already too slow" is a bad sign (not talking specifically about this PR, for that sense it can be even running valgrind).

Anyway, that's unrelated to this PR(i'll see if I have time to look into serious infrastructure (I think this is already a serious enough project to have serious infra)).

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 8, 2019

It is possible to send them money to speed it up for us (#16148 (comment)). However, that will also slow it down for "local" travis testing in forks.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 10, 2019

But this just makes me wonder if travis is hurting us in the long run.

Well a bad craftsman blames his tools. Yes, travis is problematic sometimes, but also our test suite is very much on the heavy side. Both the build and tests somehow take very long compared to other projects.

If it means thorough testing that's good, of course. But maybe it'd be nice to have a subset of fast tests that we can run as often on as many platforms as possible. Maybe foregoing for ex. depends builds.

@elichai

This comment has been minimized.

Copy link
Contributor Author

elichai commented Nov 10, 2019

But this just makes me wonder if travis is hurting us in the long run.

Well a bad craftsman blames his tools. Yes, travis is problematic sometimes, but also our test suite is very much on the heavy side. Both the build and tests somehow take very long compared to other projects.

If it means thorough testing that's good, of course. But maybe it'd be nice to have a subset of fast tests that we can run as often on as many platforms as possible. Maybe foregoing for ex. depends builds.

I still somewhat blame Travis (i.e. Using a robust Jenkins like server everything is parallel in it's own spots/machine so new jobs don't make the tests longer).

But you're right, maybe a good goal is to start figuring out what takes so much time and how can we fix that.
Is it compile time or test time? can we change includes and/or libraries to make compile time faster? can we change test framework(or the way we test) to make tests run faster? Should we introduce ccache into the CI? (These are all open questions in my mind, I never looked into them and the answer might be "no" to all of them)

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 10, 2019

Is it compile time or test time?

Without cache it is compile time (due to boost and serialize.h).
With cache it is install time (apt-get) and a bit of the functional tests.

can we change includes and/or libraries to make compile time faster?

Yes, see e.g. #17401 and #17307.

can we change test framework(or the way we test) to make tests run faster?

Yes, see e.g. #15858 or #16613.

Should we introduce ccache into the CI?

Yes (we already do for normal compiles), but it is not picked up by depends: #17103

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 22, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17591 (ci: Add big endian platform - s390x by elichai)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@elichai

This comment has been minimized.

Copy link
Contributor Author

elichai commented Nov 25, 2019

@MarcoFalke Am I dreaming or are they also supporting s390x which is BE?
I'll test with a small travis to check that it's actually BE and then PR it :)

EDIT: Definitely Big Endian :)

MarcoFalke added a commit that referenced this pull request Nov 25, 2019
da1f153 Add s390x tests to travis (Elichai Turkel)
2fa65e0 Add ci script to install on s390x (Elichai Turkel)

Pull request description:

  Discovered this as part of #17402 and a conversation with gmaxwell.

  You can see here that the platform is indeed BE: https://travis-ci.org/elichai/bitcoin/jobs/616656410#L36

  This closes #6466

ACKs for top commit:
  MarcoFalke:
    ACK da1f153

Tree-SHA512: e7e94e54e220257d91b24fddc79eab2bcaaadf0b2d1e7e6872d9757808ab2541728f00b1f3ab7e343305c0e7d91bb48a17a3f9621f6fff6c9fe6cde6682de408
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 25, 2019

Needs rebase
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 25, 2019
da1f153 Add s390x tests to travis (Elichai Turkel)
2fa65e0 Add ci script to install on s390x (Elichai Turkel)

Pull request description:

  Discovered this as part of bitcoin#17402 and a conversation with gmaxwell.

  You can see here that the platform is indeed BE: https://travis-ci.org/elichai/bitcoin/jobs/616656410#L36

  This closes bitcoin#6466

ACKs for top commit:
  MarcoFalke:
    ACK da1f153

Tree-SHA512: e7e94e54e220257d91b24fddc79eab2bcaaadf0b2d1e7e6872d9757808ab2541728f00b1f3ab7e343305c0e7d91bb48a17a3f9621f6fff6c9fe6cde6682de408
@elichai

This comment has been minimized.

Copy link
Contributor Author

elichai commented Dec 26, 2019

I don't see a good reason to leave this open.

@elichai elichai closed this Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.