Skip to content

Conversation

@datibbaw
Copy link
Contributor

This is a collection of fixes that should improve the likelihood of it being picked up by package maintainers; it also includes a few memory fixes.

Package / build changes:

  • No longer bundle libuv; instead, link against an installation using --with-uv[=DIR]; it doesn't yet assert a particular version, it won't build on 0.12 so that needs fixing later on.
  • Only build the http parser when --enable-httpparser is used and make sure it builds with and without. I've created two separate source files for ease of maintenance. Later on we could separate it into its own module (but still within the same extension).
  • Remove Darwin framework inclusion; my clang didn't like those.
  • Add dependency on sockets extensions in config.m4.
  • Removed 5.3 target and added 5.6 target.

Bug fixes:

  • Updated some test cases that were dependent on resource id's; it now uses EXPECTF.
  • Fixed two memory overflows in uv_cwd() and uv_exepath(); it also uses the MAXPATHLEN.
  • Fixed parser test case.

@datibbaw
Copy link
Contributor Author

I understand that this is a rather large change to the project; feel free to discuss the work if you're unclear about certain things or if you outright disagree :)

@rdlowrey
Copy link
Contributor

👍 for --with-uv[=DIR] and --enable-httpparser

chobie added a commit that referenced this pull request Aug 26, 2014
@chobie chobie merged commit 5817bfe into chobie:master Aug 26, 2014
@chobie
Copy link
Owner

chobie commented Aug 26, 2014

@datibbaw Thanks, this makes easy to maintain extension and publishing as PECL.

@rdlowrey
Copy link
Contributor

I know I'm a couple of months late on this, but can I get a reminder for the logic behind adding the ext/sockets as a dependency? I know we discussed this off-board, @datibbaw, but I've forgotten what the reasons were. Might be nice to note those here for posterity.

@datibbaw
Copy link
Contributor Author

@rdlowrey From what I can remember, this library has always been dependent on sockets; i.e. it won't compile without it.

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.

3 participants