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

add sendfile method for streamsocket #4007

Merged
merged 10 commits into from
Dec 11, 2023
Merged

Conversation

bas524
Copy link
Contributor

@bas524 bas524 commented Apr 13, 2023

This PR is motivated by #3277
I want add sendfile method to the stream socket
This functionality is cross platform, i.e. on posix systems it uses sendfile, on windows - TransmitFile

@obiltschnig , @aleks-f , Please review this changes and than I'll push changes in vcproj files and cmake files. Because there are many changes which adds mswsock.lib.

P.S. And further I'll add sendfile functionality to the httpserver

@bas524
Copy link
Contributor Author

bas524 commented Apr 29, 2023

@obiltschnig , @aleks-f friendly reminder

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@aleks-f aleks-f added this to the Release 1.13.0 milestone Jun 10, 2023
@aleks-f
Copy link
Member

aleks-f commented Jun 10, 2023

@bas524 I'm tentatively marking this for 1.13 release, but we have to clarify the rationale for /DPOCO_NO_AUTOMATIC_LIBS first

@bas524
Copy link
Contributor Author

bas524 commented Jun 20, 2023

@aleks-f , You can see that when we enabling tests flag -DENABLE_CRYPTO=OFF is not working, and cmake windows builds failed with error libcrypto.lib not found
This problem can be fixed with /DPOCO_NO_AUTOMATIC_LIBS

@aleks-f
Copy link
Member

aleks-f commented Jul 11, 2023

@aleks-f , You can see that when we enabling tests flag -DENABLE_CRYPTO=OFF is not working, and cmake windows builds failed with error libcrypto.lib not found This problem can be fixed with /DPOCO_NO_AUTOMATIC_LIBS

That used to work fine, I'm not sure how it got broken. In any case, it seems to me that the solution would be to make sure libraries can be found by automatic link

@bas524
Copy link
Contributor Author

bas524 commented Jul 11, 2023

@aleks-f
I think it was an error by design i.e. when you enable tests, some variables force set in ON, for ex., ENABLE_CRYPTO...
this is cmake config string for windows build in ci.yml

cmake -S. -Bcmake-build -DENABLE_NETSSL_WIN=ON -DENABLE_NETSSL=OFF -DENABLE_CRYPTO=OFF -DENABLE_JWT=OFF -DENABLE_DATA=ON -DENABLE_DATA_ODBC=ON -DENABLE_DATA_MYSQL=OFF -DENABLE_DATA_POSTGRESQL=OFF -DENABLE_TESTS=ON -DCMAKE_CXX_FLAGS="/MP /EHsc" -DCMAKE_C_FLAGS=/MP

Pay attention on -DENABLE_NETSSL_WIN=ON and -DENABLE_CRYPTO=OFF
than look at CMakeLists.txt:274

if(ENABLE_NETSSL_WIN)
	set(ENABLE_UTIL ON CACHE BOOL "Enable Util" FORCE)
	if(ENABLE_TESTS)
		set(ENABLE_CRYPTO ON CACHE BOOL "Enable Crypto" FORCE)
	endif()
endif()

You can see that ENABLE_CRYPTO forces to ON

Than look at CMakeLists.txt:287

if(ENABLE_CRYPTO AND ENABLE_TESTS)
	set(ENABLE_NETSSL ON CACHE BOOL "Enable NetSSL" FORCE)
	set(ENABLE_NET ON CACHE BOOL "Enable Net" FORCE)
	set(ENABLE_UTIL ON CACHE BOOL "Enable Util" FORCE)
endif()

At this point ENABLE_NETSSL forces to ON, but libcrypto is absent
That's all

@aleks-f
Copy link
Member

aleks-f commented Nov 27, 2023

@bas524 this pull is wildly out of sync with devel, can't work on this. if you can tidy it up for 1.13.0, I'll look into it

@aleks-f
Copy link
Member

aleks-f commented Nov 27, 2023

@bas524 and mswsock.lib should be added to progen file where necessary, not directly into vs project files

Copy link
Member

@aleks-f aleks-f left a comment

Choose a reason for hiding this comment

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

For some reason, this pull does not trigger all CI, please create a new one.

Net/testsuite/src/SocketStreamTest.cpp Outdated Show resolved Hide resolved
Net/src/SocketImpl.cpp Show resolved Hide resolved
Net/src/SocketImpl.cpp Outdated Show resolved Hide resolved
Net/testsuite/src/SocketStreamTest.cpp Show resolved Hide resolved
add NotImplemented exception for unsupported platforms
exculude <sys/sendfile.h> for POCO_EMSCRIPTEN, because https://
github.com/emscripten-core/emscripten/pull/16234
@matejk matejk merged commit 24b7122 into pocoproject:devel Dec 11, 2023
3 checks passed
@bas524
Copy link
Contributor Author

bas524 commented Dec 11, 2023

@aleks-f, Hi!
Please tell me should I make a new PR? I'm asking because @matejk already merged my commit

@aleks-f
Copy link
Member

aleks-f commented Dec 11, 2023

@bas524 at this point, too late. But now we have the CI failure because VS projects were not regenerated and are missing the mswsock.lib (which should only be added for Net-dependent projects, not all of them)

All Net-dependent project should be regenerated with ProGen. There is a new PowerShell script to do it in bulk

@aleks-f
Copy link
Member

aleks-f commented Dec 11, 2023

All Net-dependent project should be regenerated with ProGen. There is a new PowerShell script to do it in bulk

alternatively, linking could be done like we have it in eg. Crypto on Windows:

#if POCO_EXTERNAL_OPENSSL == POCO_EXTERNAL_OPENSSL_SLPRO
#if defined(POCO_DLL)
#if OPENSSL_VERSION_PREREQ(1,1)
#pragma comment(lib, "libcrypto.lib")
#pragma comment(lib, "libssl.lib")
#else
#pragma comment(lib, "libeay32.lib")
#pragma comment(lib, "ssleay32.lib")
#endif
#else
#if OPENSSL_VERSION_PREREQ(1,1)
#if defined(_WIN64)
#pragma comment(lib, "libcrypto64" POCO_LIB_SUFFIX)
#pragma comment(lib, "libssl64" POCO_LIB_SUFFIX)
#else
#pragma comment(lib, "libcrypto32" POCO_LIB_SUFFIX)
#pragma comment(lib, "libssl32" POCO_LIB_SUFFIX)
#endif
#else
#pragma comment(lib, "libeay32" POCO_LIB_SUFFIX)
#pragma comment(lib, "ssleay32" POCO_LIB_SUFFIX)
#endif
#endif

@matejk
Copy link
Contributor

matejk commented Dec 11, 2023

I'll fix this ASAP.

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