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.

Sign up for GitHub

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

Jump to bottom

Don't run clean-gen twice #214

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Don't run clean-gen twice #214

merged 2 commits into from
Nov 28, 2024

Conversation

dmathieu
Copy link
Member

@dmathieu dmathieu commented Nov 28, 2024 •
edited
Loading

It seems like clean-gen is being executed only once, even though Make asks to execute it twice.
That maybe an "optimization" on some make versions, where commands are expected to be idempotent?

@dmathieu dmathieu changed the title Setup specific make commands for clean-gen and go-mod-tidy Don't run clean-gen twice Nov 28, 2024
@dmathieu
Copy link
Member Author

From CI:

rm -rf gen
go mod tidy in .
go: warning: "all" matched no packages
go mod tidy in ./internal/tools
go mod tidy in ./otlp
go mod tidy in ./slim/otlp

So the gen folder is properly removed before running go-mod-tidy.
That also shows by the failure of make test. Since there's no gen folder anymore, I had to ignore the root package, since it doesn't include any Go files.

@dmathieu dmathieu marked this pull request as ready for review November 28, 2024 13:22
@dmathieu dmathieu requested a review from a team as a code owner November 28, 2024 13:22
florianl
florianl approved these changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@florianl florianl left a comment

Choose a reason for hiding this comment

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

This fixes the issues faced in #193.

@pellared
Copy link
Member

even though Make asks to execute it twice.

A target is always executed at most one time.

@dmathieu dmathieu merged commit c1b3008 into open-telemetry:main Nov 28, 2024
5 checks passed
@dmathieu dmathieu deleted the tidy branch November 28, 2024 14:05
pellared
pellared reviewed Nov 28, 2024
View reviewed changes
Makefile
Comment on lines +42 to +43
ALL_GO_SUB_MOD_DIRS := $(shell find . -type f -name 'go.mod' -mindepth 2 -exec dirname {} \; | sort)
OTEL_GO_MOD_DIRS := $(filter-out $(TOOLS_MOD_DIR), $(ALL_GO_SUB_MOD_DIRS))
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need clean-gen after these changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm yeah, we could indeed remove it and only run tidy in the submodules. I'm thinking it's too easy to get bitten by gen code that hasn't been cleaned though. So doing it is a good measure as well.

Copy link
Member

@pellared pellared Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to have gen (build artifacts) after running make to easier troubleshoot problems if something goes wrong. I created #215

@pellared pellared mentioned this pull request Nov 28, 2024
Closed
MikeGoldsmith added a commit to honeycombio/opentelemetry-proto-go that referenced this pull request Dec 12, 2024
@MikeGoldsmith
Merge tag 'v1.4.0' of github.com:open-telemetry/opentelemetry-proto-g…
7e3bdb1 
…o into mike/update-proto

Module set all, Version v1.4.0
-----BEGIN SSH SIGNATURE-----
U1NIU0lHAAAAAQAAAhcAAAAHc3NoLXJzYQAAAAMBAAEAAAIBAKPZJFEnbUEMpjgH+rae0P
F71PPgEX77tsI8HSkSLfnhj9mdZhTWmVtMkM1y5thLnmY5BFMXgATsKQadB/E/qNDYs0kT
F7oYjHaAp9jkwIL69bGf5M+RFhV4NxQo90A86Bh3yWFkfG3RidmEfWthvknRYw4E1QRiyR
zgBSIZ9VZgaMtu3cK5MHgNchAF0jkQBZ4YYAhCBSsFWwr/JYCFQxgBeG5tGBEfWjjDg+RQ
Vh+lU0iKForTV8/S0LzPdLI2MBexaG0eEVUMt3GQnCX/b/IpUSPmjyEo52Z9EBIRMUjlS6
89ibY62qXryktv85yqK5DJb+d2AvaavN39bakwSzkSOrRcTFyhC2j1w1WsUx1Jw8bFRQkb
ImUp1SDErizQRFKdFQGxeXDw+a4oXZrALJ9ABoCgjbXCPsc0Yq5w0QwJkA80zBxuyouC0g
PQwx9ZxwNgKfLox0ytjSXs+XuUKgHWN9qFfIUO6RZ8Aq9TxX+VfZTp2IsuUX5x/pw4Rv+9
F8jrUuWlCFxBs4kPtVZQ5gUkVtggslVPY2Pn67KOj5Uy1oIQTw5BCzcLO8GdcQ4yDXlpso
LSvp1AWQeC1mWVbBc9EOTkKo/tEYbyb2bOJSbQfzG+dvND29W6eJVdhkNmya5H/AdCV06U
kbFo97K46wpRF4GelldOXNFm5DbZ7S2nAAAAA2dpdAAAAAAAAAAGc2hhNTEyAAACFAAAAA
xyc2Etc2hhMi0yNTYAAAIAAL8gc41EJAvXAcF2UnQVoOAJnLHKB9snsORMHn1z5HRcVULr
7Z8IfPBFUbQ7y09PQboL7SnaxutTb61KaFH+9GBBaXBDc6LHh0Q9CmARhTkyjAcsD/srVU
avkp7+2rphpl/5K6EaXcBIN8SmTim9w7TZiRBNenx2dNDKy2N2++Ugj0zKvQMIIjd3kuDc
NKYfIiAlE5TlO+9iIemvKLmh1ySPK5o6LIKKBZ7BEvgnHHadeFLTDa/HIS6zicZVhzP6F+
H4xJhMO/KWOsGUfi/SM+iiP9dvIL3Po2BX1AmGLo6oSEUB2I0eTcnz4YS1i02nkrJVS5zA
9WLVfgRcj/ed6vKxSgeESen9/KODn2mOLp4fin9dzGE2EogRRsHXO4p86LYte0gHV+GvQo
+iQ+VVcFelaZBnT3Y6MvXDNWMEUg/rwbVGSF63vf+5o/lEjAq/+KVRYkR8WvYo5msbbuOl
QCOpBdKWLbzvrCeSFk1CEbTpOg2YUVgBrP7M+p1fYSKp37zxxWWrhOYZ1WUIWvj70Dkhl+
mpsZM8EMZGYWTSBxX9rGRag8y133QfFy1tHVR2mws9a+bCWDrcfhykGOVpkFIH3NDA6zWC
7WJXS0+BYtbuppe5uQi+yosy6sV1TL1TPoYmI/f3o/sPJc53RTxY/XCE+y5IgXObCXqCtO
a4RjU=
-----END SSH SIGNATURE-----

* tag 'v1.4.0' of github.com:open-telemetry/opentelemetry-proto-go: (89 commits)
  Upgrade proto to v1.4.0 (open-telemetry#193)
  Don't run clean-gen twice (open-telemetry#214)
  clean the gen folder before running tidy (open-telemetry#211)
  Remove compat from go mod tidy (open-telemetry#213)
  Update module github.com/grpc-ecosystem/grpc-gateway/v2 to v2.24.0 (open-telemetry#209)
  Update google.golang.org/genproto/googleapis/api digest to e639e21 (open-telemetry#197)
  Update module golang.org/x/net to v0.31.0 (open-telemetry#203)
  Update google.golang.org/genproto/googleapis/rpc digest to e639e21 (open-telemetry#198)
  Update module golang.org/x/text to v0.20.0 (open-telemetry#206)
  Update module go.opentelemetry.io/build-tools/multimod to v0.15.0 (open-telemetry#201)
  Prevent the use of toolchain in go.mod (open-telemetry#202)
  Document and test supported versions of Go (open-telemetry#205)
  Switch from dependabot to renovate (open-telemetry#196)
  Bump google.golang.org/protobuf from 1.35.1 to 1.35.2 in /slim/otlp (open-telemetry#192)
  Bump google.golang.org/protobuf from 1.35.1 to 1.35.2 in /otlp (open-telemetry#191)
  Bump github.com/grpc-ecosystem/grpc-gateway/v2 in /otlp (open-telemetry#188)
  Bump google.golang.org/protobuf from 1.34.2 to 1.35.1 in /slim/otlp (open-telemetry#187)
  Bump google.golang.org/protobuf from 1.34.2 to 1.35.1 in /otlp (open-telemetry#186)
  Bump github.com/grpc-ecosystem/grpc-gateway/v2 in /otlp (open-telemetry#185)
  Bump github.com/grpc-ecosystem/grpc-gateway/v2 in /otlp (open-telemetry#184)
  ...
MikeGoldsmith added a commit to honeycombio/opentelemetry-proto-go that referenced this pull request Dec 12, 2024
@MikeGoldsmith
Merge tag 'v1.4.0' of github.com:open-telemetry/opentelemetry-proto-g…
5a97322 
…o into mike/update-proto-2

Module set all, Version v1.4.0
-----BEGIN SSH SIGNATURE-----
U1NIU0lHAAAAAQAAAhcAAAAHc3NoLXJzYQAAAAMBAAEAAAIBAKPZJFEnbUEMpjgH+rae0P
F71PPgEX77tsI8HSkSLfnhj9mdZhTWmVtMkM1y5thLnmY5BFMXgATsKQadB/E/qNDYs0kT
F7oYjHaAp9jkwIL69bGf5M+RFhV4NxQo90A86Bh3yWFkfG3RidmEfWthvknRYw4E1QRiyR
zgBSIZ9VZgaMtu3cK5MHgNchAF0jkQBZ4YYAhCBSsFWwr/JYCFQxgBeG5tGBEfWjjDg+RQ
Vh+lU0iKForTV8/S0LzPdLI2MBexaG0eEVUMt3GQnCX/b/IpUSPmjyEo52Z9EBIRMUjlS6
89ibY62qXryktv85yqK5DJb+d2AvaavN39bakwSzkSOrRcTFyhC2j1w1WsUx1Jw8bFRQkb
ImUp1SDErizQRFKdFQGxeXDw+a4oXZrALJ9ABoCgjbXCPsc0Yq5w0QwJkA80zBxuyouC0g
PQwx9ZxwNgKfLox0ytjSXs+XuUKgHWN9qFfIUO6RZ8Aq9TxX+VfZTp2IsuUX5x/pw4Rv+9
F8jrUuWlCFxBs4kPtVZQ5gUkVtggslVPY2Pn67KOj5Uy1oIQTw5BCzcLO8GdcQ4yDXlpso
LSvp1AWQeC1mWVbBc9EOTkKo/tEYbyb2bOJSbQfzG+dvND29W6eJVdhkNmya5H/AdCV06U
kbFo97K46wpRF4GelldOXNFm5DbZ7S2nAAAAA2dpdAAAAAAAAAAGc2hhNTEyAAACFAAAAA
xyc2Etc2hhMi0yNTYAAAIAAL8gc41EJAvXAcF2UnQVoOAJnLHKB9snsORMHn1z5HRcVULr
7Z8IfPBFUbQ7y09PQboL7SnaxutTb61KaFH+9GBBaXBDc6LHh0Q9CmARhTkyjAcsD/srVU
avkp7+2rphpl/5K6EaXcBIN8SmTim9w7TZiRBNenx2dNDKy2N2++Ugj0zKvQMIIjd3kuDc
NKYfIiAlE5TlO+9iIemvKLmh1ySPK5o6LIKKBZ7BEvgnHHadeFLTDa/HIS6zicZVhzP6F+
H4xJhMO/KWOsGUfi/SM+iiP9dvIL3Po2BX1AmGLo6oSEUB2I0eTcnz4YS1i02nkrJVS5zA
9WLVfgRcj/ed6vKxSgeESen9/KODn2mOLp4fin9dzGE2EogRRsHXO4p86LYte0gHV+GvQo
+iQ+VVcFelaZBnT3Y6MvXDNWMEUg/rwbVGSF63vf+5o/lEjAq/+KVRYkR8WvYo5msbbuOl
QCOpBdKWLbzvrCeSFk1CEbTpOg2YUVgBrP7M+p1fYSKp37zxxWWrhOYZ1WUIWvj70Dkhl+
mpsZM8EMZGYWTSBxX9rGRag8y133QfFy1tHVR2mws9a+bCWDrcfhykGOVpkFIH3NDA6zWC
7WJXS0+BYtbuppe5uQi+yosy6sV1TL1TPoYmI/f3o/sPJc53RTxY/XCE+y5IgXObCXqCtO
a4RjU=
-----END SSH SIGNATURE-----

* tag 'v1.4.0' of github.com:open-telemetry/opentelemetry-proto-go: (89 commits)
  Upgrade proto to v1.4.0 (open-telemetry#193)
  Don't run clean-gen twice (open-telemetry#214)
  clean the gen folder before running tidy (open-telemetry#211)
  Remove compat from go mod tidy (open-telemetry#213)
  Update module github.com/grpc-ecosystem/grpc-gateway/v2 to v2.24.0 (open-telemetry#209)
  Update google.golang.org/genproto/googleapis/api digest to e639e21 (open-telemetry#197)
  Update module golang.org/x/net to v0.31.0 (open-telemetry#203)
  Update google.golang.org/genproto/googleapis/rpc digest to e639e21 (open-telemetry#198)
  Update module golang.org/x/text to v0.20.0 (open-telemetry#206)
  Update module go.opentelemetry.io/build-tools/multimod to v0.15.0 (open-telemetry#201)
  Prevent the use of toolchain in go.mod (open-telemetry#202)
  Document and test supported versions of Go (open-telemetry#205)
  Switch from dependabot to renovate (open-telemetry#196)
  Bump google.golang.org/protobuf from 1.35.1 to 1.35.2 in /slim/otlp (open-telemetry#192)
  Bump google.golang.org/protobuf from 1.35.1 to 1.35.2 in /otlp (open-telemetry#191)
  Bump github.com/grpc-ecosystem/grpc-gateway/v2 in /otlp (open-telemetry#188)
  Bump google.golang.org/protobuf from 1.34.2 to 1.35.1 in /slim/otlp (open-telemetry#187)
  Bump google.golang.org/protobuf from 1.34.2 to 1.35.1 in /otlp (open-telemetry#186)
  Bump github.com/grpc-ecosystem/grpc-gateway/v2 in /otlp (open-telemetry#185)
  Bump github.com/grpc-ecosystem/grpc-gateway/v2 in /otlp (open-telemetry#184)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@florianl florianl florianl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dmathieu @pellared @florianl