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

Dep migration #1240

Merged
merged 2 commits into from Dec 7, 2018

Conversation

Projects
None yet
4 participants
@isaachier
Copy link
Contributor

commented Dec 6, 2018

Which problem is this PR solving?

  • Resumes work from #610.

Short description of the changes

  • Title says it all.
@codecov

This comment has been minimized.

Copy link

commented Dec 6, 2018

Codecov Report

Merging #1240 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1240   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         159     159           
  Lines        7145    7145           
======================================
  Hits         7145    7145

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b18bd9...a2e06b7. Read the comment docs.

@isaachier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

@vprithvi @yurishkuro any thoughts on this change?

@jpkrohling
Copy link
Member

left a comment

Shouldn't we go directly to go modules instead of dep?

Makefile Outdated
@@ -153,13 +156,13 @@ lint: lint-gosec
@./scripts/import-order-cleanup.sh stdout > $(IMPORT_LOG)
@[ ! -s "$(FMT_LOG)" -a ! -s "$(IMPORT_LOG)" ] || (echo "Go fmt, license check, or import ordering failures, run 'make fmt'" | cat - $(FMT_LOG) && false)

.PHONY: install-glide

This comment has been minimized.

Copy link
@jpkrohling

jpkrohling Dec 6, 2018

Member

Could we keep this for a while, as an alias to install-dep + WARN message stating that it's deprecated?

This comment has been minimized.

Copy link
@isaachier

isaachier Dec 6, 2018

Author Contributor

Sure good idea.

@isaachier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

@jpkrohling I hear the reasoning to move to Go modules. This PR addresses an Uber-specific issue, where we must use dep or glide, but do not support Go modules.

Makefile Outdated
@@ -1,15 +1,18 @@
PROJECT_ROOT=github.com/jaegertracing/jaeger
# TOP_PKGS is used with 'go test'
# TODO: try to do this without glide, since it may not be installed initially
TOP_PKGS := $(shell glide novendor | \
TOP_PKGS := $(shell go list ./... | \

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 6, 2018

Member

so:

$ time go list ./...
...
go list ./...  1.51s user 2.29s system 105% cpu 3.595 total

Can we do without it altogether? Do we still need TOP_PKGS? If we do, can we use something lighter than go list?

This comment has been minimized.

Copy link
@isaachier

isaachier Dec 6, 2018

Author Contributor

Personally remember writing a script for this in some PR but have to dig it up. If we absolutely need it, will let you know.

Makefile Outdated
@@ -155,11 +158,15 @@ lint: lint-gosec

.PHONY: install-glide
install-glide:
@which glide > /dev/null || go get github.com/Masterminds/glide
@echo "Jaeger has migrated to dep, please run 'make install-dep'" 1>&2

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 6, 2018

Member

this target should be removed

This comment has been minimized.

Copy link
@isaachier

isaachier Dec 6, 2018

Author Contributor

@jpkrohling disagreed above and I added this. Do you think the target is unhelpful?

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 6, 2018

Member

he said to fall back to dep, which this doesn't. Also, this looks like it's always been an internal target, not something used outside of make, so I don't see much value in keeping just the name.

# go-tests = true
# unused-packages = true


This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 6, 2018

Member

I am not sure this file is equivalent to glide.yaml. We can debate separately if we do or do not want to pin certain things, but for the purpose of this migration I would prefer no changes to the dependencies.

Specifically, dep defaults to ^, i.e. version = "1.16.0" in dep is equivalent to "^1.16.0" in glide. In order to do an actual pin, we need to use =, e.g. version = "=1.16.0"

This comment has been minimized.

Copy link
@isaachier

isaachier Dec 6, 2018

Author Contributor

Actually, I ran dep init which did all of the automatically. I was assuming it chose the correct convention.

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 6, 2018

Member

I don't think it did, none of the projects are pinned, while many were pinned in glide.

This comment has been minimized.

Copy link
@isaachier

isaachier Dec 6, 2018

Author Contributor

OK but why did we pin these. I imagine this will end up being a nuisance for internal dep to handle constraints.

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 6, 2018

Member

we don't pin all of them, but we do pin some, esp. I remember having issues with gogo versions and had to work out a consistent working set. Like I said, we should have a discussion about not-pinning, but this migration should not be introducing changes to dependencies, imo.

This comment has been minimized.

Copy link
@isaachier

isaachier Dec 6, 2018

Author Contributor

It is funny you say that because internally gogo/protobuf seems to be the packages causing the issue I have experienced.

@yurishkuro

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

Retain install-glide but print warning …

why?

@isaachier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Retain install-glide but print warning …

why?

Ask @jpkrohling.

@isaachier isaachier force-pushed the isaachier:dep-migration branch from 513dded to 101a1f4 Dec 6, 2018


[[constraint]]
name = "github.com/spf13/pflag"
version = "1.0.0"

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 6, 2018

Member

nit: maybe explicitly use ^ when we're not pinning?

@@ -92,7 +81,7 @@ clean:

.PHONY: test
test: go-gen
bash -c "set -e; set -o pipefail; $(GOTEST) $(TOP_PKGS) | $(COLORIZE)"
bash -c "set -e; set -o pipefail; $(GOTEST) ./... | $(COLORIZE)"

This comment has been minimized.

Copy link
@yurishkuro
@@ -112,7 +101,7 @@ all-srcs:
cover: nocover
@echo pre-compiling tests
@time go test -i $(ALL_PKGS)
@./scripts/cover.sh $(shell go list $(TOP_PKGS))
@./scripts/cover.sh $(shell go list ./...)

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro Dec 6, 2018

Member

I think we need to get rid of this script, it's no longer needed. See opentracing/opentracing-go#202

But can do in another PR (add a TODO)

@yurishkuro

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

please rebase

@jpkrohling

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

Retain install-glide but print warning …

why?

The idea is to give a chance to people who might be using this target. This seems internal, but it's possible that people might be building Jaeger on their own, applying custom patches to it. It would be polite to give them a heads-up before breaking their scripts.

Migrate to dep
Signed-off-by: Isaac Hier <isaachier@gmail.com>

@isaachier isaachier force-pushed the isaachier:dep-migration branch from 836893d to 609bb9c Dec 7, 2018

Warn on install-glide and redirect to use dep
Signed-off-by: Isaac Hier <ihier@uber.com>

@yurishkuro yurishkuro merged commit 5f6af99 into jaegertracing:master Dec 7, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 7b18bd9...a2e06b7
Details
codecov/project 100% (target 100%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@isaachier isaachier deleted the isaachier:dep-migration branch Dec 7, 2018

@justinclift justinclift referenced this pull request Jan 1, 2019

Closed

Replace Glide with Dep #610

3 of 3 tasks complete

@yurishkuro yurishkuro referenced this pull request Apr 2, 2019

Closed

Switch from Glide to Dep #483

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.