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

WIP: [ticket/11163] Make ext/ dir upon composer install command #1032

Closed
wants to merge 1 commit into from

Conversation

imkingdavid
Copy link
Contributor

@@ -8,5 +8,8 @@
},
"require-dev": {
"fabpot/goutte": "1.0.x-dev"
},
"scripts": {
"post-install-cmd": "mkdir ext"
Copy link
Member

Choose a reason for hiding this comment

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

ext -> ext/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

works either way

@michaelcullum
Copy link
Member

You might need backticks.

Class is not autoloadable, can not call post-install-cmd script

Is this not tested?

@imkingdavid
Copy link
Contributor Author

As I said, make sure your composer.phar is up to date. I did test it and it
worked.

On Sat, Nov 3, 2012 at 11:08 AM, Michael C. notifications@github.comwrote:

You might need backticks.

"Class is not autoloadable, can not call post-install-cmd script"

Is this not tested?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1032#issuecomment-10040195.

@brunoais
Copy link
Contributor

brunoais commented Nov 3, 2012

I think you should also create an .htaccess and an empty index.html...

@bantu
Copy link
Collaborator

bantu commented Nov 3, 2012

This should be extended to all the other folders such as cache. The .htacces and index.html file should IMO be created by the build script. We can assume that people using phpBB via git know how to secure their webserver.

@michaelcullum
Copy link
Member

I didn't test it but travis did and it uses the composer.phar in the repo? https://travis-ci.org/#!/phpbb/phpbb3/jobs/3045295

@imkingdavid
Copy link
Contributor Author

Well I have updated composer.phar and composer.lock but the tests are failing due to it being unable to locate a file or something. So it's not my fault.

@michaelcullum
Copy link
Member

Actually, they were all failing earlier, since you updated the composer.phar only some are failing, which are not your fault. Not sure why its only failing on some versions.

@bantu
Copy link
Collaborator

bantu commented Nov 5, 2012

Please rebase.

@p
Copy link
Contributor

p commented Nov 6, 2012

Please let's not pile up all feature requests into a single PR (cough bantu).

@bantu
Copy link
Collaborator

bantu commented Nov 6, 2012

@p huh? I just want to get rid of the composer.phar file here because that has been updated already (and has to go into develop-olympus too)

@p
Copy link
Contributor

p commented Nov 6, 2012

I was referring to

This should be extended to all the other folders such as cache.

@p
Copy link
Contributor

p commented Nov 6, 2012

Although if you have to populate ext then obviously you have to do that, sorry.

@p
Copy link
Contributor

p commented Nov 6, 2012

Also, if this is done, for a future PR I would like to see a command to recreate all generated files and directories.

@michaelcullum
Copy link
Member

@p Yes, just run php ../composer.phar install and even if deps aren't updated the commands are still run.

@p
Copy link
Contributor

p commented Nov 7, 2012

Why is composer.lock changed in this PR?

5 similar comments
@p
Copy link
Contributor

p commented Nov 7, 2012

Why is composer.lock changed in this PR?

@p
Copy link
Contributor

p commented Nov 7, 2012

Why is composer.lock changed in this PR?

@p
Copy link
Contributor

p commented Nov 7, 2012

Why is composer.lock changed in this PR?

@p
Copy link
Contributor

p commented Nov 7, 2012

Why is composer.lock changed in this PR?

@p
Copy link
Contributor

p commented Nov 7, 2012

Why is composer.lock changed in this PR?

@bantu
Copy link
Collaborator

bantu commented Nov 8, 2012

composer.lock change should be removed together with composer.phar.

@imkingdavid
Copy link
Contributor Author

I rebased out the changes to composer.phar and composer.lock. Note that you may need to update your own composer to the latest in order to get it to work. Also, the travis test will fail because its composer is not updated. EDIT: And looks like I'm wrong; the tests passed.

@p
Copy link
Contributor

p commented Nov 10, 2012

If a composer update is needed it should be done in its own PR.

@bantu
Copy link
Collaborator

bantu commented Nov 10, 2012

Huh. We already updated composer in the repo for this.
On 10 Nov 2012 01:31, "Oleg Pudeyev" notifications@github.com wrote:

If a composer update is needed it should be done in its own PR.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1032#issuecomment-10249588.

@p
Copy link
Contributor

p commented Nov 10, 2012

Does not seem to work.

https://gist.github.com/d37a17e5f3009dc84d92

No ext directory was created.

1 similar comment
@p
Copy link
Contributor

p commented Nov 10, 2012

Does not seem to work.

https://gist.github.com/d37a17e5f3009dc84d92

No ext directory was created.

@p
Copy link
Contributor

p commented Nov 10, 2012

Scratch that, forgot to rsync. Patch does work.

@p
Copy link
Contributor

p commented Nov 10, 2012

Scratch that, forgot to rsync. Patch does work.

2 similar comments
@p
Copy link
Contributor

p commented Nov 10, 2012

Scratch that, forgot to rsync. Patch does work.

@p
Copy link
Contributor

p commented Nov 10, 2012

Scratch that, forgot to rsync. Patch does work.

@p
Copy link
Contributor

p commented Nov 10, 2012

The command should be mkdir -p, not mkdir.

https://gist.github.com/57970ce4225fe79f9d4f

@bantu
Copy link
Collaborator

bantu commented Nov 10, 2012

mkdir -p works on Unix, but not on Windows.
https://gist.github.com/830a2f42f06a399b83a4
@nickvergessen tested it on Windows, it throws an error anyway.

@bantu
Copy link
Collaborator

bantu commented Nov 11, 2012

Maybe the directories should be created by PHP.

@p
Copy link
Contributor

p commented Nov 11, 2012

If users checking out phpbb code from git can be assumed to know where extensions are supposed to go, maybe just create directories during package building.

@p
Copy link
Contributor

p commented Nov 11, 2012

I would also be content with a develop script to make them.

@michaelcullum
Copy link
Member

Can I ask, why are we coming up with alternatives? What is wrong with composer creating the file? Composer needs to run for a phpBB install to work and the build script calls composer.

@igorw
Copy link
Contributor

igorw commented Nov 12, 2012

I would not do this with composer. ext should be created in the repo (via ext/.gitkeep) and then ext/* should be ignored. If needed we add !ext/.gitkeep to the gitignore (which makes it not-ignored).

@michaelcullum
Copy link
Member

That was done by nathan but people said the implementation wasn't how it should be done.

@bantu
Copy link
Collaborator

bantu commented Nov 13, 2012

So, use gitkeep and have build script 1) remove gitkeep and 2) create index and .htaccess?

@michaelcullum
Copy link
Member

If we need an index.html then we might as well not have a .gitkeep

@bantu
Copy link
Collaborator

bantu commented Nov 13, 2012

The other approach is however more failure-proof.

@p
Copy link
Contributor

p commented Nov 17, 2012

Can I ask, why are we coming up with alternatives? What is wrong with composer creating the file?

The current patch requires that the directory does not already exist. Running composer install twice results in a failure to create the directory the second time because it was created on the first run. See my previous comment.

@michaelcullum
Copy link
Member

Add an if?

@p
Copy link
Contributor

p commented Nov 17, 2012

If you're talking about a shell if, the syntax will differ across operating systems.

If you're talking about a composer if, I don't know what composer provides.

@imkingdavid
Copy link
Contributor Author

If I understand correctly, this is not an acceptable solution. Closing.

@p
Copy link
Contributor

p commented Dec 12, 2012

You could invoke a php file to make the directory. However, if at that point you can simply provide this file for users to invoke as they please.

@bantu
Copy link
Collaborator

bantu commented Dec 12, 2012

I'll work on an alternative patch.

@p
Copy link
Contributor

p commented Dec 13, 2012

Filed composer/composer#1417.

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