-
Notifications
You must be signed in to change notification settings - Fork 484
fix(debian9): add compatibility with debian9 #857
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
Conversation
Some issues exist with permissions on debian 8 with this PR, which is weird because I didn't even touch any file permissions |
4123bb9
to
bcf897f
Compare
|
||
#### fossology needs up to 15 seconds to startup | ||
sleep 15 | ||
sleep 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update or remove the comment above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test default-docker-test.sh
does no longer test whether fossology is running.
|
||
#### is fossology reachable? --> check title | ||
curl -L -s http://172.18.0.22/repo/ | grep -q "<title>Getting Started with FOSSology</title>" | ||
curl -v -L -s http://172.18.0.22 | grep -q "<title>Apache2 Debian Default Page: It works</title>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is no longer checking that fossology is running or responding! Please add also the old test back in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find. I just disabled that temporary for testing, but nearly forgot to add it back in. Thanks!
90b226c
to
8e13de7
Compare
jessie) apt-get $YesOpt install postgresql-server-dev-9.4;; | ||
xenial) apt-get $YesOpt install postgresql-server-dev-9.5;; | ||
xenial|yakkety) apt-get $YesOpt install postgresql-server-dev-9.5;; | ||
stretch|buster|sid|zesty|artful) apt-get $YesOpt install postgresql-server-dev-9.6;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the moving target sid it might be good to reference a package without a version, e.g. that will bring hidden problems. A failing postgresql-server-dev-all
.apt-get install
is the better choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's okay, if I understand that correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will break soon and is the same problem as in #864. Maybe it is a good idea to remove the sid
option but I am not sure.
=> It is OK until someone finds a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it is reasonable to remove 'sid' support - as a bleeding edge unstable 'rolling' distro, much like arch, users are expected to "know what they are doing" - and for the same reason arch support was deemed to be not a priority, it is probably an extremely small use-case that anyone is running a server on 'sid' - the docker image is still there for those few
but on that note the 'buster' distro is just as volatile trailing only a few weeks behind 'sid' so they should stay or go together - but the main thing is that this is not going to "break" until v9.7 comes out - which will not exactly be broken - it is just adding yet another version to the stack as you see has been done several times already - and regardless of debian, this would still be the case for the next bi-annual release of ubuntu that includes v9.7
practically speaking, ubuntu is actually the more troublesome target than debian because it is guaranteed to requires a routine update to these install files every 6 months - so to be consistent with this decision, note that dropping support for 'buster' and 'sid' and supporting only the LTS releases of debian is the equivalent to only supporting ubuntu LTS every 2 years and not supporting any of the interim versions as has been done all along
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be clear, the point i raised was that ubuntu support is actually more work to maintain than debian because it is guaranteed to requires a routine update to these install files every 6 months - even if no dependencies change, a new ubuntu entry would need to be added every 6 months - that would not be the case for debian - unless dependencies change, a new debian entry would not need to be added until the next debian LTS version in 2-3 years - if every ubuntu version is to be supported, then keeping 'buster' and 'sid' will be exactly zero additional work because whatever dependency versions ubuntu requires, they take from what is current in debian testing (currently buster)
But Ubuntu will at least not break in the future. Unless someone of the developers uses buster or sid, we will not recognize when the dependencies break on these systems.
And AFAIK we can not test the dependencies with travis (unless we set up docker files for that)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wel as i said it will not "break" - it will just require a new version of the PG libs - and that WILL happen - and it will happen in buster and ubuntu at the same time (within a few months)
you could use debootstrap (or several other tools) to test the debian builds on travis - but yea it's probably more trouble than it's worth - to be consistent then i would suggest to drop support for the non-LTS versions of ubuntu because it is no easier to support those on travis right?
also as i mention on my original PR - the entire hassle of version numbers could be avoided by requiring the metapackages like:
postgresql-server-dev-all
instead of
postgresql-server-dev-9.5
or
postgresql-server-dev-9.6
postgresql php php-pgsql php-cli php-curl php-xml
instead of
postgresql-9.5 libapache2-mod-php5 php5 php5-pgsql php5-cli php5-curl
or
postgresql-9.6 libapache2-mod-php7.0 php7.0 php7.0-pgsql php7.0-cli php7.0-curl php7.0-xml;;
just some ideas to lessen the maintenance load - fwiw - this is a nice feature but very few upstreams manage a script like this - they usually just post the dependencies in the README and let it to the user to prepare the environment and a ./configure script to verify it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using metapackages leads to other problems. We explicitly use specific php and psql versions since we do not want to support all of them.
In my opinion, if one tries to install fossology on a not supported system, one should fail early in the install-deps-step instead of silently later, when some features in the application break.
+1 for the idea with the ./configure
step for verifying dependencies.
src/www/ui/index.php
Outdated
plugin_unload(); | ||
|
||
$container->get("db.manager")->flushStats(); | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary change.
|
||
$container->get('db.manager')->setDriver(new SqliteE($sqlite3Connection)); | ||
$liteDb = new SqliteE($sqlite3Connection); | ||
$container->get('db.manager')->setDriver($liteDb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this reformatting necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because Only variables should be passed by reference
starting with PHP 7.0
$container->get('db.manager')->setDriver(new Postgres($this->connection)); | ||
$this->dbManager = $container->get('db.manager'); | ||
$postgres = new Postgres($this->connection); | ||
$this->dbManager->setDriver($postgres); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this reformatting necessary? $this->dbManager
gets overwritten in the next line (with the same object)!
src/composer.json
Outdated
"twig/twig": "1.*", | ||
"twig/extensions": "1.*" | ||
"twig/extensions": "1.*", | ||
"phpunit/phpunit": "~5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to make phpunit a non-dev-dependency?
8314497
to
d40b941
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the support for buster and sid (as discussed in #857 (comment))
to be clear, the point i raised was that ubuntu support is actually more work to maintain than debian because it is guaranteed to requires a routine update to these install files every 6 months - even if no dependencies change, a new ubuntu entry would need to be added every 6 months - that would not be the case for debian - unless dependencies change, a new debian entry would not need to be added until the next debian LTS version in 2-3 years - if every ubuntu version is to be supported, then keeping 'buster' and 'sid' will be exactly zero additional work because whatever dependency versions ubuntu requires, they take from what is current in debian testing (currently buster) |
(from the discussion): add agent tests for PHP 7.0 and 7.1 |
60b1157
to
a4af5cc
Compare
apt-get $YesOpt install postgresql-9.5;; | ||
apt-get $YesOpt install postgresql-9.4 libapache2-mod-php5 php5 php5-pgsql php5-cli php5-curl;; | ||
xenial|yakkety) | ||
apt-get $YesOpt install postgresql-9.5 libapache2-mod-php5 php5 php5-pgsql php5-cli php5-curl;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the '5' from php packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keestux - to be clear, when you say drop support for php5, what you are really suggesting is to drop support for: squeeze, wheezy, lucid, natty, isadora, precise, oneiric, quantal, raring, saucy, trusty, jessie, xenial, yakkety
if you are only suggesting to drop the number '5' and require the meta-packages php, php-pgsql, php-cli, php-curl, etc. instead of php,5 php5-pgsql, php5-cli, php5-curl - that has already been suggested in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. I just got confused by Github which showed commit a4af5cc, but which is not present in the fresh clone that I made.
a4af5cc
to
68f5eb5
Compare
* debian stretch,buster,sid * ubuntu yakkety,zesty,artful
68f5eb5
to
c1f4cdb
Compare
tested with debian 9 (net install) -> OK tested with ubuntu 16.04.03 ->
to get the make install running.
(package postgresql-server-dev-9.5 is installed and db is running) |
Maybe also works for ubuntu 16.04+, but not tested!