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.

Sign up for GitHub

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

Jump to bottom

Simplify v1 Go buildgen's use of source roots. #9694

Merged
merged 3 commits into from May 5, 2020
Merged

Simplify v1 Go buildgen's use of source roots. #9694

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e844f4
Simplify Go buildgen's use of source roots.
benjyw May 4, 2020
ab0307f
Infer the remote root if we can.
benjyw May 5, 2020
9ca02d6
Address code review.
benjyw May 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion contrib/go/src/python/pants/contrib/go/tasks/BUILD
View file
Open in desktop
Expand Up @@ -15,7 +15,6 @@ python_library(
'src/python/pants/build_graph',
'src/python/pants/option',
'src/python/pants/process',
'src/python/pants/source',
'src/python/pants/task',
'src/python/pants/util:contextutil',
'src/python/pants/util:dirutil',
Expand Down
74 changes: 24 additions & 50 deletions contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py
View file
Open in desktop
Expand Up @@ -11,7 +11,6 @@
from pants.base.workunit import WorkUnitLabel
from pants.build_graph.address import Address
from pants.build_graph.address_lookup_error import AddressLookupError
from pants.source.source_root import SourceRootCategories
from pants.util.contextutil import temporary_dir
from pants.util.dirutil import safe_mkdir, safe_open

Expand All @@ -35,6 +34,9 @@ class WrongLocalSourceTargetTypeError(GenerationError):
For example, a Go main package was defined as a GoLibrary instead of a GoBinary.
"""

class RemoteRequired(GenerationError):
"""Indicates that the --remote-root was required but not specified."""

class NewRemoteEncounteredButRemotesNotAllowedError(GenerationError):
"""Indicates a new remote library dependency was found but --remote was not enabled."""

Expand Down Expand Up @@ -88,6 +90,10 @@ def _generate_missing(self, gopath, local_address, import_listing, visited):
remote_root, import_path
)
name = remote_pkg_path or os.path.basename(import_path)
if not self._remote_source_root:
raise self.RemoteRequired(
"You must provide the remote source root with --remote-root"
benjyw marked this conversation as resolved.
Show resolved Hide resolved
)
address = Address(os.path.join(self._remote_source_root, remote_root), name)
try:
self._build_graph.inject_address_closure(address)
Expand Down Expand Up @@ -215,6 +221,15 @@ def register_options(cls, register):
help="An optional extension for all materialized BUILD files (should include the .)",
)

register(
"--remote-root",
default=None,
Copy link
Member

@jsirois jsirois May 4, 2020 •
edited

Choose a reason for hiding this comment

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

It seems like it would be good to establish (or keep) a precedent instead of forcing every go user with external deps to add a pants.toml value for this. In fact, this is a breaking change as is with no deprecation cycle so I think adding the likely most common default - probably 3rdparty/go, at least with a dep cycle before removing, is required. This still breaks anyone out there who relied on implicit 3rd_party/go, thirdparty/go or third_party/go though.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I've added an attempt to infer which of the default options it is. It'll still break if someone set a custom remote root in [source], but I'm skeptical anyone has. And at least there's a sensible error message.

metavar="<path>",
advanced=True,
fingerprint=True,
help="Path to the remote golang source root, if any.",
)

def execute(self):
materialize = self.get_options().materialize
if materialize:
Expand Down Expand Up @@ -363,15 +378,9 @@ def gather_go_buildfiles(rel_path):
)
raise self.FloatingRemoteError("Un-pinned (FLOATING) Go remote libraries detected.")

class NoLocalRootsError(TaskError):
"""Indicates the Go local source owning targets' source roots are invalid."""

class InvalidLocalRootsError(TaskError):
"""Indicates the Go local source owning targets' source roots are invalid."""

class UnrootedLocalSourceError(TaskError):
"""Indicates there are Go local source owning targets that fall outside the source root."""

class InvalidRemoteRootsError(TaskError):
"""Indicates the Go remote library source roots are invalid."""

Expand Down Expand Up @@ -401,54 +410,19 @@ def generate_targets(self, local_go_targets=None):
# The GOPATH's 1st element is read-write, the rest are read-only; ie: their sources build to
# the 1st element's pkg/ and bin/ dirs.

go_roots_by_category = defaultdict(list)
# TODO: Add "find source roots for lang" functionality to SourceRoots and use that instead.
for sr in self.context.source_roots.all_roots():
if "go" in sr.langs:
go_roots_by_category[sr.category].append(sr.path)

if go_roots_by_category[SourceRootCategories.TEST]:
raise self.InvalidLocalRootsError("Go buildgen does not support test source roots.")
if go_roots_by_category[SourceRootCategories.UNKNOWN]:
raise self.InvalidLocalRootsError(
"Go buildgen does not support source roots of " "unknown category."
)
if not local_go_targets:
local_go_targets = self.context.scan().targets(self.is_local_src)
if not local_go_targets:
return None

local_roots = go_roots_by_category[SourceRootCategories.SOURCE]
if not local_roots:
raise self.NoLocalRootsError(
"Can only BUILD gen if a Go local sources source root is " "defined."
)
local_roots = {t.target_base for t in local_go_targets}
if len(local_roots) > 1:
raise self.InvalidLocalRootsError(
"Can only BUILD gen for a single Go local sources source "
"root, found:\n\t{}".format("\n\t".join(sorted(local_roots)))
"BUILD gen requires a single local Go source root, but found Go targets under "
f"multiple build roots: {', '.join(sorted(local_roots))}"
)
local_root = local_roots.pop()

if local_go_targets:
unrooted_locals = {t for t in local_go_targets if t.target_base != local_root}
if unrooted_locals:
raise self.UnrootedLocalSourceError(
"Cannot BUILD gen until the following targets are "
"relocated to the source root at {}:\n\t{}".format(
local_root,
"\n\t".join(sorted(t.address.reference() for t in unrooted_locals)),
)
)
else:
root = os.path.join(get_buildroot(), local_root)
local_go_targets = self.context.scan(root=root).targets(self.is_local_src)
if not local_go_targets:
return None

remote_roots = go_roots_by_category[SourceRootCategories.THIRDPARTY]
if len(remote_roots) > 1:
raise self.InvalidRemoteRootsError(
"Can only BUILD gen for a single Go remote library source "
"root, found:\n\t{}".format("\n\t".join(sorted(remote_roots)))
)
remote_root = remote_roots.pop() if remote_roots else None
remote_root = self.get_options().remote_root

generator = GoTargetGenerator(
self.import_oracle,
Expand Down
66 changes: 7 additions & 59 deletions contrib/go/tests/python/pants_test/contrib/go/tasks/test_go_buildgen.py
View file
Open in desktop
Expand Up @@ -57,36 +57,19 @@ def test_noop_no_applicable_targets(self):
task.execute()
self.assertEqual(expected, context.targets())

def test_no_local_roots_failure(self):
context = self.context(target_roots=[self.make_target("src/go/src/fred", GoBinary)])
task = self.create_task(context)
with self.assertRaises(task.NoLocalRootsError):
task.execute()

def test_multiple_local_roots_failure(self):
self.create_dir("src/go/src")
self.create_dir("src/main/go/src")
context = self.context(target_roots=[self.make_target("src/go/src/fred", GoBinary)])
context = self.context(
target_roots=[
self.make_target("src/go/src/fred", GoBinary),
self.make_target("src/main/go/src/barney", GoBinary),
]
)
task = self.create_task(context)
with self.assertRaises(task.InvalidLocalRootsError):
task.execute()

def test_unrooted_failure(self):
self.create_dir("src/go/src")
context = self.context(target_roots=[self.make_target("src2/go/src/fred", GoBinary)])
task = self.create_task(context)
with self.assertRaises(task.UnrootedLocalSourceError):
task.execute()

def test_multiple_remote_roots_failure(self):
self.create_dir("3rdparty/go")
self.create_dir("src/go/src/fred")
self.create_dir("other/3rdparty/go")
context = self.context(target_roots=[self.make_target("src/go/src/fred", GoLibrary)])
task = self.create_task(context)
with self.assertRaises(task.InvalidRemoteRootsError):
task.execute()

def test_existing_targets_wrong_type(self):
self.create_file(
relpath="src/go/src/fred/foo.go",
Expand Down Expand Up @@ -285,6 +268,7 @@ def stitch_deps_remote(self, remote=True, materialize=False, fail_floating=False
fred = self.make_target("src/go/src/fred", GoBinary)
target_roots = [fred]

self.set_options(remote_root="3rdparty/go")
context = self.context(target_roots=target_roots)
pre_execute_files = self.buildroot_files()
task = self.create_task(context)
Expand Down Expand Up @@ -350,42 +334,6 @@ def test_fail_floating(self):
with self.assertRaises(GoBuildgen.FloatingRemoteError):
self.stitch_deps_remote(remote=True, materialize=True, fail_floating=True)

def test_issues_2395(self):
# Previously, when a remote was indirectly discovered via a scan of locals (no target roots
# presented on the CLI), the remote would be queried for from the build graph under the
# erroneous assumption it had been injected. This would result in a graph miss (BUILD file was
# there on disk, but never loaded via injection) and lead to creation of a new synthetic remote
# target with no rev. The end result was lossy go remote library rev values when using the
# newer, encouraged, target-less invocation of GoBuildgen.

self.set_options(remote=True, materialize=True, fail_floating=True)
self.add_to_build_file(
relpath="3rdparty/go/pantsbuild.org/fake", target='go_remote_library(rev="v4.5.6")'
)

self.create_file(
relpath="src/go/src/jane/bar.go",
contents=dedent(
"""
package jane

import "pantsbuild.org/fake"

var PublicConstant = fake.DoesNotExistButWeShouldNotCareWhenCheckingDepsAndNotInstalling
"""
),
)
self.add_to_build_file(relpath="src/go/src/jane", target="go_library()")

context = self.context(target_roots=[])
pre_execute_files = self.buildroot_files()
task = self.create_task(context)
task.execute()

self.build_graph.reset() # Force targets to be loaded off disk
self.assertEqual("v4.5.6", self.target("3rdparty/go/pantsbuild.org/fake").rev)
self.assertEqual(pre_execute_files, self.buildroot_files())

def test_issues_2616(self):
self.set_options(remote=False)

Expand Down