Skip to content

Conversation

@sergiodj
Copy link
Collaborator

We've been having a some problems with version comparison in the script. The approach we've taken so far is to decompose the current curl version into two variables (major and minor), and then perform comparisons against these components separately. It works, but it's confusing.

Another possible approach would be to use a C-style version normalization (basically a printf "%02d%02d") and then perform comparisons against the versions we want. The problem is that these versions need also to be normalized, which can be confusing as well.

I decided to implement the second approach but abstract it as a simple function that can take a regular version string like 8.16 as well as a comparison operator that will then be passed onto to test. This reads much nicer and abstracts the complexities of version normalization away. Unfortunately due to the limitations of shell scripting it's not easy to deduplicate code in this scenario, but that should be OK for now.

@sergiodj sergiodj requested a review from samueloph November 21, 2025 19:52
@sergiodj sergiodj marked this pull request as draft November 21, 2025 19:52
@sergiodj
Copy link
Collaborator Author

I'm marking this as a draft because I still want to implement some testcases.

Cc @samueloph @polkorny @vszakats

@sergiodj sergiodj force-pushed the improve-wcurl-version-comparison branch 3 times, most recently from 0b56031 to 1d65f7e Compare November 23, 2025 05:19
Sergio Durigan Junior added 2 commits November 23, 2025 00:21
It may be important to be able to specify a different curl binary to
be invoked (e.g., when performing tests against different versions of
curl, or when curl isn't installed in PATH).  This commit introduces a
new "--curl-binary" option.  If it's not specified, wcurl defaults to
using "curl".

Signed-off-by: Sergio Durigan Junior <[email protected]>
We've been having a some problems with version comparison in the
script.  The approach we've taken so far is to decompose the current
curl version into two variables (major and minor), and then perform
comparisons against these components separately.  It works, but it's
confusing.

Another possible approach would be to use a C-style version
normalization (basically a printf "%02d%02d%02d") and then perform
comparisons against the versions we want.  The problem is that these
versions need also to be normalized, which can be confusing as well.

I decided to implement the second approach but abstract it as a simple
function that can take a regular version string like "8.16.0" as well as
a comparison operator that will then be passed onto to "test".  This
reads much nicer and abstracts the complexities of version
normalization away.  Unfortunately due to the limitations of shell
scripting it's not easy to deduplicate code in this scenario, but that
should be OK for now.

Signed-off-by: Sergio Durigan Junior <[email protected]>
@sergiodj sergiodj force-pushed the improve-wcurl-version-comparison branch from 1d65f7e to 87ec4a9 Compare November 23, 2025 05:21
@sergiodj sergiodj marked this pull request as ready for review November 23, 2025 05:22
@sergiodj
Copy link
Collaborator Author

OK, I'm happy enough to consider this ready to be reviewed.

I had to implement a new --curl-binary option in order to facilitate testing, otherwise it'd be really hard to inject customized curl versions during runtime. This also makes wcurl a bit more flexible in scenarios where curl isn't installed in PATH.

Hopefully the commits are self-explanatory and the code isn't too hard to understand, but I'm happy to explain anything that might come up during review.

@vszakats
Copy link
Member

@sergiodj Thanks for the PR and the improvements! Agreed this solution reads
nicer, though I still prefer the original proposal for being short and light. In most
cases each of these comparisons are executed on every wcurl run, and normalizing
the constant version numbers dynamically into the MMNN (now MMNNPP) form
takes quite a few instructions (2 func calls, 4 printfs, 3 cut calls, 1 test), compared
to just 1 test call otherwise. I also don't think curl would ever introduce a new
command-line option in a patch release, so checking for it seems overkill to me.

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.

2 participants