Skip to content

Conversation

@friism
Copy link
Contributor

@friism friism commented May 14, 2017

- What I did

make build doesn't work, so I fixed it. There were two problems:

- How to verify it

make build

friism added 2 commits May 13, 2017 21:41
Signed-off-by: Michael Friis <friism@gmail.com>
Signed-off-by: Michael Friis <friism@gmail.com>
@dnephin
Copy link
Contributor

dnephin commented May 14, 2017

I believe this is also fixed by #78 which includes some other stuff for cross.

. ./scripts/build/ldflags

go build -o ./build/docker --ldflags "${LDFLAGS}" github.com/docker/cli/cmd/docker
CGO_ENABLED=0 go build -o ./build/docker --ldflags "${LDFLAGS}" github.com/docker/cli/cmd/docker
Copy link
Contributor

Choose a reason for hiding this comment

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

I think CGO_ENABLED is already set in the Dockerfiles, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but these scripts are invoked from make outside the Dockerfiles too so builds not using Docker won't work without this setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, this is also fixed in #78

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool @dnephin do you want me to remove that change from this pr?

@friism
Copy link
Contributor Author

friism commented May 14, 2017

I believe this is also fixed by #78 which includes some other stuff for cross.

@dnephin that PR adds bash as a requirement which means the build won't work in builds in standard alpine-based containers. Using bash in the shebang was also my first impulse, but I found that caused the Docker-based build to fail.

@dnephin
Copy link
Contributor

dnephin commented May 14, 2017

Sure, it's also going to fail in any container that doesn't have go, or sh. Why would it run in any container but the one we've defined in this repo? My change adds bash to the Dockerfile.

@friism
Copy link
Contributor Author

friism commented May 14, 2017

It would run outside of a container (in my case on Ubuntu). That's the primary documented way to build the project: https://github.com/docker/cli#build-locally

Solving the problem by removing dependencies and complexity rather than piling more on top seems preferable.

@dnephin
Copy link
Contributor

dnephin commented May 14, 2017

bash exists on ubuntu, so why would it fail when using bash?

We should also fix those instructions. I don't think running outside a container should be recommended.

@friism
Copy link
Contributor Author

friism commented May 14, 2017

@dnephin it wouldn't fail on Ubuntu when using bash, but you didn't answer my question: Why solve the problem by adding more complexity rather than removing complexity?

@dnephin
Copy link
Contributor

dnephin commented May 14, 2017

You didn't answer my question either! You said it would fail on alpine images, and I was asking why we would support arbitrary images!

I agree we should keep dependencies to a minimum. I don't think bash is an unreasonable dependency. We need to use bash to support -o pipefail anyway, so it's not an optional dependency.

I don't see either option are more complex. Where is the additional complexity?

@friism friism closed this May 15, 2017
nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
cherry-pick update rpm changelog
nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
Add dev portion to RPM package version
Upstream-commit: 208ba254d788a0d7bfe4dde92b35b9f2f68aa1d6
Component: packaging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants