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

Travis CI tests on PHP 7.3 #3347

Merged
merged 1 commit into from
Nov 21, 2018
Merged

Travis CI tests on PHP 7.3 #3347

merged 1 commit into from
Nov 21, 2018

Conversation

BenMorel
Copy link
Contributor

Travis CI nightly is now PHP 7.4-dev. PHP 7.3 is now available on Travis.

This PR adds Travis tests on PHP 7.3.

@BenMorel
Copy link
Contributor Author

Never mind, it looks like xdebug is not available yet for PHP 7.3.

A beta version has been released last september, but it's still not readily available on Ubuntu AFAICS.

Let's put this PR on hold until a stable version comes to Ubuntu.

@Majkl578
Copy link
Contributor

Duplicate of unfinished #3323.

@Majkl578 Majkl578 marked this as a duplicate of #3323 Nov 14, 2018
@BenMorel
Copy link
Contributor Author

@Majkl578 Ah, sorry for the dup. Closing this one.

@BenMorel BenMorel closed this Nov 14, 2018
@Majkl578
Copy link
Contributor

If you want, take my commit and you can finish what @morozov suggested, I won't have time for that for next 5 days or so.

@BenMorel
Copy link
Contributor Author

Actually your commit is based on master, while mine is based on develop (no PHP 7.1 support).

Otherwise they're pretty much equivalent. @morozov are you OK if I apply your suggestions on the commit here?

@morozov
Copy link
Member

morozov commented Nov 15, 2018

@BenMorel as long as these are non-breaking changes, we can accept them in master. Sure, you can implement the suggestions from #3323 (comment) here and retarget the pull request.

@BenMorel
Copy link
Contributor Author

@morozov The problem is that master targets PHP 7.1+, while develop targets 7.2+.

Should I create 2 PRs, one targeting master and one targeting develop?

@morozov
Copy link
Member

morozov commented Nov 15, 2018

@BenMorel what is the problem? Regardless of the minimal supported version, we'll support PHP 7.3 in all actively supported branches. No need for two separate pull requests. develop is periodically rebased on top of the latest master.

@BenMorel
Copy link
Contributor Author

Ah, true, rebasing develop on master should do the job, if there are no conflicts.

OK, I'll pick up @Majkl578's commit and implement your suggestion.

@BenMorel BenMorel changed the base branch from develop to master November 15, 2018 20:45
@BenMorel BenMorel reopened this Nov 15, 2018
@BenMorel
Copy link
Contributor Author

OK, so here's what I've done:

  • Retargeted the PR on master and picked @Majkl578's commit
  • Default versions of sqlite, mysql and mysqli now run only on PHP 7.3 (they were previously running only on PHP 7.2, explicitly excluding 7.1, so I guess it makes sense to increment the version here)
  • Kept PHP 7.3 tests only on the latest version of each platform:
    • DB=mysql MYSQL_VERSION=5.7
    • DB=mysqli MYSQL_VERSION=5.7
    • DB=mariadb MARIADB_VERSION=10.3
    • DB=mariadb.mysqli MARIADB_VERSION=10.3
    • DB=pgsql POSTGRESQL_VERSION=11.0
  • Removed sqlsrv, pdo_sqlsrv and ibm_db2 tests on PHP 7.3: the sqlsrv and ibm_db2 PECL extensions do not yet support PHP 7.3 and do not compile

@morozov @Majkl578 What do you think?

@morozov
Copy link
Member

morozov commented Nov 15, 2018

Looks good so far. A couple of questions:

  1. While adding new builds, could you also reduce the number of nightly builds to not increase (or maybe even reduce) the number of builds in total? The pattern I want to achieve is:
    1. For the latest stable PHP version (which is currently 7.2), test all drivers with all platform versions.
    2. All other supported PHP versions (7.1, 7.3, nightly), test each driver with the latest version of the platform.
  2. Instead of removing sqlsrv and pdo_sqlsrv, could you try testing their v5.4.0-preview if it's not too difficult to build? It should work.

If the algorithm above makes sense, maybe we could have a script which takes all versions as input and generates the matrix? Just for the sake of discussion and as a reference point.

@BenMorel BenMorel force-pushed the travis_php73 branch 2 times, most recently from 1d9745e to b85ff57 Compare November 16, 2018 00:50
@BenMorel
Copy link
Contributor Author

  1. Sounds like a good strategy. I'm not sure whether I feel like writing a script to generate the matrix, but I can surely give it a try.
    Note that PHP 7.3 should be GA around December 6, so maybe we can be proactive and consider this one stable right now?
  2. You're right, 5.4.0preview works for both sqlsrv and pdo_sqlsrv! Fixed in b85ff57 👍

@morozov
Copy link
Member

morozov commented Nov 16, 2018

I'm not sure whether I feel like writing a script to generate the matrix, but I can surely give it a try.

You don't have to. I can give it a try myself. It's just easier to reason about an algorithm and play with the switches when it's implemented as code. If you can just update the matrix following the algorithm, it's fine too.

Note that PHP 7.3 should be GA around December 6, so maybe we can be proactive and consider this one stable right now?

We can do that but I was hoping we'd have all extensions in place by GA (at least sqlsrv and pdo_sqlsrv). Otherwise, we won't have full coverage where we're supposed to have it.

@BenMorel BenMorel force-pushed the travis_php73 branch 9 times, most recently from 0d28361 to 64b21f6 Compare November 16, 2018 16:39
@morozov
Copy link
Member

morozov commented Nov 20, 2018

@BenMorel thank you for the thorough explanation. It's most likely not a bug but internal changes in how PHP code is compiled to bytecodes. The change in the coverage is a side effect of that (e.g. like the Xdebug issue #1479).

@morozov
Copy link
Member

morozov commented Nov 20, 2018

Please squash the commits and let's get it merged.

@BenMorel
Copy link
Contributor Author

Can't you squash the commits with 1 button press?

image

@Majkl578
Copy link
Contributor

We don't squash-merge using GitHub UI because GitHub doesn't create a merge commit (which is what we want) - please squash the commits manually. 😊

@Majkl578
Copy link
Contributor

Also since you did quite some work, you may keep your work as separate commit, or add yourself into Co-Authored-By.

Co-authored-by: Michael Moravec <me@majkl.me>
Co-authored-by: Benjamin Morel <benjamin.morel@gmail.com>
@BenMorel
Copy link
Contributor Author

I didn't know who the commit would belong to, so I added both of our names (looks like it picked you as the owner, I guess because yours was the first commit?), anyway that seems to work 👍

For my personal knowledge, could you please explain very briefly what's the difference between a merge commit and the squash-and-merge functionality of GitHub?

@morozov
Copy link
Member

morozov commented Nov 21, 2018

If everyone is comfortable with the way the work is attributed, I'll merge it in a couple of hours:

Author: Michael Moravec <me@majkl.me>
Date:   Sun Aug 26 22:04:27 2018 +0200

    CI: Test against PHP 7.3 on Travis
    
    Co-authored-by: Michael Moravec <me@majkl.me>
    Co-authored-by: Benjamin Morel <benjamin.morel@gmail.com>

If it's a single commit, based on the amount of work done, I think it'd be fair if it's attributed like:

Author: Benjamin Morel <benjamin.morel@gmail.com>
Date:   Sun Aug 26 22:04:27 2018 +0200

    CI: Test against PHP 7.3 on Travis
    
    Co-authored-by: Michael Moravec <me@majkl.me>

What do you say?

@BenMorel
Copy link
Contributor Author

I don't mind either way!

@morozov morozov merged commit 296b0e5 into doctrine:master Nov 21, 2018
@morozov
Copy link
Member

morozov commented Nov 21, 2018

Done. Thank you @BenMorel and @Majkl578.

@BenMorel
Copy link
Contributor Author

Thanks 👍

@morozov could you please reply to my comment above?

I didn't get what you mean then. Do you mean a PR with the build matrix as generated by the script (without the root php: and env: entries), but without committing the script itself?

@Majkl578
Copy link
Contributor

For my personal knowledge, could you please explain very briefly what's the difference between a merge commit and the squash-and-merge functionality of GitHub?

The difference is:
a) merge commit exists so it's clear where the commit comes from,
b) multiple commits are allowed,
c) GPG signatures are retained.

@BenMorel BenMorel deleted the travis_php73 branch November 24, 2018 14:33
@morozov morozov self-assigned this Jun 7, 2019
@morozov morozov added this to the 2.9.0 milestone Jun 7, 2019
@morozov morozov mentioned this pull request Aug 20, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 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.

4 participants