Move code around to declutter the src/python/pants directory

[gshuflin]

• remove clean_global_runtime_state in lieu of calling its single line at its single callsite
• Move lock.py into the pantsd directory, since it is only ever called there. This allows src/python/pants/process to be removed.
• Move is_child_of and its associated test to its single callsite. This allows src/python/pants/fs to be removed.

[Eric-Arellano]

Excellent!

[Eric-Arellano]

Hm this technically breaks the "no side effects" thing with the engine by calling .resolve(). It's good this is now isolated to one place at least so that we're less likely to keep using it.

[gshuflin]

I spent a bit of time looking to see if there was a Python stdlib function that did this check for us already, but didn't find a convenient one.

[Eric-Arellano]

My concern is more using Path().resolve(). Ideally this function would solely use PurePath and not Path so that it didn't do any file operations. Those FS operations break the contract with the Rules API to avoid side effects.

This was a problem already before, so it's not a blocker. I'm not sure what the fix is in general, and this PR is good that it isolates this issue to only one call site.

[stuhood]

This should probably be implemented in terms of fast_relpath_optional.

[Eric-Arellano]

I think this is fine, but please wait for an ack from someone else.

[gshuflin]

test_util.py was breaking without the change just introduced in 17c507e - apparently the safe_filename_from_path function was concatenating the entire --pants-workdir path into a single long filename and creating a file by that name in --pants-physical-workdir-base. So when I removed that method, os.path.join was trying to concatenate two absolute paths, which resulted in it just using the 2nd path instead of creating a symlink within the first path. @stuhood want to make sure that this doesn't do anything unexpected.

[stuhood]

Stop using safe_filename_from_path and delete the associated code. Outside of tests it is only used in one place, to make sure that the file path specified with the --pants-workdir option is less than 100 characters. This is potentially not even a problem, so that invocation has been replaced with a warning stating that long filenames may cause a problem on systems that limit the total length of a filename; and the code removed

So, while we should stop using safe_filename_from_path, we should almost certainly replace usages with creation of "rebased" directory structures rather than removing handling of long paths entirely.

Path-length and path-component-length are limited on real systems (many Docker mounts, for example), and when issues are encountered, they can break everything in surprising ways ("breaks only on my machine"). Particularly in large monorepos

By "rebased directory structures", I'm referring to generating alternate destinations for a file/target like src/python/example/something:blah or dist/src/python/example/something/blah.py, that re-base the relative path into a new destination like dist/src/python/example/something$blah or dist/src/python/example/something/blah.py.

[gshuflin]

Stop using safe_filename_from_path and delete the associated code. Outside of tests it is only used in one place, to make sure that the file path specified with the --pants-workdir option is less than 100 characters. This is potentially not even a problem, so that invocation has been replaced with a warning stating that long filenames may cause a problem on systems that limit the total length of a filename; and the code removed

So, while we should stop using safe_filename_from_path, we should almost certainly replace usages with creation of "rebased" directory structures rather than removing handling of long paths entirely.

Path-length and path-component-length are limited on real systems (many Docker mounts, for example), and when issues are encountered, they can break everything in surprising ways ("breaks only on my machine"). Particularly in large monorepos

By "rebased directory structures", I'm referring to generating alternate destinations for a file/target like src/python/example/something:blah or dist/src/python/example/something/blah.py, that re-base the relative path into a new destination like dist/src/python/example/something$blah or dist/src/python/example/something/blah.py.

Makes sense. This was intended to be a quick refactor commit, and it sounds like actually getting rid of this functionality is both a good idea and would best be done in a separate chunk of work. I will put safe_filename_from_path back for now.

[stuhood]

..

Makes sense. This was intended to be a quick refactor commit, and it sounds like actually getting rid of this functionality is both a good idea and would best be done in a separate chunk of work. I will put safe_filename_from_path back for now.

Mm, yea. Another reason to tackle this independently is that we now have duplicated codepaths doing this:

pants/src/python/pants/build_graph/address.py

Lines 301 to 318 in e9115cd

	@property
	def path_safe_spec(self) -> str:
	"""
	:API: public
	"""
	if self._relative_file_path:
	parent_count = self._relative_file_path.count(os.path.sep)
	parent_prefix = "@" * parent_count if parent_count else "."
	file_portion = f".{self._relative_file_path.replace(os.path.sep, '.')}"
	else:
	parent_prefix = "."
	file_portion = ""
	if parent_prefix == ".":
	target_portion = f"{parent_prefix}{self._target_name}" if self._target_name else ""
	else:
	target_name = self._target_name or os.path.basename(self.spec_path)
	target_portion = f"{parent_prefix}{target_name}"
	return f"{self.spec_path.replace(os.path.sep, '.')}{file_portion}{target_portion}"