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

Support for building Windows wheels #451

Merged
merged 1 commit into from Sep 19, 2018
Merged

Support for building Windows wheels #451

merged 1 commit into from Sep 19, 2018

Conversation

edenhill
Copy link
Contributor

No description provided.

@edenhill
Copy link
Contributor Author

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);
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

@edenhill edenhill Sep 19, 2018

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")) {
Copy link
Contributor

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?

Copy link
Contributor Author

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".

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

@rnpridgeon rnpridgeon left a comment

Choose a reason for hiding this comment

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

LGTM

@edenhill edenhill merged commit c71e075 into master Sep 19, 2018
@edenhill edenhill deleted the cibuildwin branch September 19, 2018 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants