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

Bye bye submodule, hello Makefile! #25513

Merged
merged 36 commits into from Aug 29, 2016
Merged

Bye bye submodule, hello Makefile! #25513

merged 36 commits into from Aug 29, 2016

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Jul 18, 2016

@DeepDiver1975 DeepDiver1975 added this to the 9.2 milestone Jul 18, 2016
@PVince81
Copy link
Contributor

Did a quick test and it seems to work fine.
Also tested SFTP since it uses phpseclib from that bunch, also worked fine.

@PVince81
Copy link
Contributor

@DeepDiver1975 updated guzzle b2906fe so we don't have to once #25525 is merged. Please verify.

@@ -6,6 +6,7 @@
/config/mount.php
/apps/inc.php
/assets
/composer.phar
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it common to have it directly under the git root ?

Copy link
Member Author

Choose a reason for hiding this comment

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

we can also put it into the build folder ...

@PVince81
Copy link
Contributor

Tested, works.

My only concern is about bootstrapping the composer phar file directly into the git root. Would be better to have it into an excluded subdirectory or so, keeping in mind that we might want to bootstrap other tools as well (PHPUnit ?)

@PVince81
Copy link
Contributor

/me now eager to get this merged so we can improve the makefile

@DeepDiver1975
Copy link
Member Author

tools as well (PHPUnit ?)

we will grab phpunit via composer - that way we have one explicit version in every branch - much easier compared to the kung-fu we use in autotest.sh. in addition phpunit 5.*+ require php 5.6+ - we need to user different versions anyhow .....

all: install-composer-deps

composer.phar:
cd build && curl -sS https://getcomposer.org/installer | php
Copy link
Member Author

Choose a reason for hiding this comment

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

@PVince81 composer.phar is now in build sub folder

@PVince81
Copy link
Contributor

@DeepDiver1975 this also means that switching between stable branches might require the dev to rerun composer (assuming all branches have it at some point). Would running composer update or so be enough to make it adjust lib versions even if there's a downgrade ? (ex: switching from stable11 to stable10)

It also means that CI will have to fetch all dependencies for every run. I guess there will be a repository cache for composer packages somewhere on Jenkins ?

@DeepDiver1975
Copy link
Member Author

It also means that CI will have to fetch all dependencies for every run. I guess there will be a repository cache for composer packages somewhere on Jenkins ?

as long as we use explicit version and not git branch names (like dev-master) composer is caching the packages (btw: unlike npm as far as I know)

@DeepDiver1975
Copy link
Member Author

Would running composer update or so be enough to make it adjust lib versions even if there's a downgrade

composer install it will be

@DeepDiver1975
Copy link
Member Author

composer install will install dependencies as defined in composer.lock which holds the explicit versions.
composer update will update all dependencies according to composer.json and write explciit versions into composer-lock

@PVince81
Copy link
Contributor

ahem...

± % make
make: *** No rule to make target 'build/composer.phar', needed by 'install-composer-deps'.  Stop.

The makefile dependency need to refer to another rule name, not the path to a file.

@DeepDiver1975
Copy link
Member Author

@PVince81 composer cannot resolve the dependencies for php 5.4 and 5.5 - since we anyhow want to drop 5.4 and 5.5: shall we do it in here right away?

@PVince81
Copy link
Contributor

@DeepDiver1975 git add build/autotest*.sh build/buildjsdocs*.sh please 😉

@PVince81
Copy link
Contributor

@DeepDiver1975 I already added the missing files and also killed "buildjsdocs.sh" in favor of a more direct command in Makefile 😄

@DeepDiver1975
Copy link
Member Author

e793084bc9e3a1fc0f8498b0082821583f4b24811ea8aa9ff0e9326778a4483e

🎉 🎉 🎉 ALL GREEN 🎉 🎉 🎉

"blueimp-md5": "1.0.1",
"handlebars": "1.3.0",
"jcrop": "0.9.12",
"jquery": "2.1.4",
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to downgade jquery?

Copy link
Member Author

Choose a reason for hiding this comment

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

mainly bower version was wrong 😵

Copy link
Member Author

Choose a reason for hiding this comment

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

and we need to explicitly pin the versions

DeepDiver1975 and others added 13 commits August 29, 2016 21:30
We should only change/save the versions when manually updating.
This way the versions stay locked in place.
Added TEST_DATABASE and TEST_EXTERNAL to specify which env to test in.
Adjusted Jenkinsfile
Sometimes require_once would return true if the autoloader was already
loaded previously from a previous run of PHPUnit
This way composer.phar, composer libs, node js and core vendor libs are
only fetched once. If make detects that the directory exists, it will
skip the step. This is good for faster builds.
@VicDeo
Copy link
Member

VicDeo commented Aug 29, 2016

@DeepDiver1975 done

@DeepDiver1975
Copy link
Member Author

@DeepDiver1975 done

THX a lot!

@DeepDiver1975
Copy link
Member Author

All green - we are ready to go

@DeepDiver1975 DeepDiver1975 merged commit 005b3db into master Aug 29, 2016
@DeepDiver1975 DeepDiver1975 deleted the bye-bye-submodule branch August 29, 2016 22:30
# Catch-all rules
#
.PHONY: all
all: $(composer_deps) $(core_vendor)
Copy link
Contributor

Choose a reason for hiding this comment

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

@DeepDiver1975 weird, I thought we changed this to composer_dev_deps ?

@PVince81
Copy link
Contributor

PVince81 commented Sep 2, 2016

doc ticket for makefile: owncloud-archive/documentation#2604

@lock
Copy link

lock bot commented Aug 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
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.

None yet

5 participants