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

Use workspace modules with BP_GO_WORK_USE #537

Conversation

mamachanko
Copy link
Contributor

@mamachanko mamachanko commented Dec 15, 2023

Summary

When a submodule uses replace directives referring to modules within the same repository using a relative path, e.g. ../../, then the submodule cannot be built with paketo-buildpacks/go-build.

With Go's workspaces it's possible to include a submodule in the set of main modules so such a submodule can be built.

By allowing users to set BP_GO_WORK_USE the buildpack will first run go work init followed by go work use <modules...>, before building with go build <...>.

If this PR would be considered and ever get merged, I'd be happy to follow-up with a companion docs PR for https://paketo.io/docs/howto/go/.

Use Cases

I want to build cert-manager’s images w/ buildpacks. Unfortunately, but to the best of my knowledge, as of >=1.12 vanilla checkouts (e.g. [email protected]) cannot be built with paketo-buildpacks/[email protected] for their use of relative replace directives. What I've tried:

  • path=./cmd/controller doesn’t work b/c then it can’t see ../../
  • BP_GO_TARGET=./cmd/controller doesn’t work because it’s not contained by the main module
  • BP_GO_BUILD_FLAGS="-C ./cmd/controller" doesn’t work b/c of a limitation (or by design) in paketo-buildpacks/go-build. (it doesn’t put -C first, which go build expects. fixing it probably doesn’t help, b/c the buildpack makes assumptions about the working dir. it’s complicated)

(There’s a chance my research wasn’t exhaustive. Can you think of a way to build images off of vanilla cert-manager?)

However, with Go workspaces a vanilla check-out of cert-manager can be built. This is even documented by cert-manager.io/building/#go-workspaces. When the workspace use./cmd/controller it’s part of the main module and the buildpack succeeds. However, go.work is not being checked in by upstream. Afaik it's not common to check in go.work* either. As a workaround I could augment every version of upstream I want to build with go.work, but that’s not ideal. I'd rather use build a vanilla checkout with buildpacks.

After building this branch and building paketo-buildpacks/go with it, cert-manager's images built.

pack build \
  cert-manager_controller:0.0.0-mamachanko \
  --builder paketobuildpacks/builder-jammy-tiny \
  --buildpack /Users/*****/workspace/go/build/buildpackage.cnb \
  --env BP_GO_WORK_USE="./cmd/controller" \
  --env BP_GO_TARGETS="./cmd/controller"

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@mamachanko mamachanko force-pushed the topic/mamachanko/main/go-work-use branch from 7adf77c to 93ba5fb Compare December 15, 2023 10:44
@mamachanko mamachanko marked this pull request as ready for review December 15, 2023 11:02
@mamachanko mamachanko requested a review from a team as a code owner December 15, 2023 11:02
@mamachanko
Copy link
Contributor Author

@robdimsdale @ForestEckhardt Unfortunately, I couldn't figure out how to run integration tests locally. Reverse engineering the .github/workflows/test-pull-request.yml wasn't successful either since I couldn't figure out which --builder to pass to ./scripts/integration.sh. I was hoping this PR would do that for me, but a semver label is missing.

Copy link
Contributor

@ForestEckhardt ForestEckhardt left a comment

Choose a reason for hiding this comment

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

Hey there! This PR look great. My only call out is that I would like to see some unit test for the new functionality that you added. I would be more than happy to consult with you on adding those or back filling them and letting you take a look. Just let me know!

@mamachanko
Copy link
Contributor Author

My only call out is that I would like to see some unit test for the new functionality that you added.

I shall happily add unit tests!

Until then, would you be able to instruct me on how to run the integration locally?

" Running 'go work init'",
" Running 'go work use ./some/module1 ./some/module2'",
fmt.Sprintf(` Running 'go build -o %s -buildmode pie -trimpath .'`, filepath.Join(layerPath, "bin")),
" Completed in 0s",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 It's consistently expecting 0s while other tests want 1s. I can't quite tell why.

@ForestEckhardt
Copy link
Contributor

It looks like all of the integration tests are failing at the moment. Could you take a look in that?

@mamachanko mamachanko force-pushed the topic/mamachanko/main/go-work-use branch from 57f4f6b to 199a46e Compare February 2, 2024 13:36
@mamachanko
Copy link
Contributor Author

It looks like all of the integration tests are failing at the moment. Could you take a look in that?

Done. Finally I figured out how to run the integration tests for a builder locally with:

./scripts/integration.sh --builder paketobuildpacks/builder-jammy-buildpackless-tiny --token "$GIT_TOKEN"

I understand that it runs across a matrix of --builder on CI, but I hope that tiny was a good-enough approximation locally.

Also, thanks to ForestEckhardt/freezer#10 I was able to figure out how to rescue myself from "failed to open cacheManager: EOF" with rm -rf $HOME/.freezer-cache.

All that being said,

./scripts/unit.sh
./scripts/integration.sh --builder paketobuildpacks/builder-jammy-buildpackless-tiny --token "$GIT_TOKEN"

passes for me.

@ForestEckhardt ForestEckhardt added the semver:minor A change requiring a minor version bump label Feb 6, 2024
@ForestEckhardt
Copy link
Contributor

I think that this look great!

@ForestEckhardt ForestEckhardt merged commit 2f087af into paketo-buildpacks:main Feb 6, 2024
12 of 13 checks passed
@mamachanko mamachanko deleted the topic/mamachanko/main/go-work-use branch February 7, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants