Page MenuHomePhabricator

Running scap sync-dir php-1.32.0-wmf.10 fails due to syntax error
Closed, ResolvedPublic

Description

sync-dir recursively runs php -l on every file in the directory it's syncing, and also descends into the vendor directory. And apparently we have a file with invalid syntax there.

Fatal error: syntax error, unexpected T_CONST, expecting T_VARIABLE in /srv/mediawiki-staging/php-1.32.0-wmf.10/vendor/psy/psysh/test/ClassWithSecrets.php on line 16

This makes it impossible to use scap sync-dir php-1.32.0-wmf.10 as a workaround for scap sync when deploying a change that touches multiple top-level directories (e.g. includes/ and resources/) but doesn't otherwise need a full scap (doesn't touch i18n etc).

Event Timeline

Catrope created this task.Jun 28 2018, 9:02 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 28 2018, 9:02 PM
Krinkle added a subscriber: Krinkle.

Traced to https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/439955/ which first went out in wmf.10

The vendor path did have tests run with both hhvm and php70, but somehow didn't catch this and/or are less strict than the linter used by scap?

Krinkle triaged this task as Unbreak Now! priority.
Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptJun 28 2018, 9:27 PM
mmodell added a subscriber: mmodell.Jul 1 2018, 6:37 AM

scap lints every single file regardless of whether it would ever be executed by unit tests. I don't think CI is quite as strict with the way things are linted?

Krinkle added a comment.EditedJul 1 2018, 11:27 PM

On regular repos, the php linter makes no distinction between test files or src files. However, it is normal to exclude vendor/ given it would (at run-time in CI) containing a large number of files that would never be part of a prod deployment nor mediawiki-vendor (e.g. phpunit, codesniffer, etc.).

For the mediawiki-vendor repository itself, I would expect the linter to apply to all files given there is no sub folder called "vendor", and we actively to want to verify their syntax.

@mmodell Does scap currently have a list of files it excludes from linting?

The vendor path did have tests run with both hhvm and php70, but somehow didn't catch this and/or are less strict than the linter used by scap?

Scap shells out to php -l. php on deploy1001 is currently hhvm@3.18.5+dfsg-1+wmf8+deb9u1

Does scap currently have a list of files it excludes from linting?

Currently only autoload_static.php is excluded from linting. We don't have a list of files to exclude: https://github.com/wikimedia/scap/blob/master/scap/lint.py#L46

As far as what's linted, for a sync-file or sync-dir we lint everything under the code path. We don't do that for a full scap sync (which is a simple workaround for this).

Krinkle added a comment.EditedJul 9 2018, 9:04 PM

The vendor path did have tests run with both hhvm and php70, but somehow didn't catch this and/or are less strict than the linter used by scap?

Scap shells out to php -l. php on deploy1001 is currently hhvm@3.18.5+dfsg-1+wmf8+deb9u1

Does scap currently have a list of files it excludes from linting?

Currently only autoload_static.php is excluded from linting. We don't have a list of files to exclude: https://github.com/wikimedia/scap/blob/master/scap/lint.py#L46

Cool. The same file is also excluded by mediawiki-vendor's lint step:

https://gerrit.wikimedia.org/g/mediawiki/vendor/+/7a7dcec075661ae04c0b8b8e0eb459ff79412597/composer.json#145

However, this lint step also excludes more. Looking at each one to figure out the how and what.

  • --exclude composer/autoload_static.php:
    • What: This is a "production" file, committed to git.
    • Why: I do not know why this file would fail linting.
    • Scap: Also excluded it from linting, hence scap works fine.
  • --exclude jakub-onderka/php-parallel-lint/tests:
    • What: This is an development package (upstream), not committed to git.
    • Why: This directory contains example files, some of which are intentionally to test the linter.
    • Scap: (Not relevant, file only exists during the Jenkins job).
  • --exclude symfony/var-dumper/Tests/Fixtures
    • What: This is a unit test file (upstream) from a "production" package, committed to git.
    • Why: One of the test fixtures tests the PHP 7 "generators" feature. While the package supports PHP 5, it also supports PHP 7, which is why its upstream tests do contain PHP 7 files. These are not used by us, and given we require PHP 5 linting to pass for third-parties, they are excluded from PHP5 linting.
    • Scap: Not excluded, but seems to work because we are using HHVM/PHP7, which support this feature.

And, as of last week:

  • --exclude psy/psysh/test
    • What: This is a unit test file (upstream) from a "production" package, committed to git.
    • Why: It is a test fixture confirming support for the PHP 7 "private constants" feature.
    • Scap: Not excluded, but should be, because this PHP7 feature is not supported by HHVM.

Perhaps, rather than requiring Scap to be updated, we can add psysh/test (and a few others) to mediawiki-vendor's gitignore file. That way, they naturally don't end up in production for Scap to see.

It seems we already do this for a few other test directories:

https://gerrit.wikimedia.org/g/mediawiki/vendor/+/7a7dcec075661ae04c0b8b8e0eb459ff79412597/.gitignore#39

thcipriani lowered the priority of this task from Unbreak Now! to High.Jul 17 2018, 6:25 PM

This obviously isn't UBN! since it's not blocking anyone.

Perhaps, rather than requiring Scap to be updated, we can add psysh/test (and a few others) to mediawiki-vendor's gitignore file. That way, they naturally don't end up in production for Scap to see.

It seems we already do this for a few other test directories:

https://gerrit.wikimedia.org/g/mediawiki/vendor/+/7a7dcec075661ae04c0b8b8e0eb459ff79412597/.gitignore#39

No work is planned on the Scap side as this seem reasonable to me.

greg edited projects, added MediaWiki-Vendor; removed Deployments.Oct 3 2018, 7:03 PM
greg added a subscriber: greg.

Perhaps, rather than requiring Scap to be updated, we can add psysh/test (and a few others) to mediawiki-vendor's gitignore file. That way, they naturally don't end up in production for Scap to see.

greg closed this task as Resolved.Jan 23 2019, 4:25 PM
greg claimed this task.

Perhaps, rather than requiring Scap to be updated, we can add psysh/test (and a few others) to mediawiki-vendor's gitignore file. That way, they naturally don't end up in production for Scap to see.

Looks like all the non symphony ones here have been added since this task was created.
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/vendor/+/master/.gitignore#39

Resolving.

Restricted Application added a project: User-greg. · View Herald TranscriptJan 23 2019, 4:25 PM