fix(make): do not place composer at `/tmp/composer/composer` #738

maxhbr · 2017-02-01T13:03:57Z

this pull request

undos the changes to the composer placement introduced in pkg: first import of a pbconf tree #701
adds the option to pass the environmental variable COMPOSER_PHAR to
src/Makefile to allow placing the composer.phar at arbitrary places
this fixes the source installation, i.e. the documentation was not up
to date
this fixes the problems with vagrant up (see docker-compose up fails to build fossology-scheduler #732)
this fixes the problems with docker-compose build

as discussed in #731

closes #732

maxhbr · 2017-02-01T13:06:51Z

@bcornec does the commit 011ffe3 fix the composer problem in the packaging context?

bcornec · 2017-02-01T14:55:13Z

src/Makefile

-	mkdir -p $(COMPOSERTMP)
-	mv composer.phar $(COMPOSERTMP)/composer
+	composer install -q



Problem is that we're loosing the 2 steps approach here (download target and usage target)

Download target should be invoked during an environment preparation phase, not during build. Usage when we need it during build.

Also using a full path name for calling composer is not necessarily an issue, (instead of using the PATH which can be wrong) as long as we don't use something controversial such as /tmp/something. It could have been stored in the fossology tree as well, but I didn't found a variable describing it in that Makefile, so that's why I didn't use it.

Ok, that's a problem of the wrapper script. A correct altenrative would be to download composer to some temporary path (e.g. tmp/composer/) and later in the next step all the make command with the correct path (e.g. PATH='tmp/composer/:$PATH make ....).

Does this fit your 2 step approach?

bcornec

I don't think that going back that way will solve all the problems (at least not the ones for building)

bcornec · 2017-02-01T14:56:35Z

utils/run_with_composer_in_path.sh

+    [[ -e "$tmp_composer" ]] || {
+        curl -sS https://getcomposer.org/installer \
+            | php && mv composer.phar "$tmp_composer"
+    }


So I think we're back to what existed initially, and the problem related to that approach: there is no split between the download of composer and its usage again.

You could split it up again (instead of using the wrapper) as discussed in the comment above

bcornec · 2017-02-01T15:24:59Z

I think there are coherency issues that need to be solved:

You have a Vagrant file that does:
curl -sS https://getcomposer.org/installer | php
sudo mv composer.phar /usr/bin/composer

You have a Dockerfile that does:
RUN curl -sS https://getcomposer.org/installer | php &&
mkdir /tmp/composer &&
mv composer.phar /tmp/composer/composer

You have a Makefile that does:
composer_download:
curl -sS https://getcomposer.org/installer | php
mkdir -p $(COMPOSERTMP)
mv composer.phar $(COMPOSERTMP)/composer

So we have 3 ways of dealing with composer, whereas a single one should be used everywhere. I'll propose a patch for this as I think that's what is creating the issues reported.

maxhbr · 2017-02-01T16:13:34Z

In this PR all three cases are consistend and match the first version (AFAIK). Plus you have the option to implement packaging without breaking changes using $PATH.

An alternative would be to introduce a new environmental variable which can be used to define the position of composer. Would it be an option to call make with

COMPOSER=/tmp/composer/composer make composer_install

and within the makefile use something like ${COMPOSER:-composer} ??

maxhbr · 2017-02-01T16:20:34Z

The (~~untested~~ wrong, but still a good discussion starting point) changes in b84af50 might allow you to run the makefile with

make COMPOSER=/tmp/composer/composer composer_install

is that ok?

bcornec · 2017-02-01T16:37:40Z

No the calls to download composer are all different in that they all place the resulting file in different places, creating a mess when you need to call the tool afterwards through a single call !

The problem is splitting download from usage, not the Variable.
Look at #739 to see what I mean.

maxhbr · 2017-02-01T16:40:38Z

But the download is splitted. The makefiles do never download composer. For that one either calls fo-instaldepts or installs it by hand.

In your example of the mess are still two of three variants introduced by your movement of composer to /tmp/composer

maxhbr · 2017-02-01T17:13:00Z

@bcornec With 3735e45 the idea suggested in the comment above is now working and one is able to call make in the following way (after downloading composer.phar to /tmp/composer/composer.phar)

make COMPOSER_PHAR=/tmp/composer/composer phpvendors
#or
make COMPOSER_PHAR=/tmp/composer/composer install

This allows you to download composer at an arbitrary timepoint and in an arbitrary way, without introducing braking changes to other parts of the code.

note: I was not able to find the code in pbconf/fossology/rpm/fossology.spec which takes care of the download of composer.

maxhbr · 2017-02-01T17:28:01Z

one might want to replace the line 76ca5c2#diff-2f7bc9d6d5478304df37e9900a6e32f2R35 (and other lines which download composer) by an script

maxhbr · 2017-02-08T07:31:37Z

How to install composer programmatically is described here: https://getcomposer.org/doc/faqs/how-to-install-composer-programmatically.md (under MIT)

maxhbr · 2017-02-08T10:50:13Z

This PR now contains a script to install composer

of an specified version
from an trustworthy source (i.e. github, with specified commit hash)

maxhbr · 2017-03-01T12:49:04Z

@mcjaeger This PR is now rebased to current master

this commit - undos the changes to the composer placement introduced in fossology#701 - adds the option to pass the environmental variable `COMPOSER_PHAR` to `src/Makefile` to allow placing the composer.phar at arbitrary places - this fixes the source installation, i.e. the documentation was not up to date - this fixes the problems with `vagrant up` (see fossology#732) - this fixes the problems with `docker-compose build` as discussed in fossology#731 closes fossology#732

- add script to install fixed composer version - default to composer in $PATH

mcjaeger · 2017-03-06T23:11:05Z

actually it seems like the fo-apache.conf is not copied anymore, but that is another issue as this seems to be also in master

maxhbr mentioned this pull request Feb 1, 2017

fix(Dockerfile): solve composer usage #731

Closed

maxhbr requested a review from bcornec February 1, 2017 13:08

bcornec reviewed Feb 1, 2017

View reviewed changes

bcornec mentioned this pull request Feb 1, 2017

Improve composer handling #739

Closed

maxhbr force-pushed the fix/composerBinaryPlacement branch 2 times, most recently from 5d9f156 to 3735e45 Compare February 1, 2017 17:12

maxhbr force-pushed the fix/composerBinaryPlacement branch from 3735e45 to 76ca5c2 Compare February 1, 2017 17:27

maxhbr force-pushed the fix/composerBinaryPlacement branch 3 times, most recently from 9b273fb to 7e73cfd Compare February 8, 2017 14:54

maxhbr mentioned this pull request Feb 28, 2017

feat(packaging): updating existing debian packaging for current fossology #750

Merged

maxhbr force-pushed the fix/composerBinaryPlacement branch from 7e73cfd to 52ec230 Compare March 1, 2017 12:03

maxhbr added 2 commits March 2, 2017 17:19

chore(composer): refactor docker installation and placement

ae02b42

- add script to install fixed composer version - default to composer in $PATH

maxhbr force-pushed the fix/composerBinaryPlacement branch from 52ec230 to ae02b42 Compare March 2, 2017 16:19

mcjaeger merged commit 42c08f4 into fossology:master Mar 6, 2017

mcjaeger mentioned this pull request Mar 7, 2017

Source Code install of 3.1.0-rc2 on Debian 8 fails for composer error #690

Closed

fix(make): do not place composer at /tmp/composer/composer #738

fix(make): do not place composer at /tmp/composer/composer #738

Uh oh!

Conversation

maxhbr commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxhbr commented Feb 1, 2017

Uh oh!

bcornec Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

maxhbr Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

bcornec left a comment

Choose a reason for hiding this comment

Uh oh!

bcornec Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

maxhbr Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcornec commented Feb 1, 2017

Uh oh!

maxhbr commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxhbr commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcornec commented Feb 1, 2017

Uh oh!

maxhbr commented Feb 1, 2017

Uh oh!

maxhbr commented Feb 1, 2017

Uh oh!

maxhbr commented Feb 1, 2017

Uh oh!

maxhbr commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxhbr commented Feb 8, 2017

Uh oh!

maxhbr commented Mar 1, 2017

Uh oh!

mcjaeger commented Mar 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(make): do not place composer at `/tmp/composer/composer` #738

fix(make): do not place composer at `/tmp/composer/composer` #738

maxhbr commented Feb 1, 2017 •

edited

Loading

maxhbr Feb 1, 2017 •

edited

Loading

maxhbr commented Feb 1, 2017 •

edited

Loading

maxhbr commented Feb 1, 2017 •

edited

Loading

maxhbr commented Feb 8, 2017 •

edited

Loading