silverstripe / silverstripe-travis-support

Creates a SilverStripe project "around" a module, based on core version constraints and its composer.json definitions
Other
13 stars 16 forks source link

Use prefer-stable for dependencies #50

Closed kinglozzer closed 7 years ago

kinglozzer commented 7 years ago

N.b. it’s 5:30 on Friday and I’ve not thought through implications of this, but it solves the Behat issues in 3.x builds:

https://travis-ci.org/silverstripe/silverstripe-framework/jobs/306861877#L503

mink-selenium2-driver requires "instaclick/php-webdriver": "~1.1", but we were getting dev-master (presumably because of the missing prefer-stable, and that their branch alias needs updating - https://github.com/instaclick/php-webdriver/issues/82).

dhensby commented 7 years ago

Tests need updating

kinglozzer commented 7 years ago

Done 😄 can’t squash because git is segfaulting on my laptop 😳

UndefinedOffset commented 7 years ago

So this pull seems to be causing SS 4 builds to fail because the 2.0.0 version of silverstripe/sqlite3 does not work with SilverStripe 4 (no namespaces) see https://travis-ci.org/UndefinedOffset/SortableGridField/jobs/306887354 that whole build failed for the same reason.

Also maybe unrelated but it seems to be taking along time at "Updating dependencies (including require-dev)"

kinglozzer commented 7 years ago

@UndefinedOffset Unless I’m mistaken (perhaps @dhensby or @tractorcow can confirm), the recommendation is that modules simply don’t use travis-support for 4.x. It should be pretty easy migration, for my modules (example) I copied the approaches in .travis.yml for core modules.

We could probably fix the sqlite3 issue, though. It sounds like we just need a new stable tag, do you know if the sqlite3 module is ready to be tagged @dhensby?

dhensby commented 7 years ago

I'm confused - if there's no stable version of the sqlite3 module that works with SS4 then you shouldn't be requiring a stable version of it in your constraints... or is it travis-support that is requiring it?

As @kinglozzer you shouldn't really be using travis-support for SS 4 as you can now install framework/cms/etc as modules inside your module (ie: you can run composer install inside your module and run tests like that).

Having said that, if we can fix it so that it works, I'd like to...

kinglozzer commented 7 years ago

@dhensby Just had a look, travis-support does require it (and pgsql too): https://github.com/silverstripe/silverstripe-travis-support/blob/master/src/ComposerGenerator.php#L273

UndefinedOffset commented 7 years ago

Travis support is doing that requirement, as for not using this for SS4 I wonder if we should put a note in the readme about that. Or some way of causing a build failure if you do use it.

dhensby commented 7 years ago

I am some what happy to revert this as the upstream fix means 3.x builds will pass without this PR

kinglozzer commented 7 years ago

I am some what happy to revert this as the upstream fix means 3.x builds will pass without this PR

That’s true. It does leave our builds slightly fragile to similar issues in future, but if we’re effectively deprecating the use of this module then I think that’s acceptable

dhensby commented 7 years ago

ok - the choices are:

  1. Revert this PR
  2. Release a stable tag of sqlite3
  3. Leave it "broken" for SS 4 - ie: advocate not using it

I think 2 is a no go as it's not rested enough yet. 3 is a bit too dogmatic and means unnecessary pain for module developers... so I'm leaning towards 1

kinglozzer commented 7 years ago

Would tagging a beta release of sqlite3 work for now? The job @UndefinedOffset linked appears to have picked a beta tag for postgres

dhensby commented 7 years ago

No, there's already an alpha 2.1.0 tag, the 2.0.0 tag was a bit premature, it seems - as it was for an earlier version of 4.0...

UndefinedOffset commented 7 years ago

there sortablegridfield is no longer using this ;)

kinglozzer commented 7 years ago

I think we’ll revert this anyway just to be safe for other modules 😄

kinglozzer commented 7 years ago

@dhensby Now that PostgreSQL and SQLite3 have new stable tags we could revert the revert... thoughts?

dhensby commented 7 years ago

I don't feel strongly one way or the other.

On one hand using stable dependencies should allow us to have more stable test builds (because hopefully no one's tagged a buggy release) but we'll also miss any integration issues with current dev builds of dependencies which may be important...

I'm leaning towards keeping it as is, but if you feel it's better to have stable dependencies then I'm happy to revert the revert

dhensby commented 7 years ago

maybe we get some thoughts from @tractorcow ?

UndefinedOffset commented 7 years ago

I wonder if we could add an option to the travis-setup.php as an opt-in since this didn't do it before?

dhensby commented 7 years ago

I wonder if we could add an option to the travis-setup.php as an opt-in since this didn't do it before?

That is an option, but it would be one that isn't adopted by very many

UndefinedOffset commented 7 years ago

True could do the opposite then, have an opt-out. That way modules/repos using this could opt-out if requiring stable is causing issues.

dhensby commented 7 years ago

yep, that's a good point

tractorcow commented 7 years ago

I highly want to deprecate travis-support for 4.0, but if you really want to use it with your project it should be installable.

I would modify this behaviour to use an environment variable to determine if prefer_stable is true, and assume a default false (legacy) for CORE_RELEASE=3.* in order to prevent breaking any legacy builds. 3.x have enough issues as it is without new behaviour breaking already fragile builds. :)

dhensby commented 7 years ago

ok - I vote for leaving it as is and don't bother with complicating the code (unless someone else actually wants to invest the effort in this)

kinglozzer commented 7 years ago

Agreed, let’s leave it. We’ve not had prefer-stable for years and it has only caused 1 issue, so it’s definitely not worth the time investment if we’re deprecating travis-support