Skip to content

refactor: protobuf make targets for v0.50#51

Merged
damiannolan merged 6 commits intosdk-v0.50.xfrom
damian/proto-make-targets
Mar 10, 2025
Merged

refactor: protobuf make targets for v0.50#51
damiannolan merged 6 commits intosdk-v0.50.xfrom
damian/proto-make-targets

Conversation

@damiannolan
Copy link

@damiannolan damiannolan commented Mar 7, 2025

Diffs are mostly formatting to proto files

@github-actions
Copy link

github-actions bot commented Mar 7, 2025

PR Preview Action v1.6.0

🚀 View preview at
https://01builders.github.io/celestia-app/pr-preview/pr-51/

Built to branch gh-pages at 2025-03-07 12:15 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@damiannolan damiannolan marked this pull request as ready for review March 7, 2025 12:24
GRPC_GATEWAY_PROTOC_GEN_OPENAPIV2_VERSION=2.20.0

proto-deps:
@echo "Installing proto deps"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's bring this back and not use proto builder. It can be a blocker if ICL doesn't maintain it well. Depending on the tools directly should be fine.

Copy link
Author

Choose a reason for hiding this comment

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

I reverted to use raw tooling instead of docker image here 119d6d8

I had to add protoc-gen-gocosmos to list of deps and a few other tweaks to get things working, but seems good now

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for the changes in this file actually?

Copy link
Author

Choose a reason for hiding this comment

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

was just quickest to get things working by using docker image

Copy link
Collaborator

Choose a reason for hiding this comment

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

but what was there should have been working, this is how I generated the protos when we started to upgrade from 0.46. What wasn't working 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I think proto-gen was failing due to me not having protoc-gen-gocosmos iirc, plus linting was complaining with something else - that's when I decided to take a look at things. Happy to use the tooling directly tho, things are good now

@echo "--> Linting Protobuf files"
#? proto-format: Format Protobuf files
proto-format:
@find ./ -name "*.proto" -exec clang-format -i {} \;
Copy link
Author

Choose a reason for hiding this comment

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

should we add to makefile? :D

install-clang-format:
    @echo "Detecting OS..."
    @if [ "$(shell uname)" = "Darwin" ]; then \
	echo "Installing clang-format on macOS..."; \
	brew install clang-format; \
    elif [ "$(shell uname)" = "Linux" ]; then \
	echo "Installing clang-format on Linux..."; \
	sudo apt-get update && sudo apt-get install -y clang-format || \
	sudo yum install -y clang-tools-extra || \
	sudo dnf install -y clang-tools-extra;

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we can 😅 , bit ugly but if it's required. Might be nice to have one script that does platform based installation for all the tooling that's required. golanglint-ci, markdownlint, clang-format etc. But if this works out for now that's okay.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I mean, we can also just do without it for the time being and add one for all tooling if needs be. It's pretty obvious that you need to install it if its missing.

@damiannolan damiannolan merged commit 8a820ab into sdk-v0.50.x Mar 10, 2025
25 of 27 checks passed
@damiannolan damiannolan deleted the damian/proto-make-targets branch March 10, 2025 12:00
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