Skip to content

Conversation

@marcelklehr
Copy link
Member

fixes #854

Copy link
Contributor

@J0WI J0WI left a comment

Choose a reason for hiding this comment

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

Please run update.sh and make sure that all tests are passing.

@marcelklehr marcelklehr force-pushed the master branch 2 times, most recently from db3f503 to 1d2591d Compare September 19, 2019 19:47
@marcelklehr
Copy link
Member Author

Mmh. based on the travis matrix, it's not really clear to me, what's the problem. Debian fpm x86 always seems to fail, with other targets failing at random it seems 🤔

@maxmeyer
Copy link

maxmeyer commented Oct 3, 2019

@marcelklehr Did you try to build those failing images locally?

@J0WI
Copy link
Contributor

J0WI commented Oct 3, 2019

Debian uses different include locations for each architecture

@marcelklehr
Copy link
Member Author

@maxmeyer I tried yes, but it's not quite easy to do and they fail locally as well.

@J0WI
Copy link
Contributor

J0WI commented Oct 26, 2019

@tianon do you know any solution for the architecture relative library paths on Debian?

@norweeg
Copy link

norweeg commented Oct 26, 2019

@J0WI does this help at all? https://wiki.debian.org/Multiarch/LibraryPathOverview
I may not understand all that's going on in this PR, but I am anxious to see this merged

@norweeg
Copy link

norweeg commented Oct 26, 2019

Pretty sure I found the problem.

@marcelklehr, the lines where you have
test -f "/usr/include/gmp.h" || ln -s "/usr/include/$debMultiarch/gmp.h" /usr/include/gmp.h; \
or similar, i think it should be changed to
test -e "/usr/include/gmp.h" || ln -s "/usr/include/$debMultiarch/gmp.h" /usr/include/gmp.h; \

test -f means the file exists AND is a regular file, which based on the fact that you create a symbolic link on failure means that this file will never be a regular file. test -e is a check for simple existence of a file with no other conditions. If you want to check if /usr/include/gmp.h exists and is specifically a symbolic link, then use test -L instead

@marcelklehr
Copy link
Member Author

Now, we are where I've been before. Still all debian i386 builds are failing. :/

@norweeg
Copy link

norweeg commented Oct 26, 2019

@marcelklehr hmm... the build logs indicate it can't find gmp.h when building in i386... wish I could see where it was trying to look for it. The fpm image is based on debian 'buster', and according to https://packages.debian.org/buster/i386/libgmp-dev/filelist the header file should be at /usr/include/i386-linux-gnu/gmp.h. Wish I could see what dpkg-architecture --query DEB_BUILD_MULTIARCH returns on Debian 'buster' i386. I suspect it isn't "i386-linux-gnu"

@tilosp
Copy link
Member

tilosp commented Oct 26, 2019

take a look at https://travis-ci.org/nextcloud/docker/jobs/603287089#L4009

docker uses /bin/sh to build which doesn't support [[ ]], replacing it with [ ] should fix this

@norweeg
Copy link

norweeg commented Oct 26, 2019

@tilosp literally popped up seconds before I clicked "comment" to say the same thing! Also, can confirm: it builds for me with the suggested change

@marcelklehr
Copy link
Member Author

ooph, fingers crossed. Once the build passes I'll do a rebase to clean things up a bit... 😅

@tilosp
Copy link
Member

tilosp commented Oct 26, 2019

It breaks the build of some of the full example dockerfiles because they try to install gmp.
Simply removing it from those Dockerfiles should fix it.

@marcelklehr
Copy link
Member Author

Hah, if I only I had looked at the examples.

@marcelklehr
Copy link
Member Author

@tilosp @J0WI Have you thought about creating a CONTRIBUTING.md file for this repo, where people can learn how to contribute, how things are supposed to work, some basic info about the CI process? That would go a long way, I think :)

@marcelklehr marcelklehr force-pushed the master branch 2 times, most recently from db279d0 to 2e5ae50 Compare October 26, 2019 20:52
@tilosp tilosp requested a review from J0WI October 26, 2019 21:00
@tilosp
Copy link
Member

tilosp commented Oct 26, 2019

yeah having a CONTRIBUTING.md to help out new contributors makes a lot of sense.
I am not really into writing docs, and i am also not sure what info would be useful.
Maybe explaining which files to edit and which are auto-generated by update.sh.

@marcelklehr what info about the ci would haven been useful to you? Simply explaining which Dockerfiles get build?

btw could you also remove the gmp from the full/fpm-alpine example? It doesn't break the build, but building it twice still wastes resources.

Signed-off-by: Marcel Klehr <mklehr@gmx.net>
Copy link
Contributor

@J0WI J0WI left a comment

Choose a reason for hiding this comment

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

The symlink looks a bit hacky to me, but if this is the only variant working on all platforms I'm fine with it.

@tilosp tilosp merged commit 8fac176 into nextcloud:master Oct 27, 2019
@matthiasbaldi
Copy link

When will this be released? I will test it then asap :) 👍

@tilosp
Copy link
Member

tilosp commented Oct 27, 2019

Once docker-library/official-images#6867 is reviewed and merged it will be build and published by https://doi-janky.infosiftr.net/.

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.

Bookmarks is missing php-gmp

6 participants