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

Makefile: add a cross target 🌵 #2903

Merged
merged 1 commit into from
Jul 24, 2020

Conversation

vdemeester
Copy link
Member

@vdemeester vdemeester commented Jul 6, 2020

Changes

This will allow building for different target (x86_64, arm, arm64,
s390x and ppc64le).

This doesn't mean we support those architecture (especially as ko
doesn't really support multi-arch for now). But it is nice to be able
to see if our code compiles and can be package (manually,
unofficially, for those target). A check will be added in plumbing
so that we validate that, at least, a PR doesn't break those.

Signed-off-by: Vincent Demeester [email protected]

/cc @snehlatamohite

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Add a Makefile target to compile the code on multiple GOOS/GOARCH (not officially supported "yet")

This will allow building for different target (x86_64, arm, arm64,
s390x and ppc64le).

This doesn't mean we support those architecture (especially as `ko`
doesn't really support multi-arch for now). But it is nice to be able
to see if our code compiles and can be package (manually,
*unofficially*, for those target). A check will be added in plumbing
so that we validate that, at least, a PR doesn't break those.

Signed-off-by: Vincent Demeester <[email protected]>
@tekton-robot
Copy link
Collaborator

@vdemeester: GitHub didn't allow me to request PR reviews from the following users: snehlatamohite.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Changes

This will allow building for different target (x86_64, arm, arm64,
s390x and ppc64le).

This doesn't mean we support those architecture (especially as ko
doesn't really support multi-arch for now). But it is nice to be able
to see if our code compiles and can be package (manually,
unofficially, for those target). A check will be added in plumbing
so that we validate that, at least, a PR doesn't break those.

Signed-off-by: Vincent Demeester [email protected]

/cc @snehlatamohite

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Add a Makefile target to compile the code on multiple GOOS/GOARCH (not officially supported "yet")

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 6, 2020
@tekton-robot
Copy link
Collaborator

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

2 similar comments
@tekton-robot
Copy link
Collaborator

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

@tekton-robot
Copy link
Collaborator

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

@vdemeester
Copy link
Member Author

/kind misc
/cc @afrittoli @bobcatfish

@tekton-robot tekton-robot added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Jul 6, 2020
@bobcatfish
Copy link
Collaborator

I'm a bit confused about where we'd draw the line between the makefile and Task with params.

I'd imagine we'd have a build Task that takes params that prompt it to build differently, vs. a Task that calls make, what are your thoughts?

@vdemeester
Copy link
Member Author

I'd imagine we'd have a build Task that takes params that prompt it to build differently, vs. a Task that calls make, what are your thoughts?

@bobcatfish by Task I guess you mean a Tekton Task right ? The Task exists already, it is the golang-build one 😉
This PR is just about adding a way to quickly compile for multiple targets and have a check for it (tektoncd/plumbing#461). We don't use Tekton for our CI still (although that could be the first check to enable with tekton 🙃 )

I'm a bit confused about where we'd draw the line between the makefile and Task with params.

Makefile is local, Task is not. As a user I want to be able to cross compile on one of those targets, I can't do that with Task (yet or ever 😝), but I can with the make.

@bobcatfish
Copy link
Collaborator

@bobcatfish by Task I guess you mean a Tekton Task right ?

yep that's right!

We don't use Tekton for our CI still (although that could be the first check to enable with tekton 🙃 )

@vdemeester how would you see this working once we DO use Tekton for our CI here, would we have a Task that calls the Makefile, or would we duplicate the logic between using the golang-build Task and the Makefile? (or something else?)

@vdemeester
Copy link
Member Author

@vdemeester how would you see this working once we DO use Tekton for our CI here, would we have a Task that calls the Makefile, or would we duplicate the logic between using the golang-build Task and the Makefile? (or something else?)

Whatever works. Overall, I see the Makefile as the inner loop (aka quick feedback, no cluster, local dev) and the tasks/pipeline as outer loop. At some point in the future, if we can have local executors that can read pipeline specs, we could share as much as possible. Before, the decision depends on what feels the best. If we want to ensure that what the CI powered by tekton is the closest to what the user do on its machine, then we need to define Tasks that would use make (if we want to allow the user to easily run cross compile, etc…). I tend to lead towards that (that would also apply to prow tbh), as the less we have "prow" specific, the better 🙃.

@bobcatfish
Copy link
Collaborator

It sounds like it depends on whether or not it's possible to run Tasks locally - if you could easily run Tasks locally, I think you'd be into using Tasks all the way instead of a Makefile, is that right?

I really do think it's important that we find a way to make Tasks run locally ( #235 ), otherwise folks will experience frustrating with invoking different CI when they are developing vs. when they submit (or maybe I should re-state as: we have an opportunity to really improve this experience by making these two workflows the same! :D)

@vdemeester
Copy link
Member Author

It sounds like it depends on whether or not it's possible to run Tasks locally - if you could easily run Tasks locally, I think you'd be into using Tasks all the way instead of a Makefile, is that right?

It depends too 😝. How much do I need to write for using Tasks vs Makefile ? What do I prefer to do as my developer inner-loop workflow, … ? How much do I buy into the outer loop — aka do I want the CI to drive my local dev experience (or the opposite) ?

I really do think it's important that we find a way to make Tasks run locally ( #235 ), otherwise folks will experience frustrating with invoking different CI when they are developing vs. when they submit (or maybe I should re-state as: we have an opportunity to really improve this experience by making these two workflows the same! :D)

I see value on adding a way to run Tasks locally for sure but I don't see it as extremely important (at least for now, in the current state of tekton components). Mainly because fixing the "frustration with invoking different CI when they are developing vs. when they submit" is something that can be fixed outside of tekton scope — it's an organizational problem, you can make Makefile pretty portable, you can use the same tools for inner and outer loop, and thus make the differences between them really small. Adding adoption to existing tools on the inner loop (make, …) and how well and optimized a local runner of tasks/pipeline would need to be… I am definitely not saying we shouldn't do it, I see value to it, and this is something I want to do since working on docker/docker but it's not the primary value tekton should bring.

@bobcatfish
Copy link
Collaborator

Mainly because fixing the "frustration with invoking different CI when they are developing vs. when they submit" is something that can be fixed outside of tekton scope — it's an organizational problem, you can make Makefile pretty portable, you can use the same tools for inner and outer loop, and thus make the differences between them really small.

I guess it depends on the unit that you are using for this kind of sharing organizationally: you could have everyone in your org using the same Makefile, or the same Task, or you could have them all using the same bash script.

I feel like one of the things Tekton is attempting to solve is providing the standardization mechanism and making that a Task. If you use Makefiles as your standardization mechanism, we'd really only need one Task: the makefile Task.

I also see a lot of parallels between Tasks + Pipelines and Makefiles, they feel to me like they are trying to solve a lot of the same problems, so I have trouble seeing the line between the two, similarly to the line between a Task that calls a bash script which takes a bunch of args to determine what to do. In both cases I'd like to see more of the logic moved up into the Tasks and Pipelines, and less happening in the things they invoke.

@vdemeester
Copy link
Member Author

I guess it depends on the unit that you are using for this kind of sharing organizationally: you could have everyone in your org using the same Makefile, or the same Task, or you could have them all using the same bash script.

By organizational problem I didn't mean it as at the level of an organization (a company, a project, …), but on the way you want to handle inner and outer loop ; and how developer organize themselves. For example, even if you provide a make test target (or the equivalent in a Task), a developer might want to run go test ./... on its own (go run -tags e2e -run '^TestFoo$', …)

I feel like one of the things Tekton is attempting to solve is providing the standardization mechanism and making that a Task. If you use Makefiles as your standardization mechanism, we'd really only need one Task: the makefile Task.

From the website :

Tekton is a powerful and flexible open-source framework for creating CI/CD systems, allowing developers to build, test, and deploy across cloud providers and on-premise systems.

From the README.md:

The Tekton Pipelines project provides k8s-style resources for declaring CI/CD-style pipelines.

It's not yet our "goal" to provide a standard mechanism for anything else than CI/CD systems (aka local dev, inner loop). It could be at some point, it's not the case now. Tekton bring the portability of containers in the CI/CD world on top of k8s in a nice and easy way for the user (at least that's the goal 👼). It doesn't

I also see a lot of parallels between Tasks + Pipelines and Makefiles, they feel to me like they are trying to solve a lot of the same problems, so I have trouble seeing the line between the two, similarly to the line between a Task that calls a bash script which takes a bunch of args to determine what to do. In both cases I'd like to see more of the logic moved up into the Tasks and Pipelines, and less happening in the things they invoke.

I don't see a lot of parallels between Tasks + Pipelines and Makefiles.

From the GNU Make website:

The make utility automatically determines which pieces of a large program need to be recompiled, and issues commands to recompile them.

  • make predates tekton a long shot 😝
  • make doesn't have the same "DAG" build that pipeline has — although it has an optimized way to run targets or not, and can run things in parallel.
  • If you run this Makefile in a container that doesn't have go installed, it will failed. I wrote the Makefile to be as "self-standing" as possible (especially on the linters and tools used) but it is definitely not a requirement for a Makefile. A go task that has make inside is far more useful than a make task that has only make in.
  • make is optimized for the inner loop, tektoncd/pipeline is optimized for the outer loop. They serves, imho, two different needs, and they definitely can be used together. But it's not a requirement (for them to be used together).

In a long-term ideal world, you would to a make test and it would look at the test task definition and run it locally (on whatever runtime can do it). make would be just a nice way to run whatever is capable of running the task locally, a wrapper to let's say tkn task run test --from . --local --runtime=podman (or whatever commands that would be).

@bobcatfish
Copy link
Collaborator

It sounds to me like it would be good to get more clear on this inner loop vs. outer loop distinction because that sounds quite different from how I look at things.

Ideally I'd like to be running the same thing as part of my development workflow as is used as part of the automated CI systems - this whole "shift left" thing is above getting signals earlier, and what better time to get signals than as you work. If you can run the same thing locally easily that you run remotely, then great!

Maybe we should create another issue and/or doc where we can talk about this distinction?

For example, even if you provide a make test target (or the equivalent in a Task), a developer might want to run go test ./... on its own (go run -tags e2e -run '^TestFoo$', …)

What I'd want here is a way to be able to go from a Task to the containers run inside the task to the commands run inside the container. I don't want to be doing something totally different from what the CI is doing, I want to know what the CI is doing and customize it.

I don't see a lot of parallels between Tasks + Pipelines and Makefiles.

What I mean is when people use Makefiles as the interface to building + testing their projects.

I think a lot of projects have Makefiles that provide interfaces like make test, make build, etc.. Some companies would use this as an interface across all their projects, e.g. "you can always run make test on a repo in our org"

The Makefile then is the interface to the CI behaviors (testing, linting, building) and is also the place where the logic that defines those behaviors is stored.

If Tekton Tasks are used as a wrapper around calling make files and scripts, I think we've lost a lot of the power.

@vdemeester
Copy link
Member Author

vdemeester commented Jul 8, 2020

Ideally I'd like to be running the same thing as part of my development workflow as is used as part of the automated CI systems - this whole "shift left" thing is above getting signals earlier, and what better time to get signals than as you work. If you can run the same thing locally easily that you run remotely, then great!

Maybe we should create another issue and/or doc where we can talk about this distinction?

Maybe (especially as we are discussing something far from what that PR does initially, which is add a few target to an existing Makefile.

For example, even if you provide a make test target (or the equivalent in a Task), a developer might want to run go test ./... on its own (go run -tags e2e -run '^TestFoo$', …)

What I'd want here is a way to be able to go from a Task to the containers run inside the task to the commands run inside the container. I don't want to be doing something totally different from what the CI is doing, I want to know what the CI is doing and customize it.

Sure that is the ideal world, the ultimate idea. We had some discussion at docker around this: docker task … would run a task definition in a container (as much optimized as we could). Task definition would be in a file and would be building a dag (aka dependency could be defined between task). Then you could have a docker pipeline component (in swarm, but impl. detail), that would read that same file and executing the whole dag, to be used in you CI.

That's a general ideal idea for sure, and it would be nice to have at some point but, there is also pragmatism and how user are doing things now and how we can help them. And also, again, what is the goal of tekton ? I don't think it is to be as involved as this in the dev workflow — or we need to update it.

I don't see a lot of parallels between Tasks + Pipelines and Makefiles.

What I mean is when people use Makefiles as the interface to building + testing their projects.

I think a lot of projects have Makefiles that provide interfaces like make test, make build, etc.. Some companies would use this as an interface across all their projects, e.g. "you can always run make test on a repo in our org"

The Makefile then is the interface to the CI behaviors (testing, linting, building) and is also the place where the logic that defines those behaviors is stored.

If Tekton Tasks are used as a wrapper around calling make files and scripts, I think we've lost a lot of the power.

How so ? What is "power" here even ? What is Tekton trying to solve ? How should it impact the way user and team develop locally and what tool they are using for this ?

In any case, does this (above) prevents from this PR to get reviewed and merged ? 😛 🙃

@bobcatfish
Copy link
Collaborator

Okay I've started tektoncd/community#145 to take the local development workflow discussion out of this PR

In any case, does this (above) prevents from this PR to get reviewed and merged ? 😛 🙃

Considering that we've already got the Makefile, I don't think any of my thoughts here would block this PR - I do wonder about the Makefile in general but not to block this PR.

Last question about the Makefile:

Overall, I see the Makefile as the inner loop (aka quick feedback, no cluster, local dev) and the tasks/pipeline as outer loop.

I think I'm a bit confused b/c this particular Makefile I think exists so Prow can call it, and not (if I understand) to facilitate the "inner loop" of local development. Would that mean this particular Makefile is a temporary measure until we have migrated from Prow to Tekton?

@vdemeester
Copy link
Member Author

I think I'm a bit confused b/c this particular Makefile I think exists so Prow can call it, and not (if I understand) to facilitate the "inner loop" of local development. Would that mean this particular Makefile is a temporary measure until we have migrated from Prow to Tekton?

Nope, prow doesn't use the Makefile at all yet (tektoncd/plumbing#461 would make it the first job using the makefile). Prow is still using plain old bash scripts (we do use more the cli Makefile in prow but even). The Makefile is there for inner loop mainly (and if we can use some of it in prow/tekton/whatever-ci, then that's good, but that's not its primary focus).

@bobcatfish
Copy link
Collaborator

Nope, prow doesn't use the Makefile at all yet (tektoncd/plumbing#461 would make it the first job using the makefile

Right but the intention is for prow to run it right? I'm still a bit confused.

The Makefile is there for inner loop mainly

Maybe we should mention it at https://github.com/tektoncd/pipeline/blob/master/DEVELOPMENT.md then?

@vdemeester
Copy link
Member Author

Right but the intention is for prow to run it right? I'm still a bit confused.

Nope 😛. The intention is really to simplify my (my being "me the contributor") life when in the inner loop. I can run whatever the Makefile does locally (go test, kube apply, golangci-lint, …), using my IDE features, the cli, … The Makefile is just grouping those and "documenting" them a bit so that as a contributor I don't need to remember all the small things to add/set.

The Makefile is there for inner loop mainly

Maybe we should mention it at https://github.com/tektoncd/pipeline/blob/master/DEVELOPMENT.md then?

Indeed, we should probably mention it there. I can prepare a PR for this 😉

@bobcatfish
Copy link
Collaborator

The intention is really to simplify my (my being "me the contributor") life when in the inner loop. I can run whatever the Makefile does locally (go test, kube apply, golangci-lint, …), using my IDE features, the cli, … The Makefile is just grouping those and "documenting" them a bit so that as a contributor I don't need to remember all the small things to add/set.

Ah kk I think I'm starting to understand - my preference would be that we do add any tools we're giving to maintainers to our automation. Otherwise we'll be in a situation where say maybe only you are ever running this Makefile, or maybe you stop one day, does it keep working over time, do we expect people to be running it - all of this is kind of ambiguous unless we're invoking the tool/Makefile in our automation.

Copy link
Member

@sthaha sthaha left a comment

Choose a reason for hiding this comment

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

/lgtm

I find it helpful to have a make cross target that verifies that code compiles on all supported platforms.

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2020
@afrittoli
Copy link
Member

Perhaps before merging this we should add some docs to clarify that:

  • we don't explicitly support the list of architectures in the makefile
  • the makefile is only provided as a convenience tool, but it's not officially supported as being part of the development workflow - unless we start testing it?

@vdemeester
Copy link
Member Author

Perhaps before merging this we should add some docs to clarify that:

* we don't explicitly support the list of architectures in the makefile

As we don't document the makefile anywhere, that's a given, but yeah we can document that (or document which architecture we support).

* the makefile is only provided as a convenience tool, but it's not officially supported as being part of the development workflow - unless we start testing it?

The makefile is merely a convenience setup for the make tool to do inner loop work yes. We need to define what we "official support as being part of the development workflow" and if we even need to do that 😝 (aka we can recommend to use make or some scripts, but in the end, we don't have to support anything on the developer dev machine, only on our CI (imho).

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

/lgtm

@afrittoli
Copy link
Member

/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2020
@tekton-robot tekton-robot merged commit abb677d into tektoncd:master Jul 24, 2020
@vdemeester vdemeester deleted the make-cross branch July 24, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants