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

Prefactor: Move get_hash from docker backend to strutil #18715

Merged
merged 14 commits into from
Apr 13, 2023
Merged

Prefactor: Move get_hash from docker backend to strutil #18715

merged 14 commits into from
Apr 13, 2023

Conversation

thejcannon
Copy link
Member

Prefactor for #18708

@thejcannon thejcannon added needs-cherrypick category:internal CI, fixes for not-yet-released features, etc. labels Apr 11, 2023
@thejcannon thejcannon added this to the 2.16.x milestone Apr 11, 2023
@thejcannon thejcannon requested review from stuhood and kaos April 11, 2023 16:25
@thejcannon
Copy link
Member Author

Needs CP to 2.1.6.x because #18708 needs it

stuhood
stuhood approved these changes Apr 11, 2023
View reviewed changes
src/python/pants/util/strutil.py Outdated Show resolved Hide resolved
kaos
kaos approved these changes Apr 11, 2023
View reviewed changes
src/python/pants/util/strutil.py Outdated Show resolved Hide resolved
kaos
kaos requested changes Apr 11, 2023
View reviewed changes
src/python/pants/util/strutil.py Outdated Show resolved Hide resolved
kaos
kaos approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Might want to add a summary of what we've discussed here in the doc string for stable_hash()?

@thejcannon @cognifloyd
Update src/python/pants/notes/2.16.x.md
614702e 
Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>
kaos
kaos approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Can leave it or take it regarding my comment on the json encoding.

@thejcannon
Copy link
Member Author

Hmm I guess one option here is hashing the pickled bytes. That might be more stable?

@cognifloyd
Copy link
Member

Hashing a pickle will not be consistent across versions of python.

@thejcannon
Copy link
Member Author

I think that's untrue, so long as we pin the pickle protocol version

@cognifloyd
Copy link
Member

I think that's untrue, so long as we pin the pickle protocol version

I haven't used pickle much. I didn't know you could pin the pickle protocol - I thought it was merely python-version specific. Good to know.

kaos
kaos approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

I'd say good enough. ;)

(I mean, to cover more bases I think we could do the class name thing for all os that is an instance of some class, but as I said.. enough already :p )

@thejcannon thejcannon merged commit 4ad4de8 into pantsbuild:main Apr 13, 2023
@thejcannon thejcannon deleted the dicthash branch April 13, 2023 16:25
thejcannon added a commit to thejcannon/pants that referenced this pull request Apr 13, 2023
@thejcannon
Prefactor: Move get_hash from docker backend to strutil (pantsbui…
54b19a4 
…ld#18715)

Prefactor for pantsbuild#18708
thejcannon added a commit that referenced this pull request Apr 13, 2023
@thejcannon
Prefactor: Move get_hash from docker backend to strutil (Cherry-p…
843cebc 
…ick of #18715) (#18738)

Prefactor for #18708
@kaos kaos mentioned this pull request Apr 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuhood stuhood stuhood approved these changes

@kaos kaos kaos approved these changes

@cognifloyd cognifloyd cognifloyd approved these changes

Assignees
No one assigned
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Milestone
2.16.x
Development

Successfully merging this pull request may close these issues.
4 participants
@thejcannon @cognifloyd @stuhood @kaos