Skip to content

optimize make#120

Closed
daixiang0 wants to merge 3 commits into
envoyproxy:mainfrom
daixiang0:make
Closed

optimize make#120
daixiang0 wants to merge 3 commits into
envoyproxy:mainfrom
daixiang0:make

Conversation

@daixiang0
Copy link
Copy Markdown
Member

@daixiang0 daixiang0 commented Jun 28, 2022

Optimize make:

  • unite binary build
  • add clean target
  • update test target to sync with CI
  • use lint target in CI to ensure sync

Signed-off-by: Loong Dai loong.dai@intel.com

@daixiang0 daixiang0 requested a review from a team as a code owner June 28, 2022 01:34
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 28, 2022

Codecov Report

Merging #120 (251e568) into main (884481c) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #120   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            5         5           
=========================================
  Hits             5         5           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 884481c...251e568. Read the comment docs.

@daixiang0 daixiang0 force-pushed the make branch 2 times, most recently from 50b0d96 to bbbde8b Compare June 28, 2022 02:21
Comment thread Makefile.targets.mk Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this breaks darwin binaries

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Now make build replaces make build-all as build-all only support Linux, I will update for darwin,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you might end up building something similar to what @Xunzhuo already did in #113, lets wait for @Xunzhuo to wrap up #113 and then see if any more optimizations are needed.
thanks !

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Now:

root:[gateway]$ MAKE_IN_DOCKER=0 make build -n
make -f Makefile.targets.mk build
make[1]: Entering directory '/root/github/gateway'
CGO_ENABLED=0 GOOS="linux" GOARCH="amd64" go build -a -o ./bin/linux/amd64/ github.com/envoyproxy/gateway/cmd/envoy-gateway
CGO_ENABLED=0 GOOS="linux" GOARCH="arm64" go build -a -o ./bin/linux/amd64/ github.com/envoyproxy/gateway/cmd/envoy-gateway
CGO_ENABLED=0 GOOS="darwin" GOARCH="amd64" go build -a -o ./bin/linux/amd64/ github.com/envoyproxy/gateway/cmd/envoy-gateway
CGO_ENABLED=0 GOOS="darwin" GOARCH="arm64" go build -a -o ./bin/linux/amd64/ github.com/envoyproxy/gateway/cmd/envoy-gateway
make[1]: Leaving directory '/root/github/gateway'

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you might end up building something similar to what @Xunzhuo already did in #113, lets wait for @Xunzhuo to wrap up #113 and then see if any more optimizations are needed. thanks !

Sure, I can wait.

Copy link
Copy Markdown
Member Author

@daixiang0 daixiang0 Jun 30, 2022

Choose a reason for hiding this comment

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

@arkodg seems my implementation looks easier to maintain, please take a look, feel free to pick one.

Signed-off-by: Loong Dai <loong.dai@intel.com>
Signed-off-by: Loong Dai <loong.dai@intel.com>
Signed-off-by: Loong Dai <loong.dai@intel.com>
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.

3 participants