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

Do not force pkg build by default #2926

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

giggsoff
Copy link
Contributor

@giggsoff giggsoff commented Nov 15, 2022

Let's make FORCE_BUILD flag optional and rely on hash provided by linuxkit on changes (linuxkit/linuxkit#3875).
With no flag provided linuxkit will check that image published, if not, will build it.
This way we can avoid building of dependencies as well.

Few measurements on my PC (time make pkg/pillar) with changes from PR vs without:

  • after docker rm -f --volumes linuxkit-builder&&rm -rf ~/.linuxkit: 0m6,400s vs 9m15,422s
  • with change in go code (just one comment): 3m17,143s vs 2m10,048s
  • with another change in go code (to use cached layers of Dockerfile): 1m46,950s vs 2m10,048s
  • no changes in go code: 0m6,300s vs 0m44,811s

Also PR includes update of linuxkit version to speedup restart of buildkit container.

@deitch
Copy link
Contributor

deitch commented Nov 15, 2022

This speeds things up between ~50% and ~66%, which is really nice.

Let me see if I get this:

  • normal build, e.g. make pkg/strongswan or make eve-strongswan: pkg build --force
  • dependency build, e.g. make pkg/pillar or make eve-ensure-pillar: pkg build (without --force)

I have no objection to this. I do have a question, though. Why would we not remove --force entirely? That would simplify the targets and dependencies to what they are now, but improve the build time.

As it is, if you changed any files in a pkg/%, then lkt pkg build will see the changed tag and rebuild it. The only reason for --force is, "I do not care if it already exists and is unchanged, build it again", the equivalent of make -B. If the git tree in pkg/<whatever> is dirty - changed files, uncommitted, anything other than something explicitly in .gitignore, then it will do a rebuild anyways.

Can we just get rid of --force and thus simplify this PR?

Also PR includes linuxkit/linuxkit#3874 of linuxkit version to speedup restart of buildkit conatiner

Thank you for including that, and especially hats off for tracking it down.

@giggsoff
Copy link
Contributor Author

As it is, if you changed any files in a pkg/%, then lkt pkg build will see the changed tag and rebuild it.

Not sure that it cover all cases, as I mentioned before, it will not work for files not added to git, which is possible for local development:

$ ./build-tools/bin/linuxkit pkg build --platforms=linux/amd64 pkg/pillar
Building "lfedge/eve-pillar:7b27788d7bc6fa7b43482f4fcfcf77f8d18a7890"
checking for docker.io/lfedge/eve-pillar:7b27788d7bc6fa7b43482f4fcfcf77f8d18a7890 in local cache...
docker.io/lfedge/eve-pillar:7b27788d7bc6fa7b43482f4fcfcf77f8d18a7890 amd64 not found in local cache, checking registry
docker.io/lfedge/eve-pillar:7b27788d7bc6fa7b43482f4fcfcf77f8d18a7890 amd64 found on registry
Build complete, not pushing, all done.
$ touch pkg/pillar/rootfs/new-file-added
$ ./build-tools/bin/linuxkit pkg build --platforms=linux/amd64 pkg/pillar
Building "lfedge/eve-pillar:7b27788d7bc6fa7b43482f4fcfcf77f8d18a7890"
checking for docker.io/lfedge/eve-pillar:7b27788d7bc6fa7b43482f4fcfcf77f8d18a7890 in local cache...
docker.io/lfedge/eve-pillar:7b27788d7bc6fa7b43482f4fcfcf77f8d18a7890 amd64 not found in local cache, checking registry
docker.io/lfedge/eve-pillar:7b27788d7bc6fa7b43482f4fcfcf77f8d18a7890 amd64 found on registry
Build complete, not pushing, all done.

Agree, it will simplify the logic, but at least for me it is not clear why make pkg/pillar should ends in checking for external repositories even if current version is published, my expectation is that I will try to build.

@deitch
Copy link
Contributor

deitch commented Nov 15, 2022

Not sure that it cover all cases, as I mentioned before, it will not work for files not added to git, which is possible for local development

True. It captures:

  • changed files that are in git
  • changes that have been staged but not committed
  • committed changes

The only thing it misses is things that were added but not staged or committed, yet are targeted to git (i.e. not in .gitignore). We could improve that by changing the pkg show-tag command to catch those as well.

Either way, the majority of the cases are when a git-known file has changed, and the "I added a new file but didn't tell git about it" is by far the minority. I think that we are much better off having all cases run by default with no --force, and the few cases where someone needs to run with it, they can do make pillar FORCE=--force.

@eriknordmark
Copy link
Contributor

Either way, the majority of the cases are when a git-known file has changed, and the "I added a new file but didn't tell git about it" is by far the minority. I think that we are much better off having all cases run by default with no --force, and the few cases where someone needs to run with it, they can do make pillar FORCE=--force.

I would agree with @deitch.
I can't see a case where I would add a file (e.g., a pkg/pillar/foo/bar.go) without adding some code in some existing file which would call some function in the new file (and if I modify the existing file we'll do the rebuild.)
The only corner case is if I add a init() function in a new file in some existing package e.g., under pkg/pillar. That would not cause a rebuild and I'd be surprised that my new init function was never called. Thoughts?

@deitch
Copy link
Contributor

deitch commented Nov 20, 2022

FWIW, I have a PR open to linuxkit to capture not-yet-added-to-git files as well.

The only corner case is if I add a init() function in a new file in some existing package e.g., under pkg/pillar. That would not cause a rebuild and I'd be surprised that my new init function was never called. Thoughts?

Yeah, that would be a corner case. In that case, I actually am quite comfortable saying, "you need to use --force here". But in any case, the linuxkit change should capture that as well.

@giggsoff giggsoff changed the title Add eve-ensure-% target to not force build dependencies Do not force pkg build by default Nov 22, 2022
@giggsoff
Copy link
Contributor Author

Well, with linuxkit/linuxkit#3875 merged, I hope we will not hit such corner cases. I modified title, description and code.

@giggsoff
Copy link
Contributor Author

But as #2927 has higher priority and modify the version as well, I will mark the PR as draft.

@giggsoff giggsoff marked this pull request as draft November 22, 2022 08:54
@giggsoff giggsoff marked this pull request as ready for review November 26, 2022 12:25
Let's make FORCE_BUILD flag optional and rely on hash provided by
linuxkit on changes (linuxkit/linuxkit#3875).
With no flag provided linuxkit will check that
image published, if not, will build it.
This way we can avoid building of dependencies as well.

Signed-off-by: Petr Fedchenkov <giggsoff@gmail.com>
Copy link
Contributor

@deitch deitch left a comment

Choose a reason for hiding this comment

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

Thrilled to see this in.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

@eriknordmark eriknordmark merged commit 6c85ce0 into lf-edge:master Nov 29, 2022
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.

None yet

3 participants