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

Fix broken version API #61

Closed
wants to merge 1 commit into from
Closed

Fix broken version API #61

wants to merge 1 commit into from

Conversation

avsej
Copy link

@avsej avsej commented Dec 20, 2017

No description provided.

@ilovezfs
Copy link

CC @pwnall it would be great if you could take a look at this :) I'm inclined to accept it into the Homebrew formula as a patch for the time being for the sake of Homebrew/homebrew-core#21919.

iMac-TMP:~ joe$ diff -u /usr/local/opt/snappy/include/snappy-stubs-public.h ~/snappy-stubs-public.h 
--- /usr/local/opt/snappy/include/snappy-stubs-public.h	2017-08-24 16:54:23.000000000 -0700
+++ /Users/joe/snappy-stubs-public.h	2017-12-20 02:49:55.000000000 -0800
@@ -48,9 +48,9 @@
 #include <sys/uio.h>
 #endif  // HAVE_SYS_UIO_H
 
-#define SNAPPY_MAJOR 
-#define SNAPPY_MINOR 
-#define SNAPPY_PATCHLEVEL 
+#define SNAPPY_MAJOR 1
+#define SNAPPY_MINOR 1
+#define SNAPPY_PATCHLEVEL 7
 #define SNAPPY_VERSION \
     ((SNAPPY_MAJOR << 16) | (SNAPPY_MINOR << 8) | SNAPPY_PATCHLEVEL)

@pwnall
Copy link
Member

pwnall commented Dec 20, 2017

@ilovezfs I'm fairly sure this is what we'll deploy to fix the problem. pwnall@ea584ef

Thank you very much for pinging me! Is there anything else I can do to help?

@avsej
Copy link
Author

avsej commented Dec 20, 2017

@pwnall next time I should open PR to your fork?

pwnall added a commit that referenced this pull request Dec 20, 2017
Lands GitHub PR #61. The patch was also independently contributed by
Martin Gieseking <martin.gieseking@uos.de>.
@pwnall
Copy link
Member

pwnall commented Dec 20, 2017

@avsej Please keep opening PRs here. Sorry, I didn't have correct GitHub settings, and wasn't being notified.

Also, sorry, I had just gotten up when I saw this, and I didn't realize I'm looking at a PR here. I thought I'm looking at a conversation in Homebrew. I didn't see that the PR had exactly the same diff 😅

FWIW, my diff comes from https://groups.google.com/d/topic/snappy-compression/HOWxDETtj0Q/discussion which had arrived at a similar time. The commit that landed the diff 26102a0 does credit your PR. I hope this is an acceptable outcome.

@pwnall pwnall closed this Dec 20, 2017
@avsej
Copy link
Author

avsej commented Dec 20, 2017

Interesting, I took this patch from Martin's fix of the ticket I've opened on redhat bugzilla: https://src.fedoraproject.org/rpms/snappy/c/c60726a939e199f3968d75ddebde202963fdd8de?branch=master

So it looks like the loop has closed.

@ilovezfs
Copy link

Occam's razor wins again!

@pwnall
Copy link
Member

pwnall commented Dec 20, 2017

@avsej In this case, it all worked out, because Martin had signed our CLA.

In the future, if you submit code from other people, please make a note of it, so I can make the appropriate CLA checks before merging, and avoid potential headaches 😸

marcelo-rocha pushed a commit to bematech/snappy that referenced this pull request Jul 4, 2018
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