[PoC] Provide common make files #34660
Conversation
Codecov Report
@@ Coverage Diff @@
## master #34660 +/- ##
=========================================
Coverage 65.25% 65.25%
Complexity 18458 18458
=========================================
Files 1207 1207
Lines 69895 69895
Branches 1280 1280
=========================================
Hits 45608 45608
Misses 23915 23915
Partials 372 372
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #34660 +/- ##
============================================
+ Coverage 65.38% 65.38% +<.01%
Complexity 18592 18592
============================================
Files 1213 1213
Lines 70408 70408
Branches 1295 1295
============================================
+ Hits 46035 46036 +1
+ Misses 23999 23998 -1
Partials 374 374
Continue to review full report at Codecov.
|
|
Example PRs that pass CI using this code from core: Questions:
|
phil-davis
force-pushed
the
provide-common-makefiles
branch
2 times, most recently
from
ea2aa1a to
c485ae3
Compare
| CLEAN_HAS_BEEN_INCLUDED=true | ||
|
|
||
| .PHONY: clean-deps | ||
| clean-deps: ## Clean all dependencies |
There was a problem hiding this comment.
I think the clean rules need to be part of each module
the dist rules module has clean-dist, the npm deps has clean-npm-deps, etc
you can use a generic variable to which every module can add their clean rules to the main "clean" target.
see https://github.com/owncloud/customgroups/blob/master/Makefile#L25 and https://github.com/owncloud/customgroups/blob/master/rules/deps.mk#L16
|
don't think we need |
|
there is one drawback with this: app builds will now depend on the core git checkout that was used. which means that if we fix makefile rules in core we might need to do another release of apps. we'll need to settle on a single OC version when buildling apps then, we could say it always needs to be from the git checkout of the latest stable release. note that we had an internal POC from @patrickjahns where we build apps using a docker based on the OC-10.0.10 release. this approach would not work any more because the release tarball does not contain the makefile rules. might need a different approach then, like putting the makefile rules in a common repo like the coding standards one ? |
| vendor/bamarni/composer-bin-plugin: composer.lock | ||
| composer install | ||
|
|
||
| clean_deps_rules+=clean-vendor clean-vendor-bin |
|
ideas for later: a rules file with checkers. as this will be used by apps, we can check that many things are already properly in place in the dist tarball, like info.xml, changelog.md exists, etc |
If the problem caused an app tarball to be released with missing/wrong stuff, then fir sure the app has to be released again. But that is already the case if there is a problem in the local The default "recommended" action when making an app release tarball would be to have the end of the current core release branch checked out, e.g. core |
|
@PVince81 IMO this now matches the "standard" If we find we need more flexibility, then it is easy to add Or if the app really is "weird", then it does not have to include all the If we some day want to migrate to putting this in a separate repo, or use some completely different build tool, then that will be possible and should be easier because we have this common set of knowledge about what build and test services are commonly used by apps. Please review again. |
Would be better to decide this right away. We're going to need to touch every app already for this change to make sense. If later on we introduce a common location, we'd need to touch all apps yet again. |
|
OK, so how do we decide? |
|
Please do not make apps depend on core makefiles. If we are looking for a central part for makefiles, please put them in a dedicated repository to have clear separation of concerns. |
|
I removed the "To review" label. |
phil-davis
force-pushed
the
provide-common-makefiles
branch
from
aec125a to
4e5bb1d
Compare
phil-davis
force-pushed
the
provide-common-makefiles
branch
from
4e5bb1d to
41347fc
Compare
|
Closing see comment owncloud/richdocuments#253 (comment) |
See discussion below. This PR is being left here for now, until we get back to designing a better solution.
Description
Provide various
*.mkfiles in core that can be included by appMakefileso that chunks ofmakecode do not have to be repeated in every app.check-composer.mk- the common block of code that gives the warning if composer does not exist. This is included by other includes that care about composer.check-npm.mk- the common block of code that gives the warning if npm does not exist. This is included by other includes that care about npm.clean.mk- commoncleanandclean_depstargets. Othercleantargets are added by other include files.dist.mk- the "standard" way of making the tarball, and of doing aclean disthelp.mk- the "standard" way that help output is done these days in appstest-acceptance.mk- the make targets for acceptance tests and their dependency rulestest-js.mk- the JS testing make targets and their dependency rulestest-php.mk- the PHP coding standard, analysis and unit test make targets and their dependency rulestest-all.mk- a convenience that includes all 3test-*.mkfiles - so "standard" apps can just include this to get "the works" (now I am not so sure this is useful - not many apps have JS tests, some of those do or do not have acceptance tests,... so there is not really a "common" set of PHP+JS+acceptance tests)vendor-bin.mk- the dependency rules for getting thevendorandvendor-binstuff happening and cleaned. This is included by other includes that need it.Related Issue
owncloud/QA#604
Motivation and Context
How Has This Been Tested?
Local runs of JS unit, PHP unit and acceptance tests from various app repos.
PoC drone CI runs from activity, password_policy and... apps.
Types of changes
Checklist:
Open tasks: