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

[4.0] Removing the libraries/vendor folder #20059

Merged
merged 7 commits into from Apr 12, 2018

Conversation

rdeutz
Copy link
Contributor

@rdeutz rdeutz commented Apr 2, 2018

Current situation

Our libraries/vendor folder contains all 3rd part packages Joomla! depends on. This gives us a lot of headache when it comes to updating stuff or requesting packages e.g. for testing. A simple composer require will update also unrelated packages and you get hundreds of changes in PR.

We discussed the situation in different groups and voted in the production department on removing the folder contends. The voting passed with a majority so we are going to remove the folder in 4.0-dev soon. To be clear this doesn't effect the 3.x series of Joomla!

A direct impact of this change is that people can't clone the repository and have an installable Joomla! They have to make one additional step "composer install" and then they are good to go. We understand that this might be a burden for some people and that we have to explain what are the steps before and after cloning a repository. On the other side, git isn't a software distribution channel and this only infects the core and extension development. For testing and all other things we can use nightly builds or releases on download.joomla.org.

ToDo

[ ] Publishing a article on developer.joomla.org and explaining the situation
[ ] Writing documentation on docs.joomla.org
[ ] Fix the nightly build script

@brianteeman
Copy link
Contributor

Does that mean there will no longer be a nightly install OR will the script that makes that be updated to do the composer install

@mbabker
Copy link
Contributor

mbabker commented Apr 2, 2018

For testing and all other things we can use nightly builds or releases on download.joomla.org.

I thought that implied the build scripts/workflows for all build types (tagged release packages and nightlies) would all be updated appropriately 😉

@brianteeman
Copy link
Contributor

sorry I missed the final line - mea culpa

out if interest how do other projects manage this

@mbabker
Copy link
Contributor

mbabker commented Apr 2, 2018

Concrete5, OctoberCMS, Grav, and Drupal all use Composer in core and do not check the vendor directory into their VCS.

@brianteeman
Copy link
Contributor

I guess I misunderstood this folder then https://github.com/drupal/drupal/tree/8.6.x/core/assets/vendor

@mbabker
Copy link
Contributor

mbabker commented Apr 2, 2018

That's UI dependencies that NPM is used to manage. Composer strictly covers PHP dependencies.

@brianteeman
Copy link
Contributor

i guessed correctly then :)

@dgrammatiko
Copy link
Contributor

Although this PR is touching only the PHP vendors I would like to add here a note about the javascript vendors:

The reduction on lines of our repos actual code is far more dramatic for javascript:

With media/vendor

screen shot 2018-04-02 at 18 00 08

Without

screen shot 2018-04-02 at 18 00 44

A reminder here: neither any of the files belonging to libraries/vendors or media/vendor should ever changed in our repo, so removing them also prevents such behaviours.

@rdeutz
Copy link
Contributor Author

rdeutz commented Apr 2, 2018

While I agree with Dimitries that removing other "vendor" directories makes sense, I would like to keep this on the PHP level. This PR implements what we have agreed on in the production department. Other vendor directories would need another voting and PR :-)

@laoneo
Copy link
Member

laoneo commented Apr 3, 2018

Are the build scripts, etc. updated to work with this change, or will they break after merge?

@rdeutz
Copy link
Contributor Author

rdeutz commented Apr 3, 2018

If you do a composer install before running the build script anything should work.

Side note: We (automated testing team) are working on consolidation of our robo scripts and the plan is to have the build script moved to robo scripts. So we can provide ONE interface for build, develop and testing.

@Bakual
Copy link
Contributor

Bakual commented Apr 4, 2018

@rdeutz I think Allon meant that there needs to be adjustments to tne nightly script so that composer install is executed. Otherwise the nightlies will be broken.

I'm also wondering: Currently we "remove" quite a few files (eg test folders) using the .gitignore file. How do we make sure they now don't end up in the distributed package?

@rdeutz
Copy link
Contributor Author

rdeutz commented Apr 4, 2018

@Bakual added a todo to the list

@mbabker
Copy link
Contributor

mbabker commented Apr 4, 2018

Fix the nightly build script

If #19848 ever gets merged then the nightly build script would (finally) be able to use the same packaging script that is used for tagged releases. So build.php should be updated as part of this pull request with whatever changes are required to be able to build a package, I will adjust how the nightlies are built based on that as I have done for every other change implemented in the build script.

@wilsonge
Copy link
Contributor

wilsonge commented Apr 4, 2018

If #19848 ever gets merged

It's merged

@mbabker
Copy link
Contributor

mbabker commented Apr 4, 2018

And now the staging and 3.9 build jobs use the modified script. Just need to get that merged up to 4.0, have fun in git merge hell George 😝

@laoneo
Copy link
Member

laoneo commented Apr 4, 2018

We can cherry pick that commit if needed.

@mbabker
Copy link
Contributor

mbabker commented Apr 4, 2018

It's no rush. The old faithful hacked up build script will continue to suffice until this PR is merged. It'll just make my life more sane if I only have to update the 4.0-nightly.sh file one last time to point to our packager friendly build.php file instead of my hacked to all bits and supporting way too many paths build-jenkins.php file 😃

@ghost ghost mentioned this pull request Apr 9, 2018
@laoneo
Copy link
Member

laoneo commented Apr 12, 2018

I'v cherry picked #19848 with 800dd67, so the build script should be up to date.

@laoneo laoneo merged commit 2b4682f into joomla:4.0-dev Apr 12, 2018
@brianteeman
Copy link
Contributor

This really should not have been merged as there were still three outstanding todo items in the original post and it didnt have any successful user tests.

Please can this be reverted UNTIL the todo items have been completed as they severely impact any testing

@mbabker
Copy link
Contributor

mbabker commented Apr 12, 2018

I have to say this publicly. You pulled the trigger too early on this. com_patchtester is now unusable on vendor related PRs (i.e. the database stuff). The GSoC PR testing platform is not yet available. The nightly builds update is not a 30 second task (it's actually about 15 minutes but AFAIK I'm the only one who knows how that's running at the moment and it's going to be a little rude to drop the conference for this). Essentially, right now unless you're a coder you can't test patches on the 4.0 branch. This PR isn't an issue, but the fact that it was done so prematurely is very problematic.

@brianteeman
Copy link
Contributor

I dont know what the rush was to merge this. Or why procedures were broken to merge this

@laoneo
Copy link
Member

laoneo commented Apr 12, 2018

Reverted, was a misunderstanding. Sorry for that.

laoneo added a commit that referenced this pull request Apr 12, 2018
@laoneo
Copy link
Member

laoneo commented Apr 12, 2018

Commit is here f2f425f. Learned my lesson.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants