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

Build uses OS and ARCH variables. #129

Closed
wants to merge 3 commits into from
Closed

Build uses OS and ARCH variables. #129

wants to merge 3 commits into from

Conversation

pastushenkoy
Copy link
Contributor

Please read CONTRIBUTING.md for details on our code of conduct, and the process for submitting pull requests to us.

Fixes #128

Proposed Changes

  • Adds local build description
  • Fixes makefile

Description

Checklist

  • Read the CONTRIBUTING document.
  • Read the CODE OF CONDUCT document.
  • Add tests to cover changes.
  • Ensure your code follows the code style of this project.
  • Ensure CI and all other PR checks are green OR
    • Code compiles correctly.
    • Created tests which fail without the change (if possible).
    • All new and existing tests passed.
  • Add your changes to Unreleased section of CHANGELOG.
  • Improve and update the README (if necessary).
  • Ensure documentation is up-to-date. The same file will be updated in plugin index when your PR is accepted, so it will be available for end-users at http://plugins.drone.io.

Copy link
Contributor

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This would help to contributors.
CI is failing though, could you have a look at it? Maybe you should consider using GOOS and GARCH and extract them using go tool rather than using the variables that we generated using uname

README.md Outdated
@@ -266,6 +266,11 @@ Targets:
help Shows this help message
```

For testing purposes you might want to build your container and push it into your own container registry. Use this command for it:
```
CONTAINER_REPO=<your-container-reginstry-address>/drone-cache OS=linux ARCH=amd64 make container-push
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CONTAINER_REPO=<your-container-reginstry-address>/drone-cache OS=linux ARCH=amd64 make container-push
CONTAINER_REPO=<your-container-registry-address>/drone-cache OS=linux ARCH=amd64 make container-push

@pastushenkoy
Copy link
Contributor Author

@kakkoyun I suggest using linux and 'amd64` as defaults. This is most popular combination, I think.
Nevertheless, I would like to keep OS and ARCH variables to separate build args from implementation details.

But when now liche is protesting. Can you explain how to fix this issue with docs generation to not bother you next time?

Copy link
Contributor

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Let's not hard code OS and ARCH values.

CI failing because documentation generation issues. Try make docs or make README.md and commit changes. This is not relevant to your changes, it'd be great if you can just check them in.

@@ -1,7 +1,7 @@
include .bingo/Variables.mk

OS ?= $(shell uname -s | tr '[A-Z]' '[a-z]')
ARCH ?= $(shell uname -m)
OS ?= linux
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we hard-coding these? Without these parameters go tool itself pretty clever to determine the os and architecture by given these default you introduce a regression for user. They need to specify those to build. This is not what we want.

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, for example, use macbook for development and if I build container without specifying OS, I get darwin. This leads to runtime error as my drone uses linux to execute pipelines. I really doubt that anyone would do it on macos, but for those who would, goreleaser will make correct images.
As for ARCH, uname returns x86_64 instead of amd64. It leads to error when building container.
As far as I understand, the build command is used only in drone pipeline to ensure that build is not broken and for local build and development. For these purposes these values can and should be hardcoded.

@kakkoyun
Copy link
Contributor

This is not a change that we want to accept. These command is to build container if it needs to be tested locally. One can easily override defaults for their target platform. I don't see any value in hardcoding these variables. Closing for now.

Thanks a lot for contribution in any case.

@kakkoyun kakkoyun closed this Jul 30, 2020
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.

Make local builds more transparent
2 participants