search

darrengarvey / cgi

CGI and FastCGI C++ Library
http://cgi.sf.net
51 stars 32 forks source link

Merge of CGI changes and CI related topics #14

Closed leutloff closed 7 years ago

leutloff commented 8 years ago

I have enhanced the CGI lib to support my needs over the last years. It would be nice to merge the changes back into the master branch. I was not aware that you have accepted pull requests in the past years. 8-(

What aspects are you interested in? Where should we start?

You can see my use case on GitHub: https://github.com/leutloff/bergcms

This pull request is not really meant for merge. It is a place-holder to discuss the future way...

darrengarvey commented 8 years ago

Hi @leutloff - Your project looks interesting. Thanks for the set of patches.

I wonder if it might be easier to have a quick video hangouts? We could screenshare and go through the patches and just get them merged or quickly fix any issues. What timezone are you in?

leutloff commented 8 years ago

Hi @darrengarvey, glad to hear your feedback. My timezone is Berlin aka GMT+2.

darrengarvey commented 8 years ago

This is looking really good. I'm happy to push this as is. Sorry we haven't had a chance to talk yet so this PR shouldn't have to wait around any longer.

Especially good to get Travis integration in there too. Thanks a lot!

I'm just looking at the conflicts. Not sure if you have already resolved them locally?

darrengarvey commented 8 years ago

A lot of the conflicts look benign, but one that is more real is the changes that conflict with e4e548b9 from @benmccart... Ah it's fine, I can just go with the older version, you've only changed one bit in that file.

Out of interest, do you run on Win32? The changes to support pipes were a nice addition, not sure if you've tried running with those changes at all?

darrengarvey commented 8 years ago

@leutloff - I've pushed a branch to merge-pr-14 that resolves the conflicts. Would you be able to have a look over it and check I didn't overwrite any fixes you made. There was a genuine conflict in name.hpp and maybe a couple of others related to shutdown_service() calls for various Boost versions. Hopefully I didn't screw up the merge.

Also I'm not sure if github will correctly credit you (and close the PR) if I push as is, ie. fixing the conflicts locally. Hope so, but if you'd rather re-push to your branch then that's fine by me.

leutloff commented 8 years ago

@darrengarvey: Thank you for the hard work on merging the long set of patches. I am glad that you have done it!

I have merged your changes into my master. They are working as expected on my development system and in CI (bergcms and cgi). The code is now depending on C++11 standard. This is good. Adapted the CMakefile to explicitly state this.

No, I am not running on win32. From to time to time I am building my source with Visual Studio and try to run the unit tests. It is important for me to write code that is working with different compilers.

The PUT request is not working as expected at the moment. I just get a meaningless exception when executing inside apache. My unit tests working with the request and response classes are working fine, though.

When you look at the console log of the CI runs you will see that some unit tests are not working. I do not know how they are meant to be used correctly. If you need more help with the CI feel free to ask.

Thanks a lot for providing the CGI library.

aggsol commented 7 years ago

when is this coming to the master branch?

darrengarvey commented 7 years ago

Now!

Sorry I thought this had already landed all the way back in October.

aggsol commented 7 years ago

lol. that was fast :+1: