desura / Desurium

Free online games platform (juegos gratis), with an open source client. LGPL repo for Desura client. Potentially out of date. See https://github.com/desura/desura-app for newest (LGPL) client.
https://www.desura.com/es
GNU General Public License v3.0
269 stars 44 forks source link

Bug 199v2 #618

Closed makson96 closed 10 years ago

makson96 commented 10 years ago

This is the second version of pull request, which fix #199 and is big step for #255 . It includes recommendations from karolherbst.

P.S. Please someone beside me test it before merging. P.S.2 After merging do not forget to inform Gentoo guys about build changes.

karolherbst commented 10 years ago

you could also git push --force remote branch so you don't have to create a new pull request ;) But yes, now everything seems to be fine. Thanks for the work.

makson96 commented 10 years ago

You are welcome :) I will try git push --force next time :)

I would really like to see this project incorporated into Debian. P.S. There is a guy on Debian mailing list asking for the update: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=656903 If this will get merged, we can inform him about it.

karolherbst commented 10 years ago

everybody is free to use the patches from this pull request until we merge this. I want to test this a bit deeply, just to be sure.

karolherbst commented 10 years ago

also the stuff in BuildCurl for ares support should be removed, too

makson96 commented 10 years ago

Since yesterday Ubuntu repository: https://launchpad.net/~makson96/+archive/desurium contains this pull request. It works great for me and, I haven't had any complains yet.

karolherbst commented 10 years ago

I managed to test this bug and could reproduce it on a ubuntu QEMU machine. But it took a lot of more time than just 5 minutes.

karolherbst commented 10 years ago

We should wait some days until we are sure nobody complains about crashed

makson96 commented 10 years ago

OK. We are not in a hurry here. I also want to take some time, to look and remove those Unix specific stuff, which you mentioned.

makson96 commented 10 years ago

So far no complains.

Do you want me to remove from BuildCurl:

else()
  find_package(OpenSSL REQUIRED)
  set(CURL_INSTALL_DIR ${CMAKE_EXTERNAL_BINARY_DIR}/curl)
  ExternalProject_Add(
    curl
    URL ${CURL_URL}
    URL_MD5 ${CURL_MD5}
    UPDATE_COMMAND ""
    BUILD_IN_SOURCE 1
    CONFIGURE_COMMAND ./configure
        --without-librtmp --disable-ldap --disable-curldebug
        --without-zlib --disable-rtsp --disable-manual --enable-static=yes 
        --enable-shared=no --disable-pop3 --disable-imap --disable-dict
        --disable-gopher --disable-verbose --disable-smtp --disable-telnet
        --disable-tftp --disable-file --without-libidn --without-gnutls
        --without-nss --without-cyassl --with-ssl --without-axtls
        --without-libssh2 --enable-hidden-symbols --enable-cookies --without-sspi
        --disable-manual --enable-optimize=-O2 --disable-ares ${CONFIGURE_DEBUG}
        --prefix=${CURL_INSTALL_DIR}
  )
else()
  list(APPEND CURL_LIBRARIES "${CURL_LIBRARY_DIR}/libcurl.a")
  list(APPEND CURL_LIBRARIES "${OPENSSL_LIBRARIES}")
  if(MINGW)
    list(APPEND CURL_LIBRARIES "ws2_32")
  else()
    list(APPEND CURL_LIBRARIES "rt")
  endif()
if(WITH_ARES)
  list(APPEND CURL_LIBRARIES ${CARES_LIBRARIES})
endif()

Will it not break for example mingw?

karolherbst commented 10 years ago

mingw is counted as WIN32 and is targeting win API anyway, so no c-ares there.

makson96 commented 10 years ago

I am not sure about this. "If" is: "if(WIN32 AND NOT MINGW)" so mingw seems to be counted to "else" together with Unix. I was referring to your "we Can actually remove all the Unix specific stuff from BuildCurl, because even Mac OS X is shipping curl" , rather than c-ares itself, which is only in the my last quotation of code, which I am asking if you want me to remove.