php-seclib: Call to undefined method Crypt_Base::Crypt_Base()

Status changed to 'Confirmed' because the bug affects multiple users.

I have uploaded a fixed version of the phpseclib package to my PPA:

https://launchpad.net/~rhansen/+archive/ubuntu/bug1574058

It should finish building in a few minutes.

1.0.1-4 is in yakkety-proposed, so we will be able to mark the primary ubuntu task as "fix released" soon.

(BTW, why does -proposed even exist for the current development version of Ubuntu? Why not go straight to release?)

Hi Richard, thank you for taking this on.

> (BTW, why does -proposed even exist for the current development version of Ubuntu? Why not go straight to release?)

Please see https://wiki.ubuntu.com/ProposedMigration. It allows us to run dep8 tests on reverse test dependencies so updating one package cannot regress others. phpseclib is currently stuck in yakkety-proposed for this reason. The error looks quite similar to the problem you're fixing but may be in a different package.

I'm subscribing Nish to take a look as I think he may know more about a general solution for this.

This should not block an SRU to Xenial, but for a Xenial fix we want a minimal diff please. I expect a single changelog entry referencing this bug and a minimal patch, rather than basing on a newer Debian version. Just dropping the quilt patch as you have done is fine if that is the minimal requirement to fix the bug - is it? I appreciate that the newer Debian version reverted one change, but what isn't clear to me is whether that reversion is the minimal reversion necessary to fix this bug.

Additionally we should consider testing other reverse depends of phpseclib (if any exist?) for regressions, too. I think other reverse dependencies should be discussed in the Regression Potential section.

phpseclib 1.0.2-1 is in yakkety now.

I have attached a minimal debdiff to fix this. The minimal change required to properly fix this bug is to remove the patch added in 1.0.1-3. That patch renames class constructors to use the new-style __construct() name, but each rename is a backwards-incompatible change.

We could limit the revert to just the Crypt_Base class, but PHP code that uses another class's old-style constructor name would still be broken. I'm not sure if any of the reverse dependencies use the other classes' old-style constructor names, but users might have their own private code that does.

I filled in more details in the [Regression Potential] section. I'm not sure how to test the other reverse dependencies. Is there anything else I can do to help get this SRU approved?

@Richard, have you tried running the autopkgtests against this version? I expect it will fail, due to emitting the deprecation warnings.

Rather than doing a patch, would it be possible to SRU a sync of 1.0.1-4 (which is identical to the change suggested here) to 16.04?

autopkgtest results:

  * civicrm (civicrm-common, drupal7-mod-civicrm, wordpress-civicrm)
    doesn't have a debian/tests/control file
  * collabtive doesn't have a debian/tests/control file
  * dokuwiki doesn't have a debian/tests/control file
  * php-horde-mapi fails due to the warnings
  * php-numbers-words passes
  * php-horde-imp passes

I didn't test any other packages.

I would argue that the test failure in php-horde-mapi is a bug in php-horde-mapi, not phpseclib. php-horde-mapi's tests shouldn't fail due to an issue in a 3rd party dependency.

> Rather than doing a patch, would it be possible to SRU a sync of
> 1.0.1-4 (which is identical to the change suggested here) to 16.04?

That was my original suggestion, but @racb wanted a debdiff that mentions this bug in the changelog.

> I would argue that the test failure in php-horde-mapi is a bug in
> php-horde-mapi, not phpseclib. php-horde-mapi's tests shouldn't fail
> due to an issue in a 3rd party dependency.

It seems like you agree with me @nacc:

https://bugs.launchpad.net/ubuntu/+source/php-horde-mapi/+bug/1593003
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=827483

:)

On 23.06.2016 [23:36:20 -0000], Richard Hansen wrote:
> autopkgtest results:
>
> * civicrm (civicrm-common, drupal7-mod-civicrm, wordpress-civicrm)
> doesn't have a debian/tests/control file
> * collabtive doesn't have a debian/tests/control file
> * dokuwiki doesn't have a debian/tests/control file
> * php-horde-mapi fails due to the warnings
> * php-numbers-words passes
> * php-horde-imp passes
>
> I didn't test any other packages.
>
> I would argue that the test failure in php-horde-mapi is a bug in php-
> horde-mapi, not phpseclib. php-horde-mapi's tests shouldn't fail due to
> an issue in a 3rd party dependency.

Yep, I've sent a fix for php-horde-mapi to Debian, which we may need to
SRU to Xenial. But that will need to wait til its in Debian.
(http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=827483).

-Nish

> Yep, I've sent a fix for php-horde-mapi to Debian, which we may need
> to SRU to Xenial.

Do we need to SRU php-horde-mapi before we can SRU this?

On 24.06.2016 [01:10:06 -0000], Richard Hansen wrote:
> > Yep, I've sent a fix for php-horde-mapi to Debian, which we may need
> > to SRU to Xenial.
>
> Do we need to SRU php-horde-mapi before we can SRU this?

I think it will fail the autopkgtests and sit in -proposed until then,
yeah.

Careful. If the landing of one SRU breaks another package, it isn't sufficient just to SRU both. A Breaks: needs to be added so that users don't accidentally pick up one SRU without the other. See bug 1511735 for an example of how this can go wrong.

If I understand this correctly, what you want to do is:

SRU php-horde-mapi
SRU this phpseclib with a Breaks: php-horde-map (<< version-just-SRUd)

Then ask the SRU team to land both together. Though with the Breaks, apt will generally do the right thing if both don't land together, although it still could confuse users ("why won't phpseclib update?").

I assumed that php-horde-mapi would actually be broken at runtime though, as opposed to a test positive only. If it's not broken at runtime, then I guess the consequence isn't so severe. I tend to fall on the side of fixing the dep8 test in an SRU anyway though, as otherwise the test becomes useless in detecting SRU regressions.

On 27.06.2016 [12:11:41 -0000], Robie Basak wrote:
> Careful. If the landing of one SRU breaks another package, it isn't
> sufficient just to SRU both. A Breaks: needs to be added so that users
> don't accidentally pick up one SRU without the other. See bug 1511735
> for an example of how this can go wrong.

Thank you for noticing this, Robie, very good points.

It's not a real "Breaks" in this case, thankfully.

> If I understand this correctly, what you want to do is:
>
> SRU php-horde-mapi
> SRU this phpseclib with a Breaks: php-horde-map (<< version-just-SRUd)
>
> Then ask the SRU team to land both together. Though with the Breaks, apt
> will generally do the right thing if both don't land together, although
> it still could confuse users ("why won't phpseclib update?").
>
> I assumed that php-horde-mapi would actually be broken at runtime
> though, as opposed to a test positive only. If it's not broken at
> runtime, then I guess the consequence isn't so severe. I tend to fall on
> the side of fixing the dep8 test in an SRU anyway though, as otherwise
> the test becomes useless in detecting SRU regressions.

I'ts just a test positive issue, because PHP7 emits a warning to stderr
about deprecation (in src:phpseclib) of same-named constructors during
the test of php-horde-mapi. We can't fix phpseclib, as that causes
regressions like this one, so Debian (and 16.04) have add
src:php-phpseclib, which is actually v2 of phpseclib and is PHP7
compliant. Most packages have moved forward to v2, but not all, in
Stretch, so the older package still exists.

So, if I understand the process correct: this phpseclib update can be
uploaded. It will get stuck in -proposed because php-horde-mapi will
regress its tests. Once we have a proper fix in php-horde-mapi, we can
SRU that back and both will go through. However, given the SRU
timelines, etc., it might be prudent to wait for the php-horde-mapi fix
to be available before SRU'ing either.

Note also there is a much smaller fix that we have pushed to
php-horde-mapi in 16.10, so that phpseclib could sync in 16.10, and that
is to just add a

Restrictions: allow-stderr

to debian/tests/control, so the deprecation warning is not treated as a
failure.

@nacc: Would you mind nominating bug #1593003 for Xenial? I'll work on SRUifying the bug description and uploading a debdiff that just adds the allow-stderr line to debian/tests/control.

On 27.06.2016 [23:49:54 -0000], Richard Hansen wrote:
> @nacc: Would you mind nominating bug #1593003 for Xenial? I'll work on
> SRUifying the bug description and uploading a debdiff that just adds the
> allow-stderr line to debian/tests/control.

Done! Thanks for the poke!

@Nish

Thank you for your patience in explaining this to me. That sounds fine then and your expectation was correct - I have no objection to uploading ahead of the dep8 fix in php-horde-mapi, though I've just done that now.

@Richard

Uploaded - thanks! Note that I ran "update-maintainer" (see https://wiki.ubuntu.com/DebianMaintainerField) as required. I'd say a more minimal diff would be to just comment out the line in debian/patches/series rather than remove the patch. This would make it easier for the SRU team to review. But in this case dropping the diff is simple enough to follow that I didn't bother about it.

Hello Paul, or anyone else affected,

Accepted phpseclib into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/phpseclib/1.0.1-3ubuntu0.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

1.0.1-3ubuntu0.1 works for me. Thank you!

I also confirm:

Get: 1 http://archive.ubuntu.com/ubuntu xenial-proposed/universe amd64 php-seclib all 1.0.1-3ubuntu0.1 [174 kB]

This was a fresh dokuiki install, so this should definitely get pushed out.

The verification of the Stable Release Update for phpseclib has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

This bug was fixed in the package phpseclib - 1.0.1-3ubuntu0.1

---------------
phpseclib (1.0.1-3ubuntu0.1) xenial; urgency=medium

  * Delete the patch "Fix "Methods with the same name as their class
    will not be constructors in a future version of PHP" that was
    added in 1.0.1-3. All of the changes in that patch are
    backwards-incompatible and break some applications that depend on
    this package (such as dokuwiki). (LP: #1574058)

 -- Richard Hansen <email address hidden> Thu, 23 Jun 2016 16:46:59 -0400