Skip to content

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jul 1, 2019

Simplify and fix rpm/Makefile and deb/Makefile

  1. A symbol % is not working as expected in PHONY targets, so
    e.g. fedora-30 or ubuntu-cosmic were not being rebuild each time,
    because fedora-30 and ubuntu-cosmic directories exists.
    The fix is to explicitly mark every distro target as phony.
    This also has the nice side effect of bash completion
    working as it should!

  2. Remove code duplication for making packages for different distros.

  3. Add missing ubuntu releases (cosmic and disco).

Makefile: rely on targets in deb/rpm

Instead of dynamically getting list of distros to build for,
rely on the corresponding targets in sub-Makefiles. This also
ensures that deb/Makefile and rpm/Makefile will have up-to-date
list of distros included.

This also fixes the following bug:

$ make deb
for p in raspbian-stretch ubuntu-bionic ubuntu-disco ubuntu-xenial debbuild/ubuntu-disco ubuntu-cosmic debian-buster debian-stretch; do
...

As you can see, debbuild/ubuntu-disco should not be included but it
is. Could be prevented by using -maxdepth 1 argument to find.

While at it, amend the sub-Makefiles to print out the distro
that we build for.

@thaJeztah
Copy link
Member

looks like this needs a rebase

@thaJeztah
Copy link
Member

Guess we'll have to backport this to the release branches (once merged)

Copy link
Contributor

@seemethere seemethere left a comment

Choose a reason for hiding this comment

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

If we're going to go this route we'll also need to make changes to the Jenkinsfile since it attempts to use the old DOCKER_BUILD_PKGS make variable:

sh('make VERSION=0.0.1-dev DOCKER_BUILD_PKGS=ubuntu-xenial ENGINE_DIR=$(pwd)/engine CLI_DIR=$(pwd)/cli deb')

Also for the deb Makefile maybe it'd be better to leave out raspbian by default since it can only be built on arm based machines

kolyshkin added 2 commits July 2, 2019 14:43
Instead of relying on the main Makefile to pass DOCKER_BUILD_PKGS as
an argument to {rpm,deb}/Makefile, use the sub-makefile directly.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. A symbol % is not working as expected in PHONY targets, so
e.g. ubuntu-cosmic was not being rebuild each time, because
ubuntu-cosmic directory exists. The fix is to explicitly mark
every ubuntu-whatever target as phony.

2. Remove code duplication for making packages for different distros.

3. Add missing ubuntu (cosmic, disco) and debian (buster) to the
appropriate targets.

4. As a side effect, bash completion now lists all the distros
to be build.

5. Exclude raspbian from deb target as it can only be built on ARM.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

  • rebased to HEAD (added raspbian-buster)
  • fixed to not build raspbian by default from the main Makefile
  • fixed Jenkinsfile to work with updated Makefiles

@kolyshkin
Copy link
Contributor Author

Can't figure out the CI failure, what do I need to look for?

@thaJeztah
Copy link
Member

@kolyshkin looks like this is the failure; https://jenkins.dockerproject.org/job/docker/job/docker-ce-packaging/job/PR-357/3/execution/node/160/log/

21:45:12 Makefile:76: *** missing separator (did you mean TAB instead of 8 spaces?).  Stop.

kolyshkin added 2 commits July 2, 2019 15:43
1. A symbol % is not working as expected in PHONY targets, so
e.g. fedora-30 was not being rebuild each time, because
fedora-30 directory exists. The fix is to explicitly mark
every fedora-NN target as phony.

2. Remove code duplication for making packages for different distros.

3. As a side effect, bash completion now lists all the distros
to be build.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of dynamically getting list of distros to build for,
rely on the corresponding targets in sub-Makefiles. This also
ensures that deb/Makefile and rpm/Makefile will have up-to-date
list of distros included.

This also fixes the following bug:

> $ make deb
> for p in raspbian-stretch ubuntu-bionic ubuntu-disco ubuntu-xenial debbuild/ubuntu-disco ubuntu-cosmic debian-buster debian-stretch; do \
> ...

As you can see, `debbuild/ubuntu-disco` should not be included but it
is. Could be prevented by using `-maxdepth 1` argument to `find`.

While at it, amend the sub-Makefiles to print out the distro
that we build for.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

ah, stupid copy-paste error ... my vim used to mark those but it isn't now:(

00:00:48.834 Makefile:76: *** missing separator (did you mean TAB instead of 8 spaces?). Stop.

Fixed, pushed

@kolyshkin kolyshkin requested a review from seemethere July 3, 2019 00:25
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.

3 participants