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

#180 multistage build on master branch #211

Merged
merged 12 commits into from Nov 23, 2020
Merged

Conversation

smellman
Copy link
Contributor

@@ -1,4 +1,4 @@
FROM postgres:%%PG_MAJOR%%
FROM debian:%%DEBIAN_VERSION%% as builder
Copy link
Member

Choose a reason for hiding this comment

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

Question:

  • It is possible to use - same base image as in the second stage ?
    imho: it will be more secure;

Extreme case: when somebody building manually .. or just reuse this Docker script ..

  • postgres:13 pulled the latest .. but the base debian is not refreshed the same time.

Or there are a few day difference in the upstream "debian" and "postgres:13" ..
so the "postgres:13" base layer is older version ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which is better, "postgres:12" or "postgres:13"?
I think Multistage build is useful for speed up building both 12-master and 13-master so I use debian:buster-slim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, Original comment is not focus the speed.
OK, I will change code.

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 change my code but this postgis hash may fail in test so I change this PR to Draft.

Run tests: 143
Failed: 1
make: *** [runtest.mk:10: check-regress] Error 1

@smellman smellman marked this pull request as draft November 2, 2020 03:25
@smellman
Copy link
Contributor Author

smellman commented Nov 2, 2020

@ImreSamu Do you check current source code?
Now it will be fail in postgis by upstream both my branch and current master branch.
But now, my branch enable to build each modules(if upgrade gdal, it may build gdal only).

@ImreSamu
Copy link
Member

ImreSamu commented Nov 3, 2020

@smellman :

Now it will be fail in postgis by upstream both my branch and current master branch.

I see

Run tests: 143
Failed: 1
make: *** [runtest.mk:10: check-regress] Error 1

I will debug in the next few days ..

@ImreSamu
Copy link
Member

ImreSamu commented Nov 3, 2020

@smellman

(based on the travis log )

imho: The postgis git hash is very strange.

maybe the git reset --hard FETCH_HEAD .. setting always for the "origin" HEAD ?

+ git init
�[0mInitialized empty Git repository in /usr/src/postgis/.git/
�[91m+ git remote add origin https://git.osgeo.org/gitea/postgis/postgis.git
�[0m�[91m+ git fetch --depth 1 origin :a7ff53ff535eae8579a374a11a785ed053acd3c7
�[0m�[91mFrom https://git.osgeo.org/gitea/postgis/postgis
 * [new ref]                    -> a7ff53ff535eae8579a374a11a785ed053acd3c7
�[0m�[91m+ git reset --hard FETCH_HEAD
�[0mHEAD is now at 6b5aaea Have berrie bots prepend PG path to PATH

imho ( temporary ) :

@smellman
Copy link
Contributor Author

smellman commented Nov 3, 2020

maybe the git reset --hard FETCH_HEAD .. setting always for the "origin" HEAD ?

$ git init
$ git remote add origin https://git.osgeo.org/gitea/postgis/postgis.git
$ git fetch --depth 1 origin :13f821e5906e1a2663eb43b410bb6bd1deba88b6
$ git log
fatal: your current branch 'master' does not have any commits yet
$ git reset --hard FETCH_HEAD
HEAD is now at 873d3e5 Do not use abs_path if not strictly needed
$ git log
commit 873d3e5a099dff60614be54c65657eebab184e80 (grafted, HEAD -> master, 13f821e5906e1a2663eb43b410bb6bd1deba88b6)
Author: Sandro Santilli <strk@kbt.io>
Date:   Tue Nov 3 10:56:27 2020 +0100

    Do not use abs_path if not strictly needed

    Hopefully fixes windows builds (winnie).
    See #4781

It only pull specify hashed branch as one commit.

imho ( temporary ) :

* please use the latest ( known built ) hashes https://github.com/postgis/docker-postgis/blob/master/13-master/Dockerfile#L14

I use it but I get same error in build.
It seems it raise from geos or other program.
I can build it when my first commit so I will check histories.

@smellman
Copy link
Contributor Author

smellman commented Nov 3, 2020

Sorry I find the bug in my code.

@smellman
Copy link
Contributor Author

smellman commented Nov 3, 2020

@ImreSamu I try to build current 12-master.

Run tests: 141
Failed: 2
make: *** [runtest.mk:10: check-regress] Error 2

I think postgres:12 image was changed something.

@ImreSamu
Copy link
Member

ImreSamu commented Nov 4, 2020

@smellman

It only pull specify hashed branch as one commit.

imho:
Please test with the Postgis 2.0.0 version - and test the head NEWS content;
it is different for me.

# Checkout the Postgis 2.0.0 version (  Apr 3, 2012 )
# https://github.com/postgis/postgis/releases/tag/2.0.0
# https://github.com/postgis/postgis/commit/731e89549040556a4d693d3c05c1c8221c055157

export POSTGIS_GIT_HASH=731e89549040556a4d693d3c05c1c8221c055157
mkdir postgis_test_fetch_head
cd postgis_test_fetch_head
git init
git remote add origin https://git.osgeo.org/gitea/postgis/postgis.git
git fetch --depth 1 origin :${POSTGIS_GIT_HASH}
git reset --hard FETCH_HEAD
head  NEWS

# Classic
cd ..
git clone --branch master https://git.osgeo.org/gitea/postgis/postgis.git
cd postgis
git checkout ${POSTGIS_GIT_HASH}
head  NEWS

the first head NEWS contains the ( PostGIS 3.1.0alpha3 2020/xx/xx )

root@e9d0ed84e34f:/# export POSTGIS_GIT_HASH=731e89549040556a4d693d3c05c1c8221c055157
root@e9d0ed84e34f:/# mkdir postgis_test_fetch_head
root@e9d0ed84e34f:/# cd postgis_test_fetch_head
root@e9d0ed84e34f:/postgis_test_fetch_head# git init
Initialized empty Git repository in /postgis_test_fetch_head/.git/
root@e9d0ed84e34f:/postgis_test_fetch_head# git remote add origin https://git.osgeo.org/gitea/postgis/postgis.git
root@e9d0ed84e34f:/postgis_test_fetch_head# git fetch --depth 1 origin :${POSTGIS_GIT_HASH}
remote: Counting objects: 2290, done.
remote: Compressing objects: 100% (1957/1957), done.
remote: Total 2290 (delta 454), reused 1454 (delta 242)
Receiving objects: 100% (2290/2290), 12.81 MiB | 772.00 KiB/s, done.
Resolving deltas: 100% (454/454), done.
From https://git.osgeo.org/gitea/postgis/postgis
 * [new ref]                    -> 731e89549040556a4d693d3c05c1c8221c055157
root@e9d0ed84e34f:/postgis_test_fetch_head# git reset --hard FETCH_HEAD
HEAD is now at 9732cf8 Avoid encoding dependencies from subdirs
root@e9d0ed84e34f:/postgis_test_fetch_head# head  NEWS
PostGIS 3.1.0alpha3
2020/xx/xx
Only tickets not included in 3.1.0alpha2

* Breaking changes *
  - #4737, Bump minimum protobuf-c requirement to 1.1.0 (Raúl Marín)
           The configure step will now fail if the requirement isn't
           met or explicitly disabled (--without-protobuf)
  - #4258, Untangle postgis_sfcgal into its own .so (Regina Obe)

expected as in the second classic version

HEAD is now at 731e89549 Update versions for 2.0.0 tag
root@e9d0ed84e34f:/postgis# head  NEWS
PostGIS 2.0.0
2012/04/03
   
* Important / Breaking Changes  *

  - Upgrading to PostGIS 2.0 REQUIRES a dump and restore. See the
    "Hard Upgrade" section of the documentation.
  - Functions deprecated during the 1.X series (mostly non-ST variants)
    have been removed. Update your application to the new signatures.
    If you cannot update your application, see the legacy.sql file

@ImreSamu
Copy link
Member

ImreSamu commented Nov 4, 2020

@smellman

I think postgres:12 image was changed something.

If you modify for the "classic" git clone + git checkout it should work.

diff --git a/12-master/Dockerfile b/12-master/Dockerfile
index 024aa7e..d4cce58 100644
--- a/12-master/Dockerfile
+++ b/12-master/Dockerfile
@@ -65,39 +65,34 @@ RUN set -ex \
       protobuf-c-compiler \
       xsltproc \
     # sfcgal
-    && mkdir -p /usr/src/sfcgal \
-    && cd /usr/src/sfcgal \
-    && git init \
-    && git remote add origin https://gitlab.com/Oslandia/SFCGAL.git \
-    && git fetch --depth 1 origin :${SFCGAL_GIT_HASH} \
-    && git reset --hard FETCH_HEAD \
+    && mkdir -p /usr/src/ \
+    && cd /usr/src/ \
+    && git clone https://gitlab.com/Oslandia/SFCGAL.git \
+    && cd SFCGAL \
+    && git checkout ${SFCGAL_GIT_HASH} \
     && mkdir cmake-build \
     && cd cmake-build \
     && cmake .. \
     && make -j$(nproc) \
     && make install \
     && cd / \
-    && rm -fr /usr/src/sfcgal \
+    && rm -fr /usr/src/SFCGAL \
     # proj4
-    && mkdir -p /usr/src/proj \
-    && cd /usr/src/proj \
-    && git init \
-    && git remote add origin https://github.com/OSGeo/PROJ.git \
-    && git fetch --depth 1 origin :${PROJ_GIT_HASH} \
-    && git reset --hard FETCH_HEAD \
+    && cd /usr/src/ \
+    && git clone https://github.com/OSGeo/PROJ.git \
+    && cd PROJ \
+    && git checkout ${PROJ_GIT_HASH} \
     && ./autogen.sh \
     && ./configure --disable-static \
     && make -j$(nproc) \
     && make install \
     && cd / \
-    && rm -fr /usr/src/proj \
+    && rm -fr /usr/src/PROJ \
     # geos
-    && mkdir -p /usr/src/geos \
-    && cd /usr/src/geos \
-    && git init \
-    && git remote add origin https://github.com/libgeos/geos.git \
-    && git fetch --depth 1 origin :${GEOS_GIT_HASH} \
-    && git reset --hard FETCH_HEAD \
+    && cd /usr/src/ \
+    && git clone https://github.com/libgeos/geos.git \
+    && cd geos \
+    && git checkout ${GEOS_GIT_HASH} \
     && ./autogen.sh \
     && ./configure --disable-static \
     && make -j$(nproc) \
@@ -105,12 +100,10 @@ RUN set -ex \
     && cd / \
     && rm -fr /usr/src/geos \
     # gdal
-    && mkdir -p /usr/src/gdal \
-    && cd /usr/src/gdal \
-    && git init \
-    && git remote add origin https://github.com/OSGeo/gdal.git \
-    && git fetch --depth 1 origin :${GDAL_GIT_HASH} \
-    && git reset --hard FETCH_HEAD \
+    && cd /usr/src/ \
+    && git clone https://github.com/OSGeo/gdal.git \
+    && cd gdal \
+    && git checkout ${GDAL_GIT_HASH} \
     && cd gdal \
     && ./autogen.sh \
     && ./configure --disable-static \
@@ -119,12 +112,10 @@ RUN set -ex \
     && cd / \
     && rm -fr /usr/src/gdal \
     # postgis
-    && mkdir -p /usr/src/postgis \
-    && cd /usr/src/postgis \
-    && git init \
-    && git remote add origin https://git.osgeo.org/gitea/postgis/postgis.git \
-    && git fetch --depth 1 origin :${POSTGIS_GIT_HASH} \
-    && git reset --hard FETCH_HEAD \
+    && cd /usr/src/ \
+    && git clone https://git.osgeo.org/gitea/postgis/postgis.git \
+    && cd postgis \
+    && git checkout ${POSTGIS_GIT_HASH} \
     && ./autogen.sh \
 # configure options taken from:
 # https://anonscm.debian.org/cgit/pkg-grass/postgis.git/tree/debian/rules?h=jessie

@smellman
Copy link
Contributor Author

smellman commented Nov 4, 2020

@ImreSamu I tries it.
Also, I get current errors.

root@f8b2ad8223aa:/tmp/pgis_reg# cat test_103_diff
--- tickets_expected	2020-11-04 04:17:44.308731143 +0000
+++ /tmp/pgis_reg/test_103_out	2020-11-04 04:24:59.908413043 +0000
@@ -195,7 +195,7 @@
 #1543|MULTILINESTRING((0 0,10 0,10 10,0 0),(0 0))|POLYGON((0 0,10 10,10 0,0 0))
 #1578|f|f
 #1580.1|Point[S]
-ERROR:  transform: tolerance condition error (-20)
+#1580.2|0101000020430D000093107C45F81B7341B97937FF14E8AC41
 #1580.3|Point[S]
 #1596.1|public.road_pg.roads_geom SRID:3395 TYPE:POINT DIMS:2
 ERROR:  invalid SRID: 330000 not found in spatial_ref_sys
root@f8b2ad8223aa:/tmp/pgis_reg# cat test_120_diff
--- subdivide_expected	2020-11-04 04:17:44.241731143 +0000
+++ /tmp/pgis_reg/test_120_out	2020-11-04 04:25:45.722958155 +0000
@@ -4,7 +4,7 @@
 ERROR:  lwgeom_subdivide_prec: cannot subdivide to fewer than 5 vertices per output
 #3522|POINT(1 1)
 #3744|1600000000000000
-4|29321996468.6|29321996468.6|1857|256
+4|29321996468.6|29321996468.6|1856|256
 #4211|0.00008316000
 #4217|0.00002463668
 #4301|0.00213614552

@smellman smellman marked this pull request as ready for review November 4, 2020 11:29
@smellman
Copy link
Contributor Author

smellman commented Nov 4, 2020

@ImreSamu I test with old master branches of proj, geos and gdal.

PostGIS current master branch can build if use old branches of proj and geos.
I think pgis_reg/test_103 issue come from proj, and pgis_reg/test_120 issue come from geos.
Also building gdal current master branch may fail with proj and geos old branch.

@smellman
Copy link
Contributor Author

smellman commented Nov 5, 2020

GEOS issue may fixed: postgis/postgis@4ec4fbc

@smellman
Copy link
Contributor Author

smellman commented Nov 5, 2020

I write PROJ issue: https://trac.osgeo.org/postgis/ticket/4785

@ImreSamu
Copy link
Member

ImreSamu commented Nov 5, 2020

@smellman :

PostGIS current master branch can build if use old branches of proj and geos.

it is good news !

GEOS issue may fixed
I write PROJ issue:

and thanks for searching the root cause

My question:
As I understand the "original issue" was about creating a smaller docker image.
Do you have an early result about the new docker image size?

@smellman
Copy link
Contributor Author

smellman commented Nov 5, 2020

@ImreSamu

postgis13-master-test      latest              9b45e60ba355        18 hours ago        850MB
postgis/postgis            13-master           cbf3e2382b1f        7 days ago          854MB

It only 4MB smaller than current posgis-master.

But I think it is useful for debug to research root cause.
Now I find gdal build fail with old proj.

@smellman
Copy link
Contributor Author

smellman commented Nov 5, 2020

GEOS issue may fixed: postgis/postgis@4ec4fbc

I checked postgis current master was fixed this.
So, when it fixed proj issue, I will try build again.

@smellman
Copy link
Contributor Author

smellman commented Nov 6, 2020

@ImreSamu Proj issue was fixed postgis/postgis@5b2e3cb
I can build both 12-master and 13-master now.

Copy link
Member

@ImreSamu ImreSamu left a comment

Choose a reason for hiding this comment

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

@smellman: Thank you !

I have some "not critical" comments ..

sqlite3 \
&& rm -rf /var/lib/apt/lists/*

WORKDIR /usr/local
Copy link
Member

Choose a reason for hiding this comment

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

Question: This WORKDIR is important for the COPY ?

Dockerfile.master.template Show resolved Hide resolved
@@ -1,18 +1,10 @@
FROM postgres:%%PG_MAJOR%%
FROM postgres:%%PG_MAJOR%% as builder
Copy link
Member

Choose a reason for hiding this comment

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

imho: Please add some Warning comment to the top of the master Dockerfile;
At least an "experimental" warning ..

~ like

# "experimental" ;  only for testing!
# multi-stage dockerfile;  minimal docker version >= 17.05

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you comments.
I resolve all comments.

@ImreSamu
Copy link
Member

@smellman :
Thank you! 👍
All my critical and not critical comments has been resolved!
I have re-tested locally ( 12-master , 13-master ) and looks OK!

@phillipross :

  • Now It is your turn, Please review ...

@phillipross
Copy link
Contributor

@smellman @ImreSamu Thanks for your work on this! I'll take a look.

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.

None yet

3 participants