binmode: convert to macro and use it from tests #15787

vszakats · 2024-12-20T10:57:55Z

And use it from src and tests.

Syncing this functionality between platforms and build targets.

Also: Stop redefining O_BINARY in src, and use a local macro with
the same effect. O_BINARY is used in CURL_SET_BINMODE() to decide
if this functionality is supported, and redefining it makes this check
pass always in unity builds. The check is required for Apple OS, because
it offers a setmode() function, successfully detected by both CMake
and autotools, but that function has a different functionality and
signature than that expected by CURL_SET_BINMODE().

Also:

drop MetaWare High C (MS-DOS) support for set binmode.
tests/libtest/Makefile.inc: dedupe comments.
lib/curl_setup_once.h: tidy up feature guards for io.h, fcntl.h.

Ref: #15652

``` In file included from curltool_unity.c:47: ../../include/../lib/curl_binmode.c:46:33: error: too many arguments to function call, expected 1, have 2 (void)setmode(fileno(stream), O_BINARY); ~~~~~~~ ^~~~~~~~ ../../src/tool_cb_wrt.c:43:18: note: expanded from macro 'O_BINARY' ^ /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/unistd.h:730:7: note: 'setmode' declared here void *setmode(const char *) __DARWIN_ALIAS_STARTING(__MAC_10_6, __IPHONE_2_0, __DARWIN_ALIAS(setmode)); ^ 1 error generated. ``` https://github.com/curl/curl/actions/runs/12061996281/job/33634959417#step:11:563

``` In file included from curltool_unity.c:47: ../../include/../lib/curl_binmode.c:46:33: error: too many arguments to function call, expected 1, have 2 (void)setmode(fileno(stream), O_BINARY); ~~~~~~~ ^~~~~~~~ ../../src/tool_operate.c:116:20: note: expanded from macro 'O_BINARY' ^ /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/unistd.h:730:7: note: 'setmode' declared here void *setmode(const char *) __DARWIN_ALIAS_STARTING(__MAC_10_6, __IPHONE_2_0, __DARWIN_ALIAS(setmode)); ^ 1 error generated. ``` https://github.com/curl/curl/actions/runs/12062155068/job/33635362901#step:11:563

bagder · 2024-12-20T12:38:37Z

But is this even used inside the library?

vszakats · 2024-12-20T12:54:02Z

No, not used from libcurl itself, but from tests/servers, tests/libtest (and src).
There is curltool lib where this could be put, but that's only linked to tests/unit.
Seems more straighforward to distribute it via libcurl, which we already link to
the necessary targets.

bagder · 2024-12-20T13:00:24Z

I agree that is indeed practical and probably the easiest way to do it, but I still believe it is the wrong approach. I think code in lib/ should only be for the library's own sake.

vszakats · 2024-12-20T13:35:44Z

Adding it to curltool and adding curltool as a dependency to everything
feels overkill for this tiny use.

Maybe converting this to a macro and offering it via the usual macro
ways (from lib but outside libcurl) could work? What do you think?

Replace _WIN32 with their own specific guards. They are always set for _WIN32, so the end result doesn't change.

bagder · 2024-12-20T15:29:18Z

Adding it to curltool and adding curltool as a dependency to everything feels overkill for this tiny use.

Yes, that also seems like the wrong thing to do.

Maybe converting this to a macro and offering it via the usual macro ways (from lib but outside libcurl) could work? What do you think?

Less bad, but I am against adding stuff to lib/ that is not necessary for the library itself. Even if that means that the alternative might mean duplicated code or more work.

I would prefer to have a dedicated source file in the tool's source code directory or something that the test servers can use in their build as well.

vszakats · 2024-12-20T15:52:17Z

I would prefer to have a dedicated source file in the tool's source code directory or something that the test servers can use in their build as well.

Converted to a macro and moved to src/tool_setup.h.

The seems fine given the simplicity of the macro.

I like the looks of it, but we can move it back out to a separate
src/tool_binmode.h, and include that where the macro is used.
Shall I do this?

(edit: I think the builds will have to be updated to pick up headers
from src, either way.)

bagder · 2024-12-20T16:03:29Z

I like the looks of it, but we can move it back out to a separate
src/tool_binmode.h, and include that where the macro is used.
Shall I do this?

Yeah, I think that sounds like the cleanest approach.

(edit: I think the builds will have to be updated to pick up headers
from src, either way.)

Yes, I believe we don't include anything from there before.

vszakats · 2024-12-20T16:20:42Z

OK, pushed!

While we're here, the set binmode macro now has the single mention
of __HIGHC__ in core code (with the other present in curl/system.h).
Traces say it was an MS-DOS compiler. Its last release was in the early 90s. ¹

It's doubtful it'd compile curl, regardless of this guard.

Do we need it?

https://winworldpc.com/product/metaware-high-c-cpp/2x ↩

bagder · 2024-12-20T16:21:53Z

Do we need it?

Nah.

vszakats added 11 commits December 20, 2024 11:43

tests/server: use _setmode with _WIN32

d6f018b

libtest/first: sync setmode with src/tool_binmode

79cd194

make set_binmode a curlx function

8df0cdc

make set_binmode a curlx function try fix 2

0c5926b

winbuild: update for curl_binmode 2

967a5a1

VisualStudioSolution: update for curl_binmode

caf4642

vms: drop reference to tool_binmode.c

6e94c46

dedupe comments

0ad433a

fixup comment

d4198b4

Makefile.inc: cleanup

c248a39

libtest cleanup

552bc0a

github-actions bot added cmdline tool tests labels Dec 20, 2024

vszakats mentioned this pull request Dec 20, 2024

build: improve building without deprecated Windows CRT symbols #15652

Closed

5 tasks

vszakats added 2 commits December 20, 2024 12:21

vszakats added 4 commits December 20, 2024 16:12

curl_setup_once.h: tidy up io.h and fcntl.h guards

e8e3fde

Replace _WIN32 with their own specific guards. They are always set for _WIN32, so the end result doesn't change.

convert to macro CURL_SET_BINMODE()

a0c356f

use macro CURL_SET_BINMODE()

d3abaf0

fixup to noop branch

93c6d5d

move to tool_setup.h

73f83d9

add src to header path for all libtests, servers

fe8b2c3

move macro back out to tool_binmode.h

b39f027

vszakats added 2 commits December 20, 2024 17:05

move macro back out to tool_binmode.h fixup

7a89dcc

cleanups

cf026e6

vszakats changed the title ~~binmode: move internal set binmode function to libcurl~~ binmode: convert set binmode to a macro and use it from tests Dec 20, 2024

vszakats changed the title ~~binmode: convert set binmode to a macro and use it from tests~~ binmode: convert to macro and use it from tests Dec 20, 2024

drop __HIGHC__

87906b2

vszakats closed this in 250d613 Dec 21, 2024

vszakats deleted the curl-setmode branch December 21, 2024 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

binmode: convert to macro and use it from tests #15787

binmode: convert to macro and use it from tests #15787

vszakats commented Dec 20, 2024 •

edited

Loading

bagder commented Dec 20, 2024

vszakats commented Dec 20, 2024 •

edited

Loading

bagder commented Dec 20, 2024

vszakats commented Dec 20, 2024

bagder commented Dec 20, 2024

vszakats commented Dec 20, 2024 •

edited

Loading

bagder commented Dec 20, 2024

vszakats commented Dec 20, 2024 •

edited

Loading

bagder commented Dec 20, 2024

binmode: convert to macro and use it from tests #15787

binmode: convert to macro and use it from tests #15787

Conversation

vszakats commented Dec 20, 2024 • edited Loading

bagder commented Dec 20, 2024

vszakats commented Dec 20, 2024 • edited Loading

bagder commented Dec 20, 2024

vszakats commented Dec 20, 2024

bagder commented Dec 20, 2024

vszakats commented Dec 20, 2024 • edited Loading

bagder commented Dec 20, 2024

vszakats commented Dec 20, 2024 • edited Loading

Footnotes

bagder commented Dec 20, 2024

vszakats commented Dec 20, 2024 •

edited

Loading

vszakats commented Dec 20, 2024 •

edited

Loading

vszakats commented Dec 20, 2024 •

edited

Loading

vszakats commented Dec 20, 2024 •

edited

Loading