Skip to content

Conversation

bryanpkc
Copy link
Contributor

Google snappy's configure script defines WORDS_BIGENDIAN in config.h if it detects that the build machine is big-endian. rebar should do the same when building the C++ code for this NIF. This patch allows CouchDB to be built and run on big-endian Linux systems, e.g. ppc64, s390x, etc.

@asfbot
Copy link

asfbot commented Feb 18, 2015

Bryan Chan on dev@couchdb.apache.org replies:

Hello CouchDB developers,

I am new to the list; please be gentle. :-)

I created a pull request on GitHub yesterday (see below) for a relative=
ly
simple patch that would allow CouchDB to be built correctly on big-endi=
an
platforms. What is the usual process for getting this kind of change
approved and committed?

FWIW I have also sent a pull request for the same change to the upstrea=
m
project, snappy-erlang-nif.

Thanks,

Bryan

Bryan Chan
bryan.chan@ca.ibm.com

bryanpkc git@git.apache.org wrote on 2015-02-17 01:22:02 PM:
lds
e
your
feature
please
cket

@janl
Copy link
Member

janl commented Feb 18, 2015

@bryanpkc nice one! Is there a way to add a test case for this?

@asfbot
Copy link

asfbot commented Feb 18, 2015

Jan Lehnardt on dev@couchdb.apache.org replies:
Heya Bryan,

thanks for your contribution! :)

You already followed the regular process! The one thing that=E2=80=99s =
missing now is waiting for someone to do a review. Sending regular =
nudges always is a good strategy, and bringing some patience :)

Best

Jan

relatively
big-endian
upstream
builds
have
ticket

@kxepal
Copy link
Member

kxepal commented Feb 18, 2015

Hi @bryanpkc and welcome! (:

@janl I think the only way to test this is to actually try to built it on big-endian hosts.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't the search of port_env be replaced by lists:keyfind(port_env, 1, CONFIG)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kxepal I am fairly new to Erlang so I probably wasn't writing the smartest code. I have rewritten the script to use lists:keyfind and avoid the fun. Does it look better now?

Copy link
Member

Choose a reason for hiding this comment

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

Much better and, what's important, more friendly for reading by humans (:

@janl
Copy link
Member

janl commented Feb 18, 2015

@kxepal yeah I imagine, and we probably don’t want to re-shuffle the code so you can inject endianness from the tests. Maybe we can have a test that only runs on BE hosts? I just feel bad accepting code without any tests :)

@kxepal
Copy link
Member

kxepal commented Feb 18, 2015

@janl me too, especially such that we cannot test even manually during regular build process. I believe @bryanpkc made these changes because he is able to test it IRL, If he would help us with testing (and even packaging) for big endian hosts that would be awesome.

This situation is the same as our Windows support where there are no actual tests, but group of people who cares about compatibility with that system. And that's still extremely helpful.

So, @bryanpkc how do you feel about such sort of help? (:

@bryanpkc
Copy link
Contributor Author

@janl @kxepal I can build and run "make check" on s390x (and potentially ppc64 if I can borrow the machines). I could learn how to package too. What is the amount of on-going work involved?

@janl
Copy link
Member

janl commented Feb 18, 2015

@bryanpkc ongoing: whenever we make a release, run make test and report here or on the dev@couchdb.apache.org mailing list :)

Ideally we’d have a big endian CI box. I haven’t looked at this at all, but would it be possible to emulate a big endian system on a little endian host using some sort of virtualisation?

If not, would a Raspberry Pi do? We could hook one into our Jenkins :)

@kxepal
Copy link
Member

kxepal commented Feb 18, 2015

@janl with qemu it's possible to emulate different endian. Raspberry PI IIRC is a bi-endian, but little one is preferred.

@bryanpkc
Copy link
Contributor Author

@janl OK, that doesn't sound too bad. I could certainly help with testing that way for now. I am checking if we could help with CI on our hardware, but I cannot promise anything at the moment.

@kxepal, I haven't tried it but I read that the Raspberry Pi is bi-endian, like a few other ARM processors that I have used. The question is whether there is a big-endian OS image for it. Maybe qemu on the Raspberry Pi is the way to go.

@kxepal
Copy link
Member

kxepal commented Feb 18, 2015

@bryanpkc quick googling showed me this one: https://people.debian.org/~aurel32/qemu/ || http://www.aurel32.net/info/debian_mips_qemu.php

Much likely there are the others in the wild.

@janl
Copy link
Member

janl commented Feb 18, 2015

we have some CI hardware, would qemu running there would help?

@kxepal
Copy link
Member

kxepal commented Feb 18, 2015

theoretically...yes, need to try.

Copy link
Member

Choose a reason for hiding this comment

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

you can actually avoid temp variable P by using list:keyfiend expression directly in case.

@janl
Copy link
Member

janl commented Feb 18, 2015

just a note that I think the CI test discussion here is not blocking merging this in

@kxepal
Copy link
Member

kxepal commented Feb 18, 2015

All good. At least it doesn't breaks anything (:
+1 from me with the hope that we'll have big-endian host to build the couch there. Also might worth to check the other NIFs for such compatibility.

@bryanpkc please squash all your commits, remove trailing space from there:

/home/kxepal/projects/couchdb/asf/couchdb-snappy/.git/rebase-apply/patch:25: trailing whitespace.
    <<0,1>> -> 

and let's wait for a while to see what others think about.

@bryanpkc
Copy link
Contributor Author

All commits squashed, trailing space removed. Thanks for the review.

@bryanpkc
Copy link
Contributor Author

@janl I have briefly looked at Travis CI; it seems to support only Ubuntu as a build environment. Will it work with Debian? There isn't a Ubuntu build for s390x yet, AFAIK.

@janl
Copy link
Member

janl commented Feb 19, 2015

@bryanpkc I don’t think Travis is gonna help with running anything but their stock environment, but we have a Jenkins setup that we can use.

@bryanpkc
Copy link
Contributor Author

@janl @kxepal Just a gentle reminder about this pull request. It has already been merged upstream (snappy-erlang-nif). Thanks.

@asfgit asfgit merged commit 8e008be into apache:master Mar 16, 2015
@janl
Copy link
Member

janl commented Mar 16, 2015

merged!

@bryanpkc bryanpkc deleted the big-endian branch May 20, 2015 22:21
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.

5 participants