-
Notifications
You must be signed in to change notification settings - Fork 9
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
Refactoring makefile. #692
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor looks good. I see lot of redundancy over commands, should we try to reduce the amount of repeated or similar ones?
proto: proto-help | ||
proto-all: proto-gen | ||
|
||
# todo : @AJ needs to address this after removing third_party. Refer this for removal https://github.com/osmosis-labs/osmosis/blob/188abfcd15544ca07d468c0dc0169876ffde6079/scripts/makefiles/proto.mk#L39 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for now. To be addressed by @ajansari95 in his cleanup PR
scripts/makefiles/proto.mk
Outdated
@sh ./scripts/protocgen.sh | ||
|
||
# todo : @AK need the reason why it was there earlier | ||
proto-gen-1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is SDK 47 way of doing using ghcr.io/cosmos/proto-builder docker.
And ./scripts/protocgen.sh is using, buf generate --template "${project_dir}/proto/buf.gen.gogo.yaml" $file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. So we can get rid of it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure is good. Is there any way to test all the Makefile commands in one go in local. Like a command that will build, test and try maximum possibilities. Not necessarily to be part of this PR.
Yes. We can make a general command in makefile that includes all of them together. Something like |
I see only distroless build as the redundant one. Any others that you could find? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lg
1. Overview
Restructuring makefile to keep make commands more readable and easy to execute.
2. Implementation details
Adding different makefiles in scripts/makefiles
Separating them into :
3. How to test/use
Run commands one by one to test them.