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

Port engine/fs.py datatype() instances #6103

Merged
merged 7 commits into from Jul 14, 2018

Conversation

Projects
None yet
3 participants
@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Jul 12, 2018

This applies the principles learned in #6098 to the engine/fs.py classes like DirectoryToMaterialize. Refer to that PR for an explanation of why this is necessary.

Will fix the broken test in #6092. Part of #6062

pass


class DirectoryDigest(datatype([('fingerprint', str), ('serialized_bytes_length', int)])):
class DirectoryDigest(datatype([('fingerprint', binary_type), ('serialized_bytes_length', int)])):

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 12, 2018

Contributor

This has to be bytes, or we get a thread panic.

@mateor

mateor approved these changes Jul 12, 2018

Copy link
Member

mateor left a comment

Ah, much better than the options we discussed IRL, good find.

I am confused by the "part of" labels( as regarding linked PRs, not issues). This is attached to the PR of backend/jvm. Does this mean "needs merging as a prerequisite of" or "do not merge - this needs to land as a component of the linked PR"?

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Jul 12, 2018

To clarify, it's connected in these ways:

  • this must be merged first for #6092 to work, which is the port of backend/jvm
  • related to #6098, the fix of test_objects.py, because it uses the same principles, but they can be merged independently

illicitonion added a commit to twitter/pants that referenced this pull request Jul 13, 2018

Allow Rust to store `unicode` as well as `bytes`
This is currently unused, but will be used to land
pantsbuild#6103 with
DirectoryDigest.fingerprint being a `unicode` not a `bytes`.

illicitonion added a commit that referenced this pull request Jul 13, 2018

Allow Rust to store `unicode` as well as `bytes` (#6108)
This is currently unused, but will be used to land
#6103 with
DirectoryDigest.fingerprint being a `unicode` not a `bytes`.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:port-engine-fs branch from b218681 to 2bf1d71 Jul 13, 2018

@@ -198,7 +201,7 @@ def hackily_snapshot(self, context):
snapshot = context._scheduler.capture_snapshots((
PathGlobsAndRoot(
PathGlobs((script_relpath,)),
str(bootstrapdir),
native(str(bootstrapdir)),

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

Hah... given that we call the pants engine backend the "native" backend, this was not confusing at all =)

PathGlobsAndRoot(PathGlobs(("roland",), ()), temp_dir),
PathGlobsAndRoot(PathGlobs(("susannah",), ()), temp_dir),
PathGlobsAndRoot(PathGlobs(("*",), ()), temp_dir),
PathGlobsAndRoot(PathGlobs(("doesnotmatch",), ()), native(str(temp_dir))),

This comment has been minimized.

@stuhood

stuhood Jul 13, 2018

Member

This usage is a bit confusing to me. Why would we use two method calls rather than tobytes or text_type or whatever more specific thing is appropriate?

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 13, 2018

Contributor

Good point. text_type() works and is much cleaner. I forgot it can serve as a function and not just a type.

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@stuhood stuhood merged commit a1eec56 into pantsbuild:master Jul 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:port-engine-fs branch Jul 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment