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

Update Travis file and fix build #86

Conversation

Potherca
Copy link
Member

@Potherca Potherca commented Sep 11, 2019

Proposed Changes

Currently the Travis build is breaking because the URL used by the security-checker no longer exists.
This MR updates that URL to the new home in order to fix the build.

It also adds a preview for PHP 7.4 and makes minor changes to improve the build step.

@Potherca
Copy link
Member Author

@mjrider, can you review (and optionally merge) this, if @frenck is unavailable?

@Potherca Potherca force-pushed the ci-build/update-security-checker branch from 2e65b1d to 49e4850 Compare September 11, 2019 18:54
@Potherca
Copy link
Member Author

Looks like this fixes part of the issue... PHP5.4 and PHP5.5 need Trusty to build.

However, changing the matrix to this:

matrix:
  include:
    - php: 7.2
      env: SNIFF=1
    - php: 5.5
      # As the latest Debian not supporting PHP 5.5 anymore, we need to force using 'trusty'.
      dist: trusty
    - php: 5.4
      # As the latest Debian not supporting PHP 5.4 anymore, we need to force using 'trusty'.
      dist: trusty
    - php: 5.3
      # As the latest Debian not supporting PHP 5.3 anymore, we need to force using 'precise'.
      dist: precise

did not yield the expected result (i.e. Trusty being used...)

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Some suggestions:

  1. As this repo does not have a committed composer.lock file, instead of using the sensiolabs/security-checker as you are now, you could replace that Composer dependency with roave/security-advisories.
    That way, the check will still be executed, but will be done during the composer install step and it would remove your dependency on an outside URL during your builds.
  2. Adding dist: trusty at the top of the script (and removing sudo: false which is no longer supported) should fix the build failures for PHP 5.4 and 5.5.

@jrfnl
Copy link
Member

jrfnl commented Sep 11, 2019

Oh and as you're making changes to the Travis script now anyway, why not add a build against "7.4snapshot", possibly with an allow_failures for it too ?

@mjrider mjrider self-assigned this Sep 12, 2019
@Potherca Potherca force-pushed the ci-build/update-security-checker branch from f89fa5a to f55f292 Compare September 15, 2019 15:31
@Potherca Potherca force-pushed the ci-build/update-security-checker branch 2 times, most recently from 4e00d60 to bfc7155 Compare September 15, 2019 16:57
- Only run secondary checks once (on latest PHP version)
@Potherca Potherca force-pushed the ci-build/update-security-checker branch from bfc7155 to 12118a3 Compare September 15, 2019 17:05
@Potherca Potherca changed the title Update security-checker to new URL to fix build. Update Travis file and fix build Sep 15, 2019
@Potherca
Copy link
Member Author

@mjrider I'm all done. Can you please review/approve so these changes can be merged to master?

They are needed before I can update the other open MRs.

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
Special consideration for the php 5.3 build, its 🤪
Copy link
Contributor

@mjrider mjrider left a comment

Choose a reason for hiding this comment

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

image

@mjrider mjrider merged commit dda5b1d into PHPCSStandards:master Sep 20, 2019
@Potherca Potherca deleted the ci-build/update-security-checker branch August 19, 2020 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants