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.

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

Update fishhook submodule #3021

Closed
wants to merge 4 commits into from
Closed

Update fishhook submodule #3021

wants to merge 4 commits into from

Conversation

calebmackdavenport
Copy link
Contributor

@calebmackdavenport calebmackdavenport commented Oct 14, 2021

Description

Please feel free to edit this PR as much as you like.

In this pull request, I have …
Updated fishhook πŸŽ‰ facebook/fishhook@aadc161 πŸŽ‰
Thanks to @mikehardy #2895 (comment) for keeping an eye on fishhook and probably influencing the merge.

DetoxSync PR: wix-incubator/DetoxSync#11 (Also updates the submodule to grab existing dependabot updates)
DTXObjectiveCHelpers PR: wix-incubator/DTXObjectiveCHelpers#1

This fishhook fix resolves specifically the issue shown here: #2895 (comment)
Error Code: 0x00000007 (invalid protections for user data write)

Previous PR #3020


For features/enhancements:

  • N/A

For API changes:

  • N/A

@calebmackdavenport calebmackdavenport marked this pull request as ready for review October 14, 2021 17:29
@calebmackdavenport
Copy link
Contributor Author

@d4vidi feel free to use this PR and the ones on DetoxSync & ObjectiveCHelpers or close in favor of yours, did not see you had one you were working on. Thank you again πŸ™

@d4vidi
Copy link
Collaborator

d4vidi commented Oct 18, 2021

@calebmackdavenport my PR was just for some shallow testing.
Yours seems more promising -- thank you for your help (and Mike's!)

@d4vidi d4vidi closed this Oct 18, 2021
@d4vidi d4vidi reopened this Oct 18, 2021
@asafkorem asafkorem self-assigned this Oct 19, 2021
@asafkorem asafkorem requested review from asafkorem and removed request for alon-ha October 19, 2021 10:24
* update commits

* use detoxsync fishhook branch

* Fix gitmodules
@calebmackdavenport
Copy link
Contributor Author

πŸŽ‰ Tests pass πŸŽ‰

Not to over-explain if this is obvious, but I can't both have jenkins passing and be merge-ready.

I am pointing at commits that exist on:
https://github.com/calebmackdavenport/DTXObjectiveCHelpers.git
https://github.com/calebmackdavenport/DetoxSync.git

But do not exist on:
https://github.com/wix/DTXObjectiveCHelpers.git
https://github.com/wix/DetoxSync.git

As mentioned, please feel free to modify/edit/massage this PR as well as
wix-incubator/DetoxSync#11
wix-incubator/DTXObjectiveCHelpers#1

I am not the most familiar with submodules so definitely dismiss this or correct me, but it would probably easiest to:

  • Merge the two PRs on DetoxSync and DTXObjectiveCHelpers
  • Revert 22d13e3
  • Make sure this PR is pointing at the most recent commits on DetoxSync and DTXObjectiveCHelpers

@d4vidi
Copy link
Collaborator

d4vidi commented Oct 19, 2021

@calebmackdavenport thanks again for all of this. We found ourselves in the need for a more thorough cleanup (i.e. more than just adjusting the submodules' commits). We will give you all the credit when done πŸ™πŸ»

@asafkorem

@calebmackdavenport
Copy link
Contributor Author

calebmackdavenport commented Oct 19, 2021

@calebmackdavenport thanks again for all of this. We found ourselves in the need for a more thorough cleanup (i.e. more than just adjusting the submodules' commits). We will give you all the credit when done πŸ™πŸ»

@asafkorem

Awesome! Thank you and @asafkorem for the hard work 😁

@asafkorem
Copy link
Contributor

I'm closing this PR, the changes will take place in #3027.
@calebmackdavenport thanks again, note that I co-authorized you there.

@asafkorem asafkorem closed this Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants