Port cloc goal to v2 engine #8251

gshuflin · 2019-09-06T22:10:44Z

Problem

The cloc goal was using v1 engine abstractions rather than v2 engine abstractions

Solution

Rewrite the goal to use the v2 engine abstractions and delete the v1 engine code. After this commit, the ClocBinary subclass of BinaryToolBase is also removed since nothing is using it anymore.

Result

Nothing about the cloc goal should change from the user's perspective

Eric-Arellano

Looks good to me! All my suggestions are nits. Thank you for working on this!

src/python/pants/rules/core/cloc.py

src/python/pants/rules/core/cloc_test.py

src/python/pants/rules/core/register.py

src/python/pants/rules/core/cloc_test.py

…ctor (#8255) #8251 (comment) showed that `pathlib.Path` can take multiple args in the constructor, like `os.path.join()`, for a more ergonomic API that allows us to avoid using `/` to join two Paths. This also replaces `os.path` with `pathlib.Path` (and adds f-strings) to some files.

gshuflin · 2019-09-09T22:24:14Z

@stuhood @jsirois

jsirois

The only blocker is input_files.txt having non-deterministic content due to the set input being ordered by a graph walk. That graph walk could change over time as dependencies change but still produce the same set of files in a different order. Otherwise lgtm!

src/python/pants/rules/core/cloc.py

src/python/pants/rules/core/test_cloc.py

jsirois

Thanks! I'll try to keep an eye on CI and merge on green.

Eric-Arellano

Yay! Thank you!

Eric-Arellano · 2019-09-09T23:09:35Z

src/python/pants/rules/core/test_cloc.py

+  def alias_groups(cls):
+    return BuildFileAliases(targets={'java_library': JavaLibrary})
+
+  def assert_counts(self, result: Sequence[str], lang: str, num_files: int, blank: int, comment: int, code: int) -> None:


Only an FYI, not gonna make you do this. You've pushed enough changes. One of my favorite features in Python is requiring kwarg only by specifying *:

Suggested change

def assert_counts(self, result: Sequence[str], lang: str, num_files: int, blank: int, comment: int, code: int) -> None:

def assert_counts(self, result: Sequence[str], lang: str, *, num_files: int, blank: int, comment: int, code: int) -> None:

This means that anything after the star must use a keyword argument. You end up using kwargs anyways, but this would mean that the language itself enforces it for you.

That's a neat feature I didn't know about and I'll keep it in mind, but since this is test support code that we don't expect to use outside this test I think it's okay to leave this out.

stuhood

Thanks! One blocker due to sandbox leakage.

stuhood · 2019-09-10T05:52:17Z

src/python/pants/rules/core/cloc.py

+    sources = getattr(t, 'sources', None)
+    if sources is not None:
+      for f in sources.snapshot.files:
+        source_paths.add(str(Path(build_root, f)))


These are absolute, which means that the rule will escape its sandbox. Instead, they should be relative, so that the contents of the snapshot actually get used.

stuhood · 2019-09-10T05:53:09Z

src/python/pants/rules/core/cloc.py

+  transitive = options.values.transitive
+  ignored = options.values.ignored
+
+  if (transitive):


No need for parens here.

stuhood · 2019-09-10T05:55:06Z

src/python/pants/rules/core/cloc.py

+
+  file_outputs = {fc.path: fc.content.decode() for fc in files_content.dependencies}
+
+  with CountLinesOfCode.line_oriented(options, console) as (print_stdout, print_stderr):


This goal isn't line oriented I don't think: the mixin adds things that would break the output of cloc (separator character).

I think you want to skip extending LineOriented, and just use the methods of Console directly.

stuhood · 2019-09-10T05:57:41Z

src/python/pants/rules/core/cloc.py

+
+
+@rule(DownloadedClocScript, [])
+def download_cloc_script():


By hardcoding this, folks won't be able to change the version any longer.

I know that this isn't the first place that we've done this, but would you mind creating/referencing a ticket about adding support to the existing Subsystems for specifying per-platform digests for things, so that our existing options (--version, --binary-base-url, etc) can be used?

jsirois

More to come...

jsirois · 2019-10-12T01:44:48Z

src/python/pants/rules/core/cloc.py

+from pants.util.objects import datatype
+
+
+class DownloadedClocScript(datatype([('script_path', str), ('digest', Digest)])):


Nit: users do not care how the ClocScript is retrieved, just that it's the cloc script. Consider dropping the Dowloaded prefix here. In the rule function name, in contrast, I think the prefix makes perfect sense.

jsirois · 2019-10-12T04:18:44Z

src/python/pants/rules/core/cloc.py

+  digest = yield Get(Digest, DirectoriesToMerge(directories=tuple(digests_to_merge)))
+
+  cmd = (
+    '/usr/bin/perl',


This deserves a comment calling the hack out. It's a hack we also rely on to execute pexes, although we leverage PATH there in the form of the PythonSetup --interpreter-search-paths option:

pants/pants.remote.ini

Lines 30 to 40 in 455190b

[python-setup]

# TODO(#7735): This config is not ideal, that we must specify the PATH for both local and remote

# platforms. This should be replaced by a proper mechanism to differentiate between the two.

interpreter_search_paths: [

# We include the host PATH and PEXRC values so that speculation still works.

'<PATH>',

'<PEXRC>',

# This is the $PATH of the docker container, obtained by locally running `$ docker run --tag

# rbe-remote-execution sh -c 'echo $PATH'`.

"/pyenv-docker-build/versions/3.7.3/bin:/pyenv-docker-build/versions/3.6.8/bin:/pyenv-docker-build/versions/2.7.15/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin",

]

Consider filing an issue to track either:

Handling bootstrap of implicit binary requirements like these via BinaryUtil, or the more deep bootstrapping of a c compiler toolchain all the way through building perl all via rules.

Finding a way to express / impose these requirements on users so they get a better / earlier error message that the environment hosting the EPR doesn't have the required binary.

stuhood · 2019-10-12T16:12:01Z

src/python/pants/rules/core/cloc.py

+
+  digests_to_merge = []
+
+  build_root = get_buildroot()


Unused (thankfully!).

stuhood · 2019-10-12T16:17:53Z

src/python/pants/rules/core/cloc.py

+#TODO(#8459) - It would be nice if we weren't hardcoding a specific version of the cloc binary here.
+@rule
+def download_cloc_script() -> DownloadedClocScript:
+  url = "https://binaries.pantsbuild.org/bin/cloc/1.80/cloc"


Oh, hm. The URL actually needs to be constructed using BinaryUtil.Factory, because although it's fine to hardcode the version temporasrily, we can't really hardcode the URL (users use these tools from behind proxies).

@stuhood since this is not trivial to do (and since we already have an open ticket #7790 for this), in the interest of getting this PR merged I've removed the code that gets rid of the v1 version of cloc and renamed the v2 version fast-cloc. Since we're no longer dropping support for the v1 cloc, I think this should be okay to merge, and then fix the hardcoding in a subsequent commit.

stuhood · 2019-11-13T00:46:38Z

src/python/pants/rules/core/cloc.py

+#TODO(#8459) - It would be nice if we weren't hardcoding a specific version of the cloc binary here.
+@rule
+def download_cloc_script() -> DownloadedClocScript:
+  url = "https://binaries.pantsbuild.org/bin/cloc/1.80/cloc"


Eric-Arellano · 2019-11-13T00:59:45Z

src/python/pants/rules/core/cloc.py

+    register('--transitive', type=bool, fingerprint=True, default=True,
+             help='Operate on the transitive dependencies of the specified targets.  '
+                  'Unset to operate only on the specified targets.')


@cosmicexplorer @stuhood is this okay to still have given #8224?

Eric-Arellano approved these changes Sep 7, 2019

View reviewed changes

Eric-Arellano mentioned this pull request Sep 7, 2019

Refactor pathlib.Path usages to use multiple path segments constructor #8255

Merged

gshuflin force-pushed the new-v2-cloc branch from 3fad5fc to 330b5e0 Compare September 9, 2019 21:45

gshuflin marked this pull request as ready for review September 9, 2019 22:21

jsirois reviewed Sep 9, 2019

View reviewed changes

src/python/pants/rules/core/cloc.py Outdated Show resolved Hide resolved

src/python/pants/rules/core/cloc.py Outdated Show resolved Hide resolved

src/python/pants/rules/core/cloc.py Outdated Show resolved Hide resolved

src/python/pants/rules/core/test_cloc.py Outdated Show resolved Hide resolved

jsirois approved these changes Sep 9, 2019

View reviewed changes

Eric-Arellano approved these changes Sep 9, 2019

View reviewed changes

stuhood requested changes Sep 10, 2019

View reviewed changes

gshuflin force-pushed the new-v2-cloc branch from 61fe42e to 3f5cdc9 Compare October 12, 2019 00:47

jsirois reviewed Oct 12, 2019

View reviewed changes

jsirois approved these changes Oct 12, 2019

View reviewed changes

stuhood approved these changes Oct 12, 2019

View reviewed changes

src/python/pants/rules/core/cloc.py Outdated

digests_to_merge = []

build_root = get_buildroot()

Copy link

Member

stuhood Oct 12, 2019

Choose a reason for hiding this comment

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

Unused (thankfully!).

stuhood requested changes Oct 12, 2019

View reviewed changes

gshuflin force-pushed the new-v2-cloc branch from ae1c152 to 84bd87d Compare October 14, 2019 18:08

gshuflin force-pushed the new-v2-cloc branch 5 times, most recently from 9236da1 to 0591eb4 Compare November 13, 2019 00:33

stuhood approved these changes Nov 13, 2019

View reviewed changes

Eric-Arellano reviewed Nov 13, 2019

View reviewed changes

gshuflin force-pushed the new-v2-cloc branch from 0591eb4 to 95ac199 Compare November 13, 2019 19:14

Port cloc to v2

95ac199

gshuflin merged commit 58251ef into pantsbuild:master Nov 13, 2019

gshuflin deleted the new-v2-cloc branch November 13, 2019 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port cloc goal to v2 engine #8251

Port cloc goal to v2 engine #8251

gshuflin commented Sep 6, 2019 •

edited

Loading

Eric-Arellano left a comment

gshuflin commented Sep 9, 2019

jsirois left a comment

jsirois left a comment

Eric-Arellano left a comment

Eric-Arellano Sep 9, 2019

gshuflin Sep 9, 2019

stuhood left a comment

stuhood Sep 10, 2019

stuhood Sep 10, 2019

stuhood Sep 10, 2019

stuhood Sep 10, 2019

jsirois left a comment

jsirois Oct 12, 2019

jsirois Oct 12, 2019

stuhood Oct 12, 2019

stuhood Oct 12, 2019 •

edited

Loading

gshuflin Nov 13, 2019

stuhood Nov 13, 2019

stuhood Nov 13, 2019

Eric-Arellano Nov 13, 2019

	def assert_counts(self, result: Sequence[str], lang: str, num_files: int, blank: int, comment: int, code: int) -> None:
	def assert_counts(self, result: Sequence[str], lang: str, *, num_files: int, blank: int, comment: int, code: int) -> None:


		file_outputs = {fc.path: fc.content.decode() for fc in files_content.dependencies}

		with CountLinesOfCode.line_oriented(options, console) as (print_stdout, print_stderr):

		from pants.util.objects import datatype


		class DownloadedClocScript(datatype([('script_path', str), ('digest', Digest)])):

	[python-setup]
	# TODO(#7735): This config is not ideal, that we must specify the PATH for both local and remote
	# platforms. This should be replaced by a proper mechanism to differentiate between the two.
	interpreter_search_paths: [
	# We include the host PATH and PEXRC values so that speculation still works.
	'<PATH>',
	'<PEXRC>',
	# This is the $PATH of the docker container, obtained by locally running `$ docker run --tag
	# rbe-remote-execution sh -c 'echo $PATH'`.
	"/pyenv-docker-build/versions/3.7.3/bin:/pyenv-docker-build/versions/3.6.8/bin:/pyenv-docker-build/versions/2.7.15/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin",
	]

Port cloc goal to v2 engine #8251

Port cloc goal to v2 engine #8251

Conversation

gshuflin commented Sep 6, 2019 • edited Loading

Problem

Solution

Result

Eric-Arellano left a comment

Choose a reason for hiding this comment

gshuflin commented Sep 9, 2019

jsirois left a comment

Choose a reason for hiding this comment

jsirois left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Oct 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gshuflin commented Sep 6, 2019 •

edited

Loading

stuhood Oct 12, 2019 •

edited

Loading