-
Notifications
You must be signed in to change notification settings - Fork 915
Support for building Windows wheels #451
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
Conversation
I think we'll want to publish Windows wheels for the next maintenance release, but perhaps mark them as experimental for the first release. |
@@ -881,7 +891,7 @@ static PyObject *Admin_alter_configs (Handle *self, PyObject *args, | |||
* Parse the list of ConfigResources and convert to | |||
* corresponding C types. | |||
*/ | |||
c_objs = alloca(sizeof(*c_objs) * cnt); | |||
c_objs = malloc(sizeof(*c_objs) * cnt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if any implications are there to allocating on the heap as opposed to the stack for these Admin operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*aside from needing to add free
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially used alloca() to avoid having to call free, alloca is also alot faster, but this isn't the hot-path so it doesn't matter.
Had to change it to malloc since the MSVC for Python 2.7 is really old and doesn't support alloca.
@@ -1444,7 +1444,7 @@ static int producer_conf_set_special (Handle *self, rd_kafka_conf_t *conf, | |||
rd_kafka_topic_conf_t *tconf, | |||
const char *name, PyObject *valobj) { | |||
|
|||
if (!strcasecmp(name, "on_delivery")) { | |||
if (!strcmp(name, "on_delivery")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an strcasecmp
equivalent for windows ? Otherwise tolower()
or some equivalent seem appropriate prior to comparison. While I agree function arguments should be case-sensitive they haven't been in the past so doesn't this technically break the api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are Windows alternatives, but since python is case-sensitive, and we haven't made any claims that on_delivery, et.al, may be specified in non lower-case, I think it is safe to remove this "feature".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're good with it I'm good with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I'm good with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.