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

Fix docker image builds with an actually-reliable dependency skip #5071

Merged
merged 3 commits into from
Feb 1, 2023

Conversation

Groxx
Copy link
Contributor

@Groxx Groxx commented Feb 1, 2023

Honestly this is so obvious in retrospect that I don't know why I tried to do the fake-codegen thing.
The easiest way to skip dependencies is... to not declare that dependency in the first place.


Our github actions to release a new docker image are failing with errors like

Step 14/58 : RUN CGO_ENABLED=0 make copyright cadence-cassandra-tool cadence-sql-tool cadence cadence-server cadence-bench cadence-canary
 ---> Running in 689359fdaa6b
.bin/copyright
building revive from internal/tools/go.mod...
go: downloading github.com/mgechev/revive v1.2.2-0.20220724073416-db56db0b6a11
go: downloading github.com/mitchellh/go-homedir v1.1.0
go: downloading github.com/BurntSushi/toml v1.2.0
go: downloading github.com/mattn/go-colorable v0.1.12
go: downloading github.com/mgechev/dots v0.0.0-20210922191527-e955255bf517
go: downloading github.com/chavacava/garif v0.0.0-20220630083739-93517212f375
go: downloading github.com/olekukonko/tablewriter v0.0.5
go: downloading golang.org/x/sys v0.0.0-2022061022[130](https://github.com/uber/cadence/actions/runs/4059406201/jobs/6987848251#step:4:131)4-9f5ed59c137d
go: downloading github.com/mattn/go-runewidth v0.0.9
building goimports from internal/tools/go.mod...
Makefile:252: *** idls/ submodule must exist, or build will fail.  Run `git submodule update --init` and try again.  Stop.
The command '/bin/sh -c CGO_ENABLED=0 make copyright cadence-cassandra-tool cadence-sql-tool cadence cadence-server cadence-bench cadence-canary' returned a non-zero code: 2

which comes from at least two things:

  1. make copyright [stuff that depends on copyright] doesn't make sense with Make. Order isn't guaranteed like that, and the newly-invalidated tree of dependencies may not be noticed. So that's bad.
  2. make .fake-codegen didn't work anyway, because Make noticed other dependencies of lint/codegen/etc were still missing, and it started building them.

The fix is pretty obvious - don't bother trying to touch all the dependencies, just remove the dependency.
This way it trivially does what we want: just calls go build a bunch, without making any changes to files.


When merged, I'll cherry-pick this over to the 0.25.x branch so we can do a release there.

Honestly this is so obvious in retrospect that I don't know why I tried to do the fake-codegen thing.
The easiest way to skip dependencies is... to not declare that dependency in the first place.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 01860aab-06b5-490b-a64a-04aec331e6d3

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 97 unchanged lines in 16 files lost coverage.
  • Overall coverage increased (+0.02%) to 57.237%

Files with Coverage Reduction New Missed Lines %
common/task/weightedRoundRobinTaskScheduler.go 1 89.64%
client/history/client.go 2 38.1%
client/history/metricClient.go 2 45.3%
common/peerprovider/ringpopprovider/config.go 2 81.58%
common/task/parallelTaskProcessor.go 2 92.75%
service/history/handler.go 2 47.15%
service/history/task/transfer_standby_task_executor.go 2 87.24%
common/persistence/dataManagerInterfaces.go 3 58.66%
service/frontend/workflowHandler.go 4 59.98%
service/history/task/transfer_active_task_executor.go 4 72.22%
Totals Coverage Status
Change from base Build 01860a83-5b45-41fc-855a-f94b59a57d2a: 0.02%
Covered Lines: 84983
Relevant Lines: 148476

💛 - Coveralls

@Groxx Groxx merged commit 361a107 into uber:master Feb 1, 2023
Groxx added a commit to Groxx/cadence that referenced this pull request Feb 1, 2023
Minor conflicts when cherry-picking from 361a107,
so I'm doing this as a new PR for completeness.  The end result is identical.
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