Skip to content
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

Don't use the RC versions of sqlsrv/pdo_sqlsrv in AppVeyor #3153

Closed
wants to merge 4 commits into from

Conversation

mlocati
Copy link
Contributor

@mlocati mlocati commented May 15, 2018

Q A
Type improvement
BC Break no
Fixed issues

Summary

AppVeyor is currently configured to test the 5.2.0rc1 versions of the sqlsrv/pdo_sqlsrv PHP extensions.
BTW the very latest version is now 5.2.0: let's use it.
This is done by using the PhpManager PowerShell module, that's able to automatically install the most recent stable version of PHP extensions, without the need of manually specifying their download URL and version.

@mlocati
Copy link
Contributor Author

mlocati commented May 15, 2018

To speed up AppVeyor we could also install xdebug only if $Env:coverage is yes (otherwise it's not needed).

.appveyor.yml Outdated
}
if (!(Test-Path c:\tools\cacert\bundle.pem)) {
appveyor-retry appveyor DownloadFile https://curl.haxx.se/ca/cacert.pem -Filename C:\tools\cacert\bundle.pem
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed now, PHP block should end before installing Composer/Ocular - could you please move it above download Composer? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about appveyor-retry cinst -y sqlite?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should stay as part of the PHP block.
The question is whether we even need it, we don't test against SQLite (although we originally wanted to). Imho we can drop SQLite completely.

@Majkl578
Copy link
Contributor

AppVeyor is currently configured to test the 5.2.0rc1

That is because we started testing against PHP 7.2 at the time when no stable PECL version was compatible.

Otherwise I can't review the rest since I am not a Windows guy.

@Majkl578 Majkl578 requested a review from morozov May 15, 2018 11:59
@Majkl578 Majkl578 added the CI label May 15, 2018
@mlocati
Copy link
Contributor Author

mlocati commented May 15, 2018

AppVeyor is currently configured to test the 5.2.0rc1

That is because we started testing against PHP 7.2 at the time when no stable PECL version was compatible.

Yes, the 5.2.0 (not RC) is quite recent.

@morozov
Copy link
Member

morozov commented May 15, 2018

The pull request header looks misleading in the way that it's not about the RC versions but is about introducing PhpManager. BTW, the extension versions are updated in #3151 which looks like a much simpler change.

To the question of using PhpManager, given that our installation process is scripted anyways, what's the real use of having another CI dependency? Instead of just using the PHPs own way of configuring extensions, we'll use a 3rd party tool.

@mlocati
Copy link
Contributor Author

mlocati commented May 15, 2018

The only reason to use PhpManager is that's really easy to use. BTW obviously it's not mandatory.
Should I close this PR?

@morozov
Copy link
Member

morozov commented May 15, 2018

In my opinion, the "easy to use" argument is not really relevant in this case since we don't "use" anything on CI, things are pretty much static there. Given that most of the maintainers have little experience with Windows and PowerShell, introducing one more dependency will make the scripts even harder to maintain.

@mlocati
Copy link
Contributor Author

mlocati commented May 15, 2018

That's why there are libraries maintained by people that knows a specific language better...

@morozov
Copy link
Member

morozov commented May 15, 2018

From what I can see, it uses the same PowerShell language but instead of operating on the level of PECL packages and php.ini configuration which everyone knows about it has its own API. This API should be convenient for a daily use as a developer or for scripting different environments but in our case, we have just one environment which is static.

@mlocati
Copy link
Contributor Author

mlocati commented May 16, 2018

Yes, they are both PowerShell commands, but PhpManager abstracts pear/pecl, and make it very easy to write and maintain PHP installations.

For example, here's how the current DBAL script is translated to PhpManager:

  • From
appveyor-retry cinst --params '""/InstallDir:C:\tools\php""' --ignore-checksums -y php --version ((choco search php --exact --all-versions -r | select-string -pattern $env:php | sort { [version]($_ -split '\|' | select -last 1) } -Descending | Select-Object -first 1) -replace '[php|]','')
copy php.ini-production php.ini
Add-Content php.ini "`n date.timezone=UTC"
Add-Content php.ini "`n extension_dir=ext"
  • To
Install-Php -Version $env:php -Architecture $env:platform -ThreadSafe $false -Path c:\tools\php -TimeZone UTC -AddToPath System -InitialPhpIni Production
  • From
if (!(Test-Path C:\tools\cacert)) {
  New-Item -path c:\tools\ -name cacert -itemtype directory
}
if (!(Test-Path c:\tools\cacert\bundle.pem)) {
  appveyor-retry appveyor DownloadFile https://curl.haxx.se/ca/cacert.pem -Filename C:\tools\cacert\bundle.pem
}
Add-Content php.ini "`n curl.cainfo=C:\tools\cacert\bundle.pem"
  • To
Update-PhpCAInfo
  • From
Add-Content php.ini "`n memory_limit=1G"
  • To
Set-PhpIniKey -Key memory_limit -Value 1G
  • From
Add-Content php.ini "`n extension=php_openssl.dll"
Add-Content php.ini "`n extension=php_mbstring.dll"
Add-Content php.ini "`n extension=php_fileinfo.dll"
Add-Content php.ini "`n extension=php_pdo_sqlite.dll"
Add-Content php.ini "`n extension=php_sqlite3.dll"
Add-Content php.ini "`n extension=php_curl.dll"
  • To
Enable-PhpExtension -Extension openssl
Enable-PhpExtension -Extension mbstring
Enable-PhpExtension -Extension fileinfo
Enable-PhpExtension -Extension pdo_sqlite
Enable-PhpExtension -Extension sqlite3
Enable-PhpExtension -Extension curl
  • From
$DLLVersion = "5.2.0rc1"
cd c:\tools\php\ext
$source = "https://windows.php.net/downloads/pecl/releases/sqlsrv/$($DLLVersion)/php_sqlsrv-$($DLLVersion)-$($env:php)-nts-vc15-x64.zip"
$destination = "c:\tools\php\ext\php_sqlsrv-$($DLLVersion)-$($env:php)-nts-vc15-x64.zip"
Invoke-WebRequest $source -OutFile $destination
7z x -y php_sqlsrv-$($DLLVersion)-$($env:php)-nts-vc15-x64.zip > $null
$source = "https://windows.php.net/downloads/pecl/releases/pdo_sqlsrv/$($DLLVersion)/php_pdo_sqlsrv-$($DLLVersion)-$($env:php)-nts-vc15-x64.zip"
$destination = "c:\tools\php\ext\php_pdo_sqlsrv-$($DLLVersion)-$($env:php)-nts-vc15-x64.zip"
Invoke-WebRequest $source -OutFile $destination
7z x -y php_pdo_sqlsrv-$($DLLVersion)-$($env:php)-nts-vc15-x64.zip > $null
$DLLVersion = "2.6.0"
$source = "https://xdebug.org/files/php_xdebug-$($DLLVersion)-$($env:php)-vc15-nts-x86_64.dll"
$destination = "c:\tools\php\ext\php_xdebug.dll"
Invoke-WebRequest $source -OutFile $destination
Remove-Item c:\tools\php\* -include .zip
Add-Content php.ini "`nextension=php_sqlsrv.dll"
Add-Content php.ini "`nextension=php_pdo_sqlsrv.dll"
Add-Content php.ini "`nzend_extension=php_xdebug.dll"
  • To
Install-PhpExtension -Extension sqlsrv
Install-PhpExtension -Extension pdo_sqlsrv
Install-PhpExtension -Extension xdebug

Furthermore, disabling a PHP extension using Get-Content/Set-Content would be quite complex, whereas with PhpManager you only have to call Disable-PhpExtension.

About the problem of learning a new tool, well, it's not that hard with PowerShell (the first commands you learn when developing PS scripts are Get-Command -Module <module> to list the commands offered by a module, and Get-Help <command> to get extensive help about that command).

TL;DR: I thought that using PhpManager would have made maintainers' life easier, but it seems it's not the case: so please feel absolutely free to close this PR 😉

@morozov
Copy link
Member

morozov commented May 16, 2018

Sorry, I'm going to close it.

@morozov morozov closed this May 16, 2018
@morozov morozov self-assigned this May 16, 2018
@morozov morozov removed their request for review May 16, 2018 17:45
@mlocati mlocati deleted the sqlsrv-not-rc branch May 16, 2018 17:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants