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

Split the remotetool into a separate library #568

Merged
merged 1 commit into from
May 13, 2024

Conversation

gkousik
Copy link
Collaborator

@gkousik gkousik commented May 13, 2024

In this CL, I'm creating an embeddedtool binary so that I can subsequently use it in reproxytool binary in re-client to implement the log file filtering functionalities. There's no behavior change associated with this CL.

Tested: Ran show_action and show_action_2 (error'ed out) on the new remotetool.

@gkousik gkousik requested review from mrahs and ywmei-brt1 May 13, 2024 18:50
@gkousik gkousik changed the title Split the remotetool into its a separate binary Split the remotetool into a separate library May 13, 2024
@mrahs
Copy link
Collaborator

mrahs commented May 13, 2024

It sounds like you intend to import the new package from outside the cmd dir. Last I checked, go won't let you do that. A more suitable place would be go/pkg directory.

@gkousik
Copy link
Collaborator Author

gkousik commented May 13, 2024

It sounds like you intend to import the new package from outside the cmd dir. Last I checked, go won't let you do that. A more suitable place would be go/pkg directory.

ah, I intend to import the new package in reclient (reproxytool), but that's also inside the cmd dir. I tried and it seems to work.

@mrahs
Copy link
Collaborator

mrahs commented May 13, 2024

It sounds like you intend to import the new package from outside the cmd dir. Last I checked, go won't let you do that. A more suitable place would be go/pkg directory.

ah, I intend to import the new package in reclient (reproxytool), but that's also inside the cmd dir. I tried and it seems to work.

Ah, it looks like there is no inherent restrictions on importing from the cmd directory. The tooling only enforces that for internal packages: go.dev/doc/go1.4#internalpackages
OTOH, it's recommended for reusable packages to be outside the cmd directory: go.dev/doc/modules/layout

For the flags, it's not recommended to let packages define their own: https://google.github.io/styleguide/go/decisions#flags

@gkousik gkousik force-pushed the update-remotetool branch 4 times, most recently from 01cadfb to 9e32ffd Compare May 13, 2024 20:44
go/pkg/tool/embeddedtool.go Outdated Show resolved Hide resolved
In this CL, I'm creating an embeddedtool binary so that I can
subsequently use it in reproxytool binary in re-client to implement the
log file filtering functionalities. There's no behavior change
associated with this CL.

Tested: Ran show_action and show_action_2 (error'ed out) on the new
remotetool.
@gkousik
Copy link
Collaborator Author

gkousik commented May 13, 2024

Ah, it looks like there is no inherent restrictions on importing from the cmd directory. The tooling only enforces that for internal packages: go.dev/doc/go1.4#internalpackages OTOH, it's recommended for reusable packages to be outside the cmd directory: go.dev/doc/modules/layout

Moved to pkg/tool as we discussed offline and got rid of the separate embeddedtool pkg.

For the flags, it's not recommended to let packages define their own: https://google.github.io/styleguide/go/decisions#flags

Done!

@gkousik gkousik merged commit a7403b5 into bazelbuild:master May 13, 2024
6 checks passed
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.

2 participants