-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Build phar with box and bump dependency versions #372
Conversation
@@ -1,5 +1,7 @@ | |||
<?xml version="1.0"?> | |||
<phpunit bootstrap="src/test/php/PDepend/bootstrap.php"> | |||
<phpunit bootstrap="src/test/php/PDepend/bootstrap.php" | |||
beStrictAboutTestsThatDoNotTestAnything="false" |
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.
There's loads of tests with no assertions, but one thing at a time.
Codecov Report
@@ Coverage Diff @@
## master #372 +/- ##
============================================
+ Coverage 87.88% 89.24% +1.35%
Complexity 3105 3105
============================================
Files 202 202
Lines 8939 8728 -211
============================================
- Hits 7856 7789 -67
+ Misses 1083 939 -144
Continue to review full report at Codecov.
|
default: build | ||
|
||
build: install test | ||
.PHONY: build |
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.
I think it's more standard to put the phone before, but that's just CS anyway...
ln -sf ../vendor/bin/phpunit tools/phpunit | ||
|
||
tools/box: | ||
curl -Ls https://github.com/humbug/box/releases/download/3.7.0/box.phar -o tools/box && chmod +x tools/box |
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 those targets I would add touch $@
to avoid the target to get outdated. You sometimes have cases where the target timestamp is not updated properly otherwise making the whole Makefile usage more flimsy.
This comment applies to the other Makefile targets
"base-path": "build/phar", | ||
"output": "../pdepend.phar", | ||
"compression": "GZ", | ||
"directories": ["src/bin", "src/main", "vendor"], |
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.
note that this could be improve (but I would suggest to do that in another PR)
cp -r src/bin src/main build/phar/src | ||
cp -r composer.json LICENSE build/phar | ||
|
||
cd build/phar && composer config platform.php 7.1 && composer update --no-dev -o -a |
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.
I think the platform needs to be a specific version, e.g. 7.1.3
. -o -a
are overkill since -a
implies -o
and Box will take care of dumping the autoloader in authoritative mode anyway (so doing it here results in the autoloader being dumped twice)
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.
I changed settings to use environment variable instead of hard coded tokens, then resolved the conflict on .travis.yml
I think we should even pick PHP 7.2 as minimum level of the next major PDepend, like for PHPMD, and this could start as soon as version 2 with PHP 5.3 would support properly PHP 7.0 to 7.3. Seems more logical to me to first fully support a PHP level before jumping to it. That's why I switched the base branch to 3.x but it's not engraved in stone, we can debate.
"symfony/dependency-injection": "^2.3.0|^3|^4", | ||
"symfony/filesystem": "^2.3.0|^3|^4", | ||
"symfony/config": "^2.3.0|^3|^4" | ||
"php": "^7.1,<7.4", |
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.
PHPMD 3 will be ^7.2, I think we should align. And I think we should also target PHP 7.4 for the next major release of PDepend.
}, | ||
"require-dev": { | ||
"phpunit/phpunit": "^4.8|^5.7", | ||
"squizlabs/php_codesniffer": "^2.0.0" | ||
"phpunit/phpunit": "^7.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.
If we PHP ^7.2, PHPUnit can be ^8.0
re #371 #364
Advantages of dropping legacy PHP versions:
Advantages of box:
Disadvantages of box: