windows: always use curl's basename() implementation #10475
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Unrelated failure while linking a test: Possibly a regression after 5a9a04d. |
bagder
added a commit
that referenced
this pull request
Feb 11, 2023
Fixes a build regression. Follow-up to 5a9a04d Reported-by: Viktor Szakats Ref: #10475 (comment)
jay
reviewed
Feb 12, 2023
| @@ -838,6 +838,10 @@ int getpwuid_r(uid_t uid, struct passwd *pwd, char *buf, | |||
| #define USE_HTTP3 | |||
| #endif | |||
|
|
|||
| #if defined(HAVE_BASENAME) && defined(WIN32) | |||
There was a problem hiding this comment.
I suggest a brief comment explaining why. Also is this a bug in mingw-w64 or is it our bug?
There was a problem hiding this comment.
Added a comment.
Sticking with the short example from #10261 (comment), this seems to be a mingw-w64 issue. But hard to say for certain without reproducing/debugging this with that exact (raw byte!) input and locale (and possibly the OS env). Also the original bug report is a slightly different case.
[ Just by eyeballing the source, there seems to be one bug at least, where the if (*refname) condition is always true, so the else branch never executes. ]
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
Feb 12, 2023
The `basename()` implementation provided by mingw-w64 [1] makes assumptions about input encoding and thus may break with non-ASCII strings. `basename()` was auto-detected with CMake, autotools and since 68fa9bf (2022-10-13), also in `Makefile.mk` after syncing its behaviour with the mainline build methods. A similar patch for curl-for-win broke official Windows builds earlier, in release 7.83.1_4 (2022-06-15). This patch forces all Windows builds to use curl's internal `basename()` implementation to avoid such problems. [1]: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/misc/basename.c Reported-by: UnicornZhang on Github Assisted-by: Cherish98 on Github Fixes curl#10261 Closes curl#10475
vszakats
force-pushed
the
win-no-basename
branch
from
58c6b08
to
2580f93
Compare
bagder
added a commit
that referenced
this pull request
Feb 12, 2023
Fixes a build regression. Follow-up to 5a9a04d Reported-by: Viktor Szakats Ref: #10475 (comment) Closes #10477
bagder
approved these changes
Feb 12, 2023
The `basename()` [1][2] implementation provided by mingw-w64 [3] makes assumptions about input encoding and thus may break with non-ASCII strings. `basename()` was auto-detected with CMake, autotools and since 68fa9bf (2022-10-13), also in `Makefile.mk` after syncing its behaviour with the mainline build methods. A similar patch for curl-for-win broke official Windows builds earlier, in release 7.83.1_4 (2022-06-15). This patch forces all Windows builds to use curl's internal `basename()` implementation to avoid such problems. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/basename.html [2]: https://www.man7.org/linux/man-pages/man3/basename.3.html [3]: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/misc/basename.c Reported-by: UnicornZhang on Github Assisted-by: Cherish98 on Github Fixes curl#10261 Closes curl#10475
vszakats
force-pushed
the
win-no-basename
branch
from
2580f93
to
14118ee
Compare
bch
pushed a commit
to bch/curl
that referenced
this pull request
Jul 19, 2023
Fixes a build regression. Follow-up to 5a9a04d Reported-by: Viktor Szakats Ref: curl#10475 (comment) Closes curl#10477
bch
pushed a commit
to bch/curl
that referenced
this pull request
Jul 19, 2023
The `basename()` [1][2] implementation provided by mingw-w64 [3] makes assumptions about input encoding and may break with non-ASCII strings. `basename()` was auto-detected with CMake, autotools and since 68fa9bf (2022-10-13), also in `Makefile.mk` after syncing its behaviour with the mainline build methods. A similar patch for curl-for-win broke official Windows builds earlier, in release 7.83.1_4 (2022-06-15). This patch forces all Windows builds to use curl's internal `basename()` implementation to avoid such problems. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/basename.html [2]: https://www.man7.org/linux/man-pages/man3/basename.3.html [3]: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/misc/basename.c Reported-by: UnicornZhang on Github Assisted-by: Cherish98 on Github Reviewed-by: Daniel Stenberg Fixes curl#10261 Closes curl#10475
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
basename()[1][2] implementation provided by mingw-w64 [3] makes assumptions about input encoding and may break with non-ASCII strings.basename()was auto-detected with CMake, autotools and since 68fa9bf (2022-10-13), also inMakefile.mkafter syncing its behaviour with the mainline build methods. A similar patch for curl-for-win broke official Windows builds earlier, in release 7.83.1_4 (2022-06-15).This patch forces all Windows builds to use curl's internal
basename()implementation to avoid such problems.[1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/basename.html
[2]: https://www.man7.org/linux/man-pages/man3/basename.3.html
[3]: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/misc/basename.c
Reported-by: UnicornZhang on Github
Assisted-by: Cherish98 on Github
Fixes #10261
Closes #10475