Skip to content

Commit

Permalink
Fix test for curl 7.83.0
Browse files Browse the repository at this point in the history 
libcurl 7.83.0 removed some trailing exclamation marks from error
messages[1]; we have to cater to that.

[1] <curl/curl@6968fb9>
  • Loading branch information
@cmb69
cmb69 committed Apr 27, 2022
1 parent 3648610 commit a4179e4
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion ext/curl/tests/curl_basic_007.phpt
View file
Expand Up @@ -20,5 +20,5 @@ curl_close($ch);

?>
--EXPECTF--
string(%d) "No URL set!%w"
string(%d) "No URL set%A"

This comment has been minimized.

Sign in to view

Copy link
@mvorisek

mvorisek Apr 27, 2022 •
edited

Contributor

@cmb69 this must go into PHP-8.0, needed for Windows testing in GH Actions

This comment has been minimized.

Sign in to view

Copy link
@cmb69

cmb69 Apr 27, 2022

Author Contributor

Is anybody using cURL 7.83.0 with PHP-8.0? I mean, we can still backport if necessary.

This comment has been minimized.

Sign in to view

Copy link
@mvorisek

mvorisek Apr 27, 2022

Contributor

yes, it is needed at least with GH Action WIndows build/test env

This comment has been minimized.

Sign in to view

Copy link
@cmb69

cmb69 Apr 27, 2022

Author Contributor

No. At least so far only master is updated to use cURL 7.83.0, and it's not likely to change (soon).

This comment has been minimized.

Sign in to view

Copy link
@mvorisek

mvorisek Apr 27, 2022

Contributor

why this error then?

https://github.com/php/php-src/runs/6193829277?check_suite_focus=true#step:8:528

This comment has been minimized.

Sign in to view

Copy link
@cmb69

cmb69 Apr 27, 2022

Author Contributor

Oh, that uses the wrong branch on GH:

php-src/.github/scripts/windows/build_task.bat

Lines 36 to 40 in ad71c0e

if /i "%APPVEYOR_REPO_BRANCH:~0,4%" equ "php-" (
set BRANCH=%APPVEYOR_REPO_BRANCH:~4,3%
) else (
set BRANCH=master
)

https://github.com/php/php-src/runs/6193829277?check_suite_focus=true#step:7:111

While the branch should be PHP-8.0.

This comment has been minimized.

Sign in to view

Copy link
@mvorisek

mvorisek Apr 27, 2022

Contributor

in PR, target branch can be detected, but in non-PR branch push, this detection is wrong

should I parse https://github.com/php/php-src/blob/master/main/php_version.h#L8 or is there a better way to detect PHP x.y version prior compilation?

This comment has been minimized.

Sign in to view

Copy link
@cmb69

cmb69 Apr 27, 2022

Author Contributor

I suggest to give it try calling phpsdk_deps without --branch %BRANCH%. The branch should be properly determined by phpsdk_deps.

This comment has been minimized.

Sign in to view

Copy link
@mvorisek

mvorisek Apr 27, 2022

Contributor

var DEPS_DIR needs to be set

I have implemented a simple parsing: https://github.com/php/php-src/blob/cef3e35241bd17b82d5644f6d481a970bfdf0db1/.github/scripts/windows/find-target-branch.bat

this matches the original AppVeyour code, if you think we do not need --branch %BRANCH% and no DEPS_DIR (used in configure command), please submit a PR againts my branch for 8392

This comment has been minimized.

Sign in to view

Copy link
@cmb69

cmb69 Apr 28, 2022

Author Contributor

var DEPS_DIR needs to be set

Ah, right, but the %BRANCH% is only needed for caching, and that is not even relevant for our GH actions right now. I'm not strongly against implementing the version parsing for CI, but in the long run we should get rid of it, since it duplicates what is already done by the PHPSDK.

This comment has been minimized.

Sign in to view

Copy link
@andypost

andypost May 3, 2022

Contributor

Getting test fail of 8.1.6rc1 and 8.0.19rc1 with curl 7.83.0 https://pkgs.alpinelinux.org/packages?name=curl&branch=edge

TEST 4697/15394 [ext/curl/tests/curl_basic_007.phpt]
========DIFF========
001+ string(10) "No URL set"
001- string(%d) "No URL set!%w"
     int(3)
========DONE========
FAIL Test curl_error() & curl_errno() function without url [ext/curl/tests/curl_basic_007.phpt]

This comment has been minimized.

Sign in to view

Copy link
@andypost

andypost May 3, 2022

Contributor

Using this commit as patch allows to pass test, Thank you!

This comment has been minimized.

Sign in to view

Copy link
@cmb69

cmb69 May 3, 2022

Author Contributor

Okay, I'll backport the fix.

This comment has been minimized.

Sign in to view

Copy link
@cmb69

cmb69 May 3, 2022

Author Contributor

Done.

This comment has been minimized.

Sign in to view

Copy link
@andypost

andypost May 3, 2022

Contributor

Thank you ☺️
int(3)

0 comments on commit a4179e4

Please sign in to comment.