Skip to content

curltime: use libcurl time functions in src and tests/server #16653

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

Closed
wants to merge 6 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Mar 10, 2025

The curl tool and tests/server used 2 parallel implementations
of libcurl's Curl_now() and Curl_timediff() functions.

Make them use the libcurl one.


w/o ws https://github.com/curl/curl/pull/16653/files?w=1

@vszakats vszakats marked this pull request as draft March 10, 2025 13:58
@vszakats vszakats marked this pull request as ready for review March 10, 2025 14:40
@bagder
Copy link
Member

bagder commented Mar 10, 2025

We have traditionally tried to use the curlx_ prefix for functions we export "by source" from the library to the client and while I can't say exactly what problem we solve with this, I think maybe we should stick with it. It makes it more clear that we do this sharing on purpose.

@vszakats
Copy link
Member Author

We have traditionally tried to use the curlx_ prefix for functions we export "by source" from the library to the client and while I can't say exactly what problem we solve with this, I think maybe we should stick with it. It makes it more clear that we do this sharing on purpose.

I admit the rules guiding this didn't fully click for me, making it a bit fuzzy.

Wouldn't such renaming make this PR harder to follow? (except for *_now_init()
that's newly added.) Curl_now is called from a lot of places in libcurl.

Or just do a macro alias? But, that's hurting grepping for function names.

There also seems to be a bunch of existing functions that weren't renamed when
shared from libcurl, so I wonder if this'd better be done in a separate tidy-up commit,
for all functions involved? (pton/ntop, getpid, winapi_strerror, Curl_*alloc (?), nop_stmt)

When renaming shared ones to curlx_, what do we do with those in the same API
group, but not shared outside libcurl? E.g. Curl_timediff_ceil and Curl_timediff_us?
I guess we could rename them too for consistency, but I'm not sure.

(It makes me think that internal sharing could be done differently, but I couldn't
think of a coherent step that'd simplify things all around. Also perhaps this would
better be done after deprecating winbuild to make it easier to execute.)

Either way, let me know, and I can do the renaming here.

@jay
Copy link
Member

jay commented Mar 10, 2025

there was some problem having the same function name in both the tool and the library, some compilers did not like it, which led to some curlx prefixing like this and this , see #5946 discussion. i'm not sure if that's true throughout or even if it applies here but it was an issue. there's probably a better discussion of it but i can't find it atm

@vszakats
Copy link
Member Author

vszakats commented Mar 10, 2025

there was some problem having the same function name in both the tool and the library, some compilers did not like it, which led to some curlx prefixing like this and this , see #5946 discussion. i'm not sure if that's true throughout or even if it applies here but it was an issue. there's probably a better discussion of it but i can't find it atm

Interesting, these seem to make a clone of these APIs under a different prefix, possibly to avoid issues when linking two copies of these functions to targets? I wonder if that's still an issue or something to resolve on the build level instead. Worthy to keep in mind and test if'd be touching this in the future.

@bagder
Copy link
Member

bagder commented Mar 10, 2025

After further discussions I think we came to the conclusion that sticking to the curlx_ prefix is the preferred choice. To group them better.

We might in a somewhat longer term move towards extracting those functions into a separate "helper library" that then can be linked statically by both the tool, the library and test servers. Could save us from having to rebuild the same source files up to three times in a single build.

@github-actions github-actions bot added the CI Continuous Integration label Mar 11, 2025
@vszakats
Copy link
Member Author

vszakats commented Mar 11, 2025

Actually in this case, curlx_* is required. curlx_now() uses global variables which we initialize differently compared to the libcurl Curl_now(). This means two different copies of these functions pulled into the binary, one used by libcurl (Curl_now(), initialized via curl_global_init() → Curl_win32_init()) and another for tool/server code (curlx_now(), initialized via main() → win32_init() → curlx_now_init()).

dynbuf needs the same duplication due to conditional code. I'm guessing the rest may also need this for memdebug, but I'm not sure.

@vszakats vszakats closed this in 436d4a3 Mar 12, 2025
@vszakats vszakats deleted the curltime branch March 12, 2025 10:34
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
The curl tool and tests/server used 2 parallel implementations
of libcurl's `Curl_now()` and `Curl_timediff()` functions.

Make them use the libcurl one.

Closes curl#16653
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration cmdline tool tests
Development

Successfully merging this pull request may close these issues.

3 participants