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

DOCKER_BUILDKIT=1 is not passed when using docker-compose #2818

Merged
merged 7 commits into from
Sep 30, 2022

Conversation

punishell
Copy link
Contributor

DOCKER_BUILDKIT=1 is not passed when using docker-compose it needs to be changed to DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1 in order to make e.g:
make localnet-build works

Closes: #XXX

What is the purpose of the change

changes are needed to make commands works e.g: make localnet-build works

Brief Changelog

(for example:)

  • passing DOCKER_BUILDKIT=1 to docker-compose

Testing and Verifying

(Please pick one of the following options)

This change is already covered by existing tests.

This change added tests and can be verified as follows:

(example:)

  • COMPOSE_DOCKER_CLI_BUILD=1 to Makefile

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? ( no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? ( no)
  • How is the feature or change documented? (not applicable )

DOCKER_BUILDKIT=1 is not passed when using docker-compose it needs to be changed to DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1 in order to make
e.g:
make localnet-build  works
@punishell punishell requested a review from a team September 22, 2022 11:37
@czarcas7ic
Copy link
Member

It seems like make localnet-build and other localnet commands works for me. Tagging @niccoloraspa in case I'm missing something

@niccoloraspa
Copy link
Member

I think it was addressed in #2795

@punishell
Copy link
Contributor Author

Here is a output:

michal@ubuntu:~/osmosis$ sudo make localnet-build
[sudo] password for michal: 
Building osmosisd
Step 1/19 : ARG GO_VERSION="1.18"
Step 2/19 : ARG RUNNER_IMAGE="gcr.io/distroless/static"
Step 3/19 : FROM golang:${GO_VERSION}-alpine as builder
 ---> b68eed002951
Step 4/19 : RUN set -eux; apk add --no-cache ca-certificates build-base; apk add git linux-headers
 ---> Using cache
 ---> 2b27f7b9b243
Step 5/19 : WORKDIR /osmosis
 ---> Using cache
 ---> 0be37bc9bb61
Step 6/19 : COPY go.* ./
 ---> Using cache
 ---> b4dcd46360dd
Step 7/19 : RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/root/go/pkg/mod     go mod download
ERROR: Service 'osmosisd' failed to build: the --mount option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled
make: *** [Makefile:364: localnet-build] Error 1

Makefile Outdated

docker-build-debug:
@DOCKER_BUILDKIT=1 docker build -t osmosis:${COMMIT} --build-arg BASE_IMG_TAG=debug -f Dockerfile .
@DOCKER_BUILDKIT=1 docker tag osmosis:${COMMIT} osmosis:debug
NFTvuln1155 docker build -t osmosis:${COMMIT} --build-arg BASE_IMG_TAG=debug -f Dockerfile .
Copy link
Member

Choose a reason for hiding this comment

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

... what?

@ValarDragon
Copy link
Member

Does seem like COMPOSE_DOCKER_CLI_BUILD is a common thing env var to set: https://stackoverflow.com/questions/58592259/how-do-you-enable-buildkit-with-docker-compose

But can you explain the nftvuln line / why you included it?

changed only 
from 
@DOCKER_BUILDKIT=1
to
@DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1
@punishell
Copy link
Contributor Author

punishell commented Sep 27, 2022

sorry for that but i have been fixing it in GitHub web :D now should be ok

regarding

But can you explain the nftvuln line / why you included it?

looks like i have been doing two things at the same time and copy paste some nonsense sorry

fix spaces
Makefile Outdated
Comment on lines 378 to 391
localnet-build-state-export:
@DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1 docker-compose -f tests/localosmosis/state_export/docker-compose.yml build

localnet-start-state-export:
@docker-compose -f tests/localosmosis/state_export/docker-compose.yml up

localnet-startd-state-export:
@docker-compose -f tests/localosmosis/state_export/docker-compose.yml up -d

localnet-stop-state-export:
@docker-compose -f tests/localosmosis/docker-compose.yml down

localnet-remove-state-export:
@docker-compose -f tests/localosmosis/mainnet_state/docker-compose-state-export.yml down
rm -rf $(PWD)/tests/localosmosis/state_export/.osmosisd
Copy link
Member

Choose a reason for hiding this comment

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

cc @niccoloraspa @czarcas7ic for these, as they are new or different than the old ones.

Everything else LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure sounds legit

@niccoloraspa
Copy link
Member

niccoloraspa commented Sep 27, 2022

I think there are too many changes.

I'm happy to add COMPOSE_DOCKER_CLI_BUILD=1 to any Makefile target that is using docker-compose build.
It doesn't make sense to add it to docker build ... commands, though.

For example:

docker-build-debug:
	@DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1 docker build -t osmosis:${COMMIT} --build-arg BASE_IMG_TAG=debug -f Dockerfile .

This doesn't make sense. Same for all other targets docker-build-e2e-init-chain, docker-build-e2e-init-node, etc...

To avoid any merge conflicts maybe it's best to fetch main and reapply the relevant changes.

sorry for wasting your time :D
Makefile Outdated Show resolved Hide resolved
@niccoloraspa
Copy link
Member

niccoloraspa commented Sep 28, 2022

Hey @punishell,

thanks for the quick updates.
There are some fixes to do before merging:

  • COMPOSE_DOCKER_CLI_BUILD=1 must be present only on a docker-compose build command not on the others (up, upd, down). See comments below

  • There are some extra spaces added to be removed (See comments below)

Also, there are conflicts to be solved with the current Makefile, could you fix those?

After that, we can merge.

P.S. I saw the comment on the commit about wasting our time . Please don't feel this way, we are grateful for your contributions, and reviewing work is expected.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@niccoloraspa niccoloraspa added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v12.x backport patches to v12.x branch labels Sep 30, 2022
@niccoloraspa niccoloraspa merged commit 3c7eab9 into osmosis-labs:main Sep 30, 2022
mergify bot pushed a commit that referenced this pull request Sep 30, 2022
* DOCKER_BUILDKIT=1 is not passed when using docker-compose

DOCKER_BUILDKIT=1 is not passed when using docker-compose it needs to be changed to DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1 in order to make
e.g:
make localnet-build  works

* Update Makefile

changed only
from
@DOCKER_BUILDKIT=1
to
@DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1

* Update Makefile

fix spaces

* Update Makefile

sorry for wasting your time :D

* Update Makefile

* Update Makefile

Co-authored-by: Niccolo Raspa <[email protected]>
(cherry picked from commit 3c7eab9)
niccoloraspa pushed a commit that referenced this pull request Sep 30, 2022
)

* DOCKER_BUILDKIT=1 is not passed when using docker-compose

DOCKER_BUILDKIT=1 is not passed when using docker-compose it needs to be changed to DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1 in order to make
e.g:
make localnet-build  works

* Update Makefile

changed only
from
@DOCKER_BUILDKIT=1
to
@DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1

* Update Makefile

fix spaces

* Update Makefile

sorry for wasting your time :D

* Update Makefile

* Update Makefile

Co-authored-by: Niccolo Raspa <[email protected]>
(cherry picked from commit 3c7eab9)

Co-authored-by: punishell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v12.x backport patches to v12.x branch T:build V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants