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

Change dependency organization in bazel #3407

Merged
merged 4 commits into from
Nov 20, 2019

Conversation

lukedirtwalker
Copy link
Collaborator

@lukedirtwalker lukedirtwalker commented Nov 20, 2019

Instead of having all go dependencies in the WORKSPACE have 2 files:

  • tool_deps.bzl contains all deps for linting & tooling.
  • go_deps.bzl contains all deps for go code.

Also remove unused deps.

This change is Reviewable

@lukedirtwalker lukedirtwalker changed the title Remove unused dependencies Change dependency organization in bazel Nov 20, 2019
Copy link
Contributor

@kormat kormat left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)


lint_deps.bzl, line 1 at r1 (raw file):

load("@bazel_gazelle//:deps.bzl", "go_repository")

tool_deps might be a better name; it's not just linting deps that are in here.

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):
fetch.sh needs an update to read from deps files


Copy link
Contributor

@sustrik sustrik left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)


lint_deps.bzl, line 3 at r1 (raw file):

load("@bazel_gazelle//:deps.bzl", "go_repository")

def lint_deps():

Maybe add the comment about comments in the file here as well.

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @kormat and @sustrik)


lint_deps.bzl, line 1 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

tool_deps might be a better name; it's not just linting deps that are in here.

Done.


lint_deps.bzl, line 3 at r1 (raw file):

Previously, sustrik (Martin Sustrik) wrote…

Maybe add the comment about comments in the file here as well.

No longer needed. The folders need to be listed in fetch.sh

Copy link
Contributor

@sustrik sustrik left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kormat and @lukedirtwalker)


go_deps.bzl, line 13 at r2 (raw file):

        name = "com_github_antlr_antlr4",
        commit = "be58ebffde8e29c154192c019608f0a5b8e6a064",
        importpath = "github.com/antlr/antlr4",  # runtime/Go/antlr

Shouldn't we remove these comments now that the directory is defined elswhere?

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kormat and @sustrik)


go_deps.bzl, line 13 at r2 (raw file):

Previously, sustrik (Martin Sustrik) wrote…

Shouldn't we remove these comments now that the directory is defined elswhere?

We will once we have the go.mod file.

Copy link
Contributor

@kormat kormat left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @sustrik)


tools/fetch.sh, line 81 at r2 (raw file):

EOF

for q in $(bazel query "kind('go_repository rule', //external:*)"); do

Add --noshow_progress - it removes a line from stderr.


tools/fetch.sh, line 86 at r2 (raw file):

    fi
    if [[ $q == //external* ]]; then
        dep_name=${q:11} # in front is "//external:"

dep_name=${q#//external:}

Copy link
Contributor

@sustrik sustrik left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kormat)


tools/fetch.sh, line 81 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Add --noshow_progress - it removes a line from stderr.

Done.


tools/fetch.sh, line 86 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

dep_name=${q#//external:}

Done.

Copy link
Contributor

@kormat kormat left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukedirtwalker lukedirtwalker merged commit 836d6ec into scionproto:master Nov 20, 2019
@lukedirtwalker lukedirtwalker deleted the pubDepClean branch November 20, 2019 13:12
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