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

expose a v2 ruleset for BinaryToolBase #8859

Merged
merged 6 commits into from
Feb 11, 2020
Merged

expose a v2 ruleset for BinaryToolBase #8859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
608a7be
add binary tool url fetch rule
cosmicexplorer Nov 29, 2019
3fb6b63
fix unit test failure
cosmicexplorer Jan 26, 2020
84d7712
make python lint integration tests work
cosmicexplorer Jan 26, 2020
451b7af
fix version arguments to fix integration test failures
cosmicexplorer Jan 27, 2020
ad3ae8d
cleanup rule splats
cosmicexplorer Feb 10, 2020
b2f0e6d
fix imports for pex subsystem
cosmicexplorer Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/python/pants/backend/awslambda/python/awslambda_python_rules_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,24 @@
from pants.engine.selectors import Params
from pants.rules.core import strip_source_root
from pants.testutil.option.util import create_options_bootstrapper
from pants.testutil.subsystem.util import init_subsystems
from pants.testutil.test_base import TestBase


class TestPythonAWSLambdaCreation(TestBase):

def setUp(self):
super().setUp()
init_subsystems([download_pex_bin.DownloadedPexBin.Factory])

@classmethod
def rules(cls):
return (
*super().rules(),
*awslambda_python_rules(),
*download_pex_bin.rules(),
# If we pull in the subsystem_rule() as well from this file, we get an error saying the scope
# 'download-pex-bin' was not found when trying to fetch the appropriate scope.
download_pex_bin.download_pex_bin,
Comment on lines +42 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

(See below commit in the bandit file about the possibility of trying to fix this.)

*inject_init.rules(),
*pex.rules(),
*pex_from_target_closure.rules(),
Expand All @@ -43,6 +50,7 @@ def rules(cls):
*strip_source_root.rules(),
*subprocess_environment.rules(),
RootRule(PythonAWSLambdaAdaptor),
RootRule(download_pex_bin.DownloadedPexBin.Factory),
)

def create_python_awslambda(self, addr: str) -> Tuple[str, bytes]:
Expand All @@ -52,6 +60,7 @@ def create_python_awslambda(self, addr: str) -> Tuple[str, bytes]:
Params(
target.adaptor,
create_options_bootstrapper(args=["--backend-packages2=pants.backend.awslambda.python"]),
download_pex_bin.DownloadedPexBin.Factory.global_instance(),
),
)
files_content = list(self.request_single_product(FilesContent,
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/backend/graph_info/subsystems/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
python_library(
dependencies = [
'src/python/pants/binaries',
'src/python/pants/engine:fs',
],
tags = {"partially_type_checked"},
)
12 changes: 11 additions & 1 deletion src/python/pants/backend/graph_info/subsystems/cloc_binary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from pants.binaries.binary_tool import Script
from pants.binaries.binary_tool import Script, ToolForPlatform, ToolVersion
from pants.engine.fs import Digest
from pants.engine.platform import PlatformConstraint


class ClocBinary(Script):
Expand All @@ -12,3 +14,11 @@ class ClocBinary(Script):

replaces_scope = 'cloc'
replaces_name = 'version'

default_versions_and_digests = {
PlatformConstraint.none: ToolForPlatform(
digest=Digest('2b23012b1c3c53bd6b9dd43cd6aa75715eed4feb2cb6db56ac3fbbd2dffeac9d',
546279),
version=ToolVersion('1.80'),
),
}
25 changes: 23 additions & 2 deletions src/python/pants/backend/python/lint/bandit/rules_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

from pants.backend.python.lint.bandit.rules import BanditTarget
from pants.backend.python.lint.bandit.rules import rules as bandit_rules
from pants.backend.python.rules import download_pex_bin, pex
from pants.backend.python.subsystems import python_native_code, subprocess_environment
from pants.backend.python.targets.python_library import PythonLibrary
from pants.build_graph.address import Address
from pants.build_graph.build_file_aliases import BuildFileAliases
Expand All @@ -17,10 +19,16 @@
from pants.rules.core.lint import LintResult
from pants.source.wrapped_globs import EagerFilesetWithSpec
from pants.testutil.option.util import create_options_bootstrapper
from pants.testutil.subsystem.util import init_subsystems
from pants.testutil.test_base import TestBase


class BanditIntegrationTest(TestBase):

def setUp(self):
super().setUp()
init_subsystems([download_pex_bin.DownloadedPexBin.Factory])

good_source = FileContent(path="test/good.py", content=b"hashlib.sha256()\n")
bad_source = FileContent(path="test/bad.py", content=b"hashlib.md5()\n")
# MD5 is a insecure hashing function
Expand All @@ -31,7 +39,16 @@ def alias_groups(cls) -> BuildFileAliases:

@classmethod
def rules(cls):
return (*super().rules(), *bandit_rules(), RootRule(BanditTarget))
return (
*super().rules(),
*bandit_rules(),
download_pex_bin.download_pex_bin,
*pex.rules(),
*python_native_code.rules(),
*subprocess_environment.rules(),
RootRule(BanditTarget),
RootRule(download_pex_bin.DownloadedPexBin.Factory),
)
Comment on lines -34 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this is a bit of a regression. The purpose of #9059 was to only need to have (*super().rules(), *bandit_rules(), RootRule(BanditTarget)) in the test runner and nothing else.

Can you please check these things:

  1. Can you remove *pex.rules(), *python_native_code.rules(), *subprocess_environment.rules() from here and see if the tests work?
  2. Does this tool still work if you remove from pants.ini pants.backend.python and only have pants.backend.python.lint.bandit enabled?

If possible, it'd also be great to get the DownloadPexBin.Factory thing working so that you don't need to special-case it here. I know you were having some trouble with that. What issue were you running into?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know you were having some trouble with that. What issue were you running into?

I don't recall this issue?

The purpose of #9059 was to only need to have (*super().rules(), *bandit_rules(), RootRule(BanditTarget))

This PR was from before then, so I suspect this change will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removing the same from the python awslambda test didn't work. I don't know if this was really "special-cased" as you describe -- I recall @gshuflin had this issue yesterday where we needed to inject PythonSetup in this exact way. I in fact used this PR to demonstrate how that was necessary. I'm not sure what kind of special-casing you're referring to with this comment and I am under the impression that injecting these as RootRules is correct. As @gshuflin was also discussing with me yesterday, if you're concerned about the boilerplate, factoring this code into a common test base class seems appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am under the impression that injecting these as RootRules is correct

We do not want to register subsystems as RootRules in tests, nor do we want to have to call init_subsystem in V2 tests. The subystem_rule declaration from production code should ideally work with the tests too.

Note that these lint tests did not previously need to call RootRule(PythonSetup), RootRule(SourceRootConfig), etc.

Originally, we actually did have to do this, but thanks to #8943 we no longer need to because we pass OptionsBootstrapper to the tests.

--

I recall @gshuflin had this issue yesterday where we needed to inject PythonSetup in this exact way.

We figured out the issue yesterday and no longer need to pass RootRule(PythonSetup). The issue yesterday was that we were passing RootRule(BuildRoot) when we shouldn't have, which meant then registering a bunch of rules that were unnecessary. See https://github.com/pantsbuild/pants/pull/9077/files#diff-dbe7c30c024e6830b7a7786c4521fbf2 for what we ended up registering.

if you're concerned about the boilerplate, factoring this code into a common test base class seems appropriate.

That's one option, but even better would be being able to go back to the only boilerplate being the test having to register this:

def rules():
  return (*super().rules(), *bandit_rules(), RootRule(BanditTarget))

The first win would be to try this:

Can you remove *pex.rules(), *python_native_code.rules(), *subprocess_environment.rules() from here and see if the tests work?

The next win would be

It'd also be great to get the DownloadPexBin.Factory thing working so that you don't need to special-case it here

Per this, Ok, removing the same from the python awslambda test didn't work., it sounds like that second part is a little harder to do though. What is the issue you run into with that second change?


def run_bandit(
self,
Expand Down Expand Up @@ -60,7 +77,11 @@ def run_bandit(
)
)
return self.request_single_product(
LintResult, Params(target, create_options_bootstrapper(args=args)),
LintResult, Params(
target,
create_options_bootstrapper(args=args),
download_pex_bin.DownloadedPexBin.Factory.global_instance(),
),
)

def test_single_passing_source(self) -> None:
Expand Down
30 changes: 27 additions & 3 deletions src/python/pants/backend/python/lint/black/rules_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

from pants.backend.python.lint.black.rules import BlackTarget
from pants.backend.python.lint.black.rules import rules as black_rules
from pants.backend.python.rules import download_pex_bin, pex
from pants.backend.python.subsystems import python_native_code, subprocess_environment
from pants.build_graph.address import Address
from pants.engine.fs import Digest, FileContent, InputFilesContent, Snapshot
from pants.engine.legacy.structs import TargetAdaptor
Expand All @@ -16,11 +18,16 @@
from pants.rules.core.lint import LintResult
from pants.source.wrapped_globs import EagerFilesetWithSpec
from pants.testutil.option.util import create_options_bootstrapper
from pants.testutil.subsystem.util import init_subsystems
from pants.testutil.test_base import TestBase


class BlackIntegrationTest(TestBase):

def setUp(self):
super().setUp()
init_subsystems([download_pex_bin.DownloadedPexBin.Factory])

good_source = FileContent(path="test/good.py", content=b'animal = "Koala"\n')
bad_source = FileContent(path="test/bad.py", content=b'name= "Anakin"\n')
fixed_bad_source = FileContent(path="test/bad.py", content=b'name = "Anakin"\n')
Expand All @@ -30,7 +37,16 @@ class BlackIntegrationTest(TestBase):

@classmethod
def rules(cls):
return (*super().rules(), *black_rules(), RootRule(BlackTarget))
return (
*super().rules(),
*black_rules(),
download_pex_bin.download_pex_bin,
*pex.rules(),
*python_native_code.rules(),
*subprocess_environment.rules(),
RootRule(BlackTarget),
RootRule(download_pex_bin.DownloadedPexBin.Factory),
)
Comment on lines +40 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto on the bandit comments


def run_black(
self,
Expand All @@ -56,8 +72,16 @@ def run_black(
lint_target = BlackTarget(target_adaptor)
fmt_target = BlackTarget(target_adaptor, prior_formatter_result_digest=input_snapshot.directory_digest)
options_bootstrapper = create_options_bootstrapper(args=args)
lint_result = self.request_single_product(LintResult, Params(lint_target, options_bootstrapper))
fmt_result = self.request_single_product(FmtResult, Params(fmt_target, options_bootstrapper))
lint_result = self.request_single_product(LintResult, Params(
lint_target,
options_bootstrapper,
download_pex_bin.DownloadedPexBin.Factory.global_instance(),
))
fmt_result = self.request_single_product(FmtResult, Params(
fmt_target,
options_bootstrapper,
download_pex_bin.DownloadedPexBin.Factory.global_instance(),
))
return lint_result, fmt_result

def get_digest(self, source_files: List[FileContent]) -> Digest:
Expand Down
25 changes: 23 additions & 2 deletions src/python/pants/backend/python/lint/flake8/rules_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

from pants.backend.python.lint.flake8.rules import Flake8Target
from pants.backend.python.lint.flake8.rules import rules as flake8_rules
from pants.backend.python.rules import download_pex_bin, pex
from pants.backend.python.subsystems import python_native_code, subprocess_environment
from pants.backend.python.targets.python_library import PythonLibrary
from pants.build_graph.address import Address
from pants.build_graph.build_file_aliases import BuildFileAliases
Expand All @@ -18,10 +20,16 @@
from pants.source.wrapped_globs import EagerFilesetWithSpec
from pants.testutil.interpreter_selection_utils import skip_unless_python27_and_python3_present
from pants.testutil.option.util import create_options_bootstrapper
from pants.testutil.subsystem.util import init_subsystems
from pants.testutil.test_base import TestBase


class Flake8IntegrationTest(TestBase):

def setUp(self):
super().setUp()
init_subsystems([download_pex_bin.DownloadedPexBin.Factory])

good_source = FileContent(path="test/good.py", content=b"print('Nothing suspicious here..')\n")
bad_source = FileContent(path="test/bad.py", content=b"import typing\n") # unused import
py3_only_source = FileContent(path="test/py3.py", content=b"version: str = 'Py3 > Py2'\n")
Expand All @@ -32,7 +40,16 @@ def alias_groups(cls) -> BuildFileAliases:

@classmethod
def rules(cls):
return (*super().rules(), *flake8_rules(), RootRule(Flake8Target))
return (
*super().rules(),
*flake8_rules(),
download_pex_bin.download_pex_bin,
*pex.rules(),
*python_native_code.rules(),
*subprocess_environment.rules(),
RootRule(Flake8Target),
RootRule(download_pex_bin.DownloadedPexBin.Factory),
)
Comment on lines +43 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto on the bandit comments


def run_flake8(
self,
Expand Down Expand Up @@ -61,7 +78,11 @@ def run_flake8(
)
)
return self.request_single_product(
LintResult, Params(target, create_options_bootstrapper(args=args)),
LintResult, Params(
target,
create_options_bootstrapper(args=args),
download_pex_bin.DownloadedPexBin.Factory.global_instance(),
),
)

def test_single_passing_source(self) -> None:
Expand Down
30 changes: 27 additions & 3 deletions src/python/pants/backend/python/lint/isort/rules_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

from pants.backend.python.lint.isort.rules import IsortTarget
from pants.backend.python.lint.isort.rules import rules as isort_rules
from pants.backend.python.rules import download_pex_bin, pex
from pants.backend.python.subsystems import python_native_code, subprocess_environment
from pants.build_graph.address import Address
from pants.engine.fs import Digest, FileContent, InputFilesContent, Snapshot
from pants.engine.legacy.structs import TargetAdaptor
Expand All @@ -14,11 +16,16 @@
from pants.rules.core.lint import LintResult
from pants.source.wrapped_globs import EagerFilesetWithSpec
from pants.testutil.option.util import create_options_bootstrapper
from pants.testutil.subsystem.util import init_subsystems
from pants.testutil.test_base import TestBase


class IsortIntegrationTest(TestBase):

def setUp(self):
super().setUp()
init_subsystems([download_pex_bin.DownloadedPexBin.Factory])

good_source = FileContent(path="test/good.py", content=b'from animals import cat, dog\n')
bad_source = FileContent(path="test/bad.py", content=b'from colors import green, blue\n')
fixed_bad_source = FileContent(path="test/bad.py", content=b'from colors import blue, green\n')
Expand All @@ -36,7 +43,16 @@ class IsortIntegrationTest(TestBase):

@classmethod
def rules(cls):
return (*super().rules(), *isort_rules(), RootRule(IsortTarget))
return (
*super().rules(),
*isort_rules(),
download_pex_bin.download_pex_bin,
*pex.rules(),
*python_native_code.rules(),
*subprocess_environment.rules(),
RootRule(IsortTarget),
RootRule(download_pex_bin.DownloadedPexBin.Factory),
)
Comment on lines +46 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto on the bandit comments


def run_isort(
self,
Expand Down Expand Up @@ -64,8 +80,16 @@ def run_isort(
target_adaptor, prior_formatter_result_digest=input_snapshot.directory_digest,
)
options_bootstrapper = create_options_bootstrapper(args=args)
lint_result = self.request_single_product(LintResult, Params(lint_target, options_bootstrapper))
fmt_result = self.request_single_product(FmtResult, Params(fmt_target, options_bootstrapper))
lint_result = self.request_single_product(LintResult, Params(
lint_target,
options_bootstrapper,
download_pex_bin.DownloadedPexBin.Factory.global_instance(),
))
fmt_result = self.request_single_product(FmtResult, Params(
fmt_target,
options_bootstrapper,
download_pex_bin.DownloadedPexBin.Factory.global_instance(),
))
return lint_result, fmt_result

def get_digest(self, source_files: List[FileContent]) -> Digest:
Expand Down
15 changes: 14 additions & 1 deletion src/python/pants/backend/python/lint/python_format_target_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
_ConcretePythonFormatTarget,
format_python_target,
)
from pants.backend.python.rules import download_pex_bin, pex
from pants.backend.python.subsystems import python_native_code, subprocess_environment
from pants.build_graph.address import Address
from pants.engine.fs import Digest, FileContent, InputFilesContent, Snapshot
from pants.engine.legacy.structs import TargetAdaptor
Expand All @@ -19,21 +21,31 @@
from pants.rules.core.fmt import AggregatedFmtResults
from pants.source.wrapped_globs import EagerFilesetWithSpec
from pants.testutil.option.util import create_options_bootstrapper
from pants.testutil.subsystem.util import init_subsystems
from pants.testutil.test_base import TestBase


class PythonFormatTargetIntegrationTest(TestBase):

def setUp(self):
super().setUp()
init_subsystems([download_pex_bin.DownloadedPexBin.Factory])

@classmethod
def rules(cls):
return (
*super().rules(),
format_python_target,
*black_rules(),
*isort_rules(),
download_pex_bin.download_pex_bin,
*pex.rules(),
*python_native_code.rules(),
*subprocess_environment.rules(),
Comment on lines +41 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto on the bandit comments

RootRule(_ConcretePythonFormatTarget),
RootRule(BlackTarget),
RootRule(IsortTarget),
RootRule(download_pex_bin.DownloadedPexBin.Factory),
)

def run_black_and_isort(
Expand All @@ -55,7 +67,8 @@ def run_black_and_isort(
args=[
"--backend-packages2=['pants.backend.python.lint.black', 'pants.backend.python.lint.isort']"
],
)
),
download_pex_bin.DownloadedPexBin.Factory.global_instance(),
)
)
return results
Expand Down
Loading