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

Makefile composer #34630

Merged
merged 3 commits into from
Feb 27, 2019
Merged

Makefile composer #34630

merged 3 commits into from
Feb 27, 2019

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Feb 27, 2019

Description

  • modify Makefile so it expects and finds a pre-installed composer on the system, rather than downloading a copy.
  • modify .drone.yml so it no longer tries to self-install composer

This makes composer-related stuff work like it does in the app repos.

This is a resolved version of PR #32213 and moves forward from that.

Related Issue

owncloud/QA#582

Motivation and Context

Have a consistent CI and development environment in core as in apps.

How Has This Been Tested?

Local

make clean
make
make test-php-style
make test-php

to see that these still work locally.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes) (and CI scripting)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@@ -61,7 +61,7 @@ pipeline:
image: owncloudci/php:${PHP_VERSION}
pull: true
commands:
- ./tests/drone/composer-install.sh
- make install-composer-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

somehow I had the feeling that all drone scripts were avoiding the Makefile for some reason... not sure if it's good to add it back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In recent times we have been using the make targets directly in the drone scripts - e.g. make test-acceptance-api etc. This has happened in both core and apps.

@@ -38,7 +41,7 @@ YARN := $(shell command -v yarn 2> /dev/null)
KARMA=$(NODE_PREFIX)/node_modules/.bin/karma
JSDOC=$(NODE_PREFIX)/node_modules/.bin/jsdoc
PHPUNIT="$(shell pwd)/lib/composer/phpunit/phpunit/phpunit"
COMPOSER_BIN=build/composer.phar
COMPOSER_BIN := $(shell command -v composer 2> /dev/null)
Copy link
Contributor

Choose a reason for hiding this comment

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

what if there is no composer ? when to show a warning ?

note that in some build paths like JS tests no composer is required for Drone, so if a warning is shown it must only apply whenever composer is really needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add the "standard" code block that has been added in app make files:

ifndef COMPOSER_BIN
    $(error composer is not available on your system, please install composer)
endif

We never made it smart in the app make files! It is likely emitting this warning in some drone build jobs that do not have composer in their docker, and also do not need it.

I will think about how to make it smarter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at drone. It has a composer: pipeline step that uses image: owncloudci/php which has composer available and works.
Javascript runs test-javascript.sh and does not go via Makefile so it does not notice if composer is there or not. In any case it uses the same docker image that has composer in it.

phan phpstan owncloud-coding-standard use make commands. They use php and composer anyway and have that docker images also.

So everything relevant in drone has composer already installed.

I have added the message block in my post above ^
That will only appear when a developer first does some make command and they do not have composer installed on their system. They should install it ;)

@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #34630 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34630      +/-   ##
============================================
+ Coverage     65.23%   65.23%   +<.01%     
  Complexity    18438    18438              
============================================
  Files          1203     1203              
  Lines         69825    69825              
  Branches       1280     1280              
============================================
+ Hits          45548    45549       +1     
+ Misses        23905    23904       -1     
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 53.04% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.63% <ø> (ø) 18438 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
lib/private/Server.php 86.68% <0%> (+0.11%) 253% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a9faa...205c511. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #34630 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34630      +/-   ##
============================================
+ Coverage     65.23%   65.23%   +<.01%     
  Complexity    18438    18438              
============================================
  Files          1203     1203              
  Lines         69825    69825              
  Branches       1280     1280              
============================================
+ Hits          45548    45549       +1     
+ Misses        23905    23904       -1     
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 53.04% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.63% <ø> (ø) 18438 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
lib/private/Server.php 86.68% <0%> (+0.11%) 253% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a9faa...205c511. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #34630 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #34630   +/-   ##
=========================================
  Coverage     65.23%   65.23%           
  Complexity    18438    18438           
=========================================
  Files          1203     1203           
  Lines         69825    69825           
  Branches       1280     1280           
=========================================
  Hits          45548    45548           
  Misses        23905    23905           
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 53.04% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.63% <ø> (ø) 18438 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
apps/files_trashbin/lib/Expiration.php 96.55% <0%> (-1.73%) 29% <0%> (ø)
lib/private/Server.php 86.68% <0%> (+0.11%) 253% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a9faa...efcbf86. Read the comment docs.

@phil-davis
Copy link
Contributor Author

@PVince81 @individual-it @patrickjahns ready for review again

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

fine 👍

@PVince81 PVince81 merged commit 3b6332f into master Feb 27, 2019
@PVince81 PVince81 deleted the makefile-composer branch February 27, 2019 15:40
@PVince81
Copy link
Contributor

@phil-davis please backport

This was referenced Feb 27, 2019
@phil-davis
Copy link
Contributor Author

Backport stable10 #34637

@lock lock bot locked as resolved and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants