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

Composer build defaulting to webdir #67

Closed
wants to merge 11 commits into
base: develop
from

Conversation

Projects
None yet
7 participants
@ArthurHlt
Contributor

ArthurHlt commented Apr 24, 2015

By default, composer app will install dependencies inside the webdir (as it work locally).
The var "NO_WEBDIR_SET": "false" as been created inside defaults/options.json.
This var become true if no webdir has been set and if there is no default webdir (htdocs) available in sources.

See discussion here: #65

@dmikusa-pivotal you were right I couldn't run the test suite cause of my company proxy (of course NTLMV2 proxy ...) and my super windows...

@cfdreddbot

This comment has been minimized.

Show comment
Hide comment
@cfdreddbot

cfdreddbot Apr 24, 2015

Hey ArthurHlt!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

cfdreddbot commented Apr 24, 2015

Hey ArthurHlt!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

1 similar comment
@cfdreddbot

This comment has been minimized.

Show comment
Hide comment
@cfdreddbot

cfdreddbot Apr 24, 2015

Hey ArthurHlt!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

cfdreddbot commented Apr 24, 2015

Hey ArthurHlt!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

@cf-gitbot

This comment has been minimized.

Show comment
Hide comment
@cf-gitbot

cf-gitbot Apr 24, 2015

Collaborator

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/93193664.

Collaborator

cf-gitbot commented Apr 24, 2015

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/93193664.

.into('{BUILD_DIR}/{LIBDIR}')
.where_name_matches('^%s/.*$' % vendor_path)
.done())

This comment has been minimized.

@dmikusa-pivotal

dmikusa-pivotal Apr 24, 2015

Member

I think this should be OK. We need to run all the tests and see what breaks.

@dmikusa-pivotal

dmikusa-pivotal Apr 24, 2015

Member

I think this should be OK. We need to run all the tests and see what breaks.

@dmikusa-pivotal

View changes

Show outdated Hide outdated extensions/composer/extension.py Outdated
@dmikusa-pivotal

View changes

Show outdated Hide outdated extensions/composer/extension.py Outdated
@dmikusa-pivotal

This comment has been minimized.

Show comment
Hide comment
@dmikusa-pivotal

dmikusa-pivotal Apr 24, 2015

Member

Looking good. I made a couple notes and we need to run against tests, but this looks promising.

Member

dmikusa-pivotal commented Apr 24, 2015

Looking good. I made a couple notes and we need to run against tests, but this looks promising.

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Apr 24, 2015

Contributor

Should be better now

Contributor

ArthurHlt commented Apr 24, 2015

Should be better now

@dmikusa-pivotal

This comment has been minimized.

Show comment
Hide comment
@dmikusa-pivotal

dmikusa-pivotal Apr 24, 2015

Member

Fantastic!

Member

dmikusa-pivotal commented Apr 24, 2015

Fantastic!

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jun 4, 2015

Contributor

Sorry for disturbing but any news about this pull request? Could be great to merge it

Contributor

ArthurHlt commented Jun 4, 2015

Sorry for disturbing but any news about this pull request? Could be great to merge it

@jtarchie

This comment has been minimized.

Show comment
Hide comment
@jtarchie

jtarchie Jun 29, 2015

Member

@ArthurHlt, could you please rebase this with the latest develop?

It would also be great to document the config option NO_WEBDIR_SET in the docs/config.md.

Thanks!

Member

jtarchie commented Jun 29, 2015

@ArthurHlt, could you please rebase this with the latest develop?

It would also be great to document the config option NO_WEBDIR_SET in the docs/config.md.

Thanks!

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jun 30, 2015

Contributor

it's done

Contributor

ArthurHlt commented Jun 30, 2015

it's done

@mhoran

This comment has been minimized.

Show comment
Hide comment
@mhoran

mhoran Jun 30, 2015

Member

Thanks @ArthurHlt. We tried to pull in your changes but ran into a few issues.

First off, it looks like something went wrong with your rebase. It actually looks like the branch got merged into itself. We were able to merge in the rebased commits and cherry pick the documentation commit, which fixed the pull.

More importantly: both the nose tests and the integration tests are failing. The run_tests.py script can be used to run the nose tests. Likely changing the COMPOSER_BIN_DIR or introduction of the new option itself caused these failures.

The integration tests are failing for a number of reasons; reasons we haven't seen before. Our most basic test is failing to run the app at all. You can run the integration tests by following the instructions in the machete README. Please let us know if anything needs further explanation.

Member

mhoran commented Jun 30, 2015

Thanks @ArthurHlt. We tried to pull in your changes but ran into a few issues.

First off, it looks like something went wrong with your rebase. It actually looks like the branch got merged into itself. We were able to merge in the rebased commits and cherry pick the documentation commit, which fixed the pull.

More importantly: both the nose tests and the integration tests are failing. The run_tests.py script can be used to run the nose tests. Likely changing the COMPOSER_BIN_DIR or introduction of the new option itself caused these failures.

The integration tests are failing for a number of reasons; reasons we haven't seen before. Our most basic test is failing to run the app at all. You can run the integration tests by following the instructions in the machete README. Please let us know if anything needs further explanation.

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jun 30, 2015

Contributor

Hello @mhoran,
Sorry for the rebase.
This PR haven't test or modify test I thought @dmikusa-pivotal wanted to do it.
About run your integreation test, as I said in the first post:

@dmikusa-pivotal you were right I couldn't run the test suite cause of my company proxy (of course NTLMV2 proxy ...) and my super windows...

So I'm sorry I will try on my mac

Contributor

ArthurHlt commented Jun 30, 2015

Hello @mhoran,
Sorry for the rebase.
This PR haven't test or modify test I thought @dmikusa-pivotal wanted to do it.
About run your integreation test, as I said in the first post:

@dmikusa-pivotal you were right I couldn't run the test suite cause of my company proxy (of course NTLMV2 proxy ...) and my super windows...

So I'm sorry I will try on my mac

@jtarchie

This comment has been minimized.

Show comment
Hide comment
@jtarchie

jtarchie Jun 30, 2015

Member

@ArthurHlt, I work with @mhoran.

Sorry we missed the bit on the company proxy. I'll take a look at the failing to narrow down the reason for the failure. Might have to ping you for some more information later.

Member

jtarchie commented Jun 30, 2015

@ArthurHlt, I work with @mhoran.

Sorry we missed the bit on the company proxy. I'll take a look at the failing to narrow down the reason for the failure. Might have to ping you for some more information later.

@jtarchie

This comment has been minimized.

Show comment
Hide comment
@jtarchie

jtarchie Jun 30, 2015

Member

@ArthurHlt, this line is causing some of our tests to fail.

For example, our fixture for symphony, fails with a cf push with this log statement.

2015-06-30T14:41:38.60-0400 [STG/0]      ERR Composer could not find a composer.json file in /tmp/staged/app/public
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR To initialize a project, please create a composer.json file as described in the http://getcomposer.org/ "Getting Started" section
       2015-06-30T14:41:38.60-0400 [STG/0]      OUT -----> Composer command failed
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR Traceback (most recent call last):
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/scripts/compile.py", line 51, in <module>
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     .from_build_pack('lib/additional_commands')
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/lib/build_pack_utils/builder.py", line 189, in extensions
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     process_extension(path, ctx, 'compile', process, args=[self])
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/lib/build_pack_utils/utils.py", line 69, in process_extension
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     success(getattr(extn, to_call)(*args))
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/extensions/composer/extension.py", line 410, in compile
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     return composer.compile(install)
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/lib/extension_helpers.py", line 154, in compile
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     self._compile(install)
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/extensions/composer/extension.py", line 172, in _compile
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     self.run()
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/extensions/composer/extension.py", line 292, in run
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     *self._ctx['COMPOSER_INSTALL_OPTIONS'])
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/extensions/composer/extension.py", line 340, in run
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     shell=True)
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/lib/build_pack_utils/runner.py", line 109, in stream_output
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     raise CalledProcessError(retcode, cmd)
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR build_pack_utils.runner.CalledProcessError: Command '<open file '<fdopen>', mode 'w' at 0x7fbcaa6db780>' returned non-zero exit status 1

This implies it is breaking backwards compatibility of the composer extension.

Can you help narrow down the issue?

Member

jtarchie commented Jun 30, 2015

@ArthurHlt, this line is causing some of our tests to fail.

For example, our fixture for symphony, fails with a cf push with this log statement.

2015-06-30T14:41:38.60-0400 [STG/0]      ERR Composer could not find a composer.json file in /tmp/staged/app/public
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR To initialize a project, please create a composer.json file as described in the http://getcomposer.org/ "Getting Started" section
       2015-06-30T14:41:38.60-0400 [STG/0]      OUT -----> Composer command failed
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR Traceback (most recent call last):
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/scripts/compile.py", line 51, in <module>
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     .from_build_pack('lib/additional_commands')
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/lib/build_pack_utils/builder.py", line 189, in extensions
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     process_extension(path, ctx, 'compile', process, args=[self])
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/lib/build_pack_utils/utils.py", line 69, in process_extension
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     success(getattr(extn, to_call)(*args))
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/extensions/composer/extension.py", line 410, in compile
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     return composer.compile(install)
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/lib/extension_helpers.py", line 154, in compile
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     self._compile(install)
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/extensions/composer/extension.py", line 172, in _compile
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     self.run()
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/extensions/composer/extension.py", line 292, in run
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     *self._ctx['COMPOSER_INSTALL_OPTIONS'])
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/extensions/composer/extension.py", line 340, in run
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     shell=True)
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR   File "/var/vcap/data/dea_next/admin_buildpacks/965f6f77-43bc-43d8-8f62-af7a6485f136_4f4fb921b49f7839852d711e1619435dee9b692f/lib/build_pack_utils/runner.py", line 109, in stream_output
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR     raise CalledProcessError(retcode, cmd)
       2015-06-30T14:41:38.60-0400 [STG/0]      ERR build_pack_utils.runner.CalledProcessError: Command '<open file '<fdopen>', mode 'w' at 0x7fbcaa6db780>' returned non-zero exit status 1

This implies it is breaking backwards compatibility of the composer extension.

Can you help narrow down the issue?

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jul 1, 2015

Contributor

So I've made a couple of tests manually and I've found the error.
In fact I've added a var in config/options.json, it's NO_WEBDIR_SET and I've set it to false and I tought that this var will be in the context as a boolean but it was not, it was just text.

So like I've made condition as a boolean on a text value it was always true and if it's true composer files will not be moved inside the right directory.

You can found the fix (in options.json the value is empty now and it's work)

Contributor

ArthurHlt commented Jul 1, 2015

So I've made a couple of tests manually and I've found the error.
In fact I've added a var in config/options.json, it's NO_WEBDIR_SET and I've set it to false and I tought that this var will be in the context as a boolean but it was not, it was just text.

So like I've made condition as a boolean on a text value it was always true and if it's true composer files will not be moved inside the right directory.

You can found the fix (in options.json the value is empty now and it's work)

@dmikusa-pivotal

This comment has been minimized.

Show comment
Hide comment
@dmikusa-pivotal

dmikusa-pivotal Jul 1, 2015

Member

If you want it to be false, you should set it to "False" with a capital "F". Python will convert that to a false boolean when it parses the JSON. Leaving the string blank, would be unclear IMHO.

Member

dmikusa-pivotal commented Jul 1, 2015

If you want it to be false, you should set it to "False" with a capital "F". Python will convert that to a false boolean when it parses the JSON. Leaving the string blank, would be unclear IMHO.

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jul 1, 2015

Contributor

Thanks for the tip @dmikusa-pivotal it was unclear to me cause in this file we can found value as true. So I change it to False

Contributor

ArthurHlt commented Jul 1, 2015

Thanks for the tip @dmikusa-pivotal it was unclear to me cause in this file we can found value as true. So I change it to False

@dmikusa-pivotal

This comment has been minimized.

Show comment
Hide comment
@dmikusa-pivotal

dmikusa-pivotal Jul 1, 2015

Member

Disregard. I was wrong on that, sorry for the bad advise. I did a couple quick tests to confirm, here's how it works.

1.) "true", "false", "True" and "true" are parsed and converted to strings.

2.) true and false (without quotes) are parsed and turned to booleans.

It seems like we should be using the latter (i.e. removing quotes). I think it's working now because the string "true" is truthy, i.e. if "true": is actually true. We should probably fix that at some point, because if you did a comparison to True boolean it would fail and be confusing.

Member

dmikusa-pivotal commented Jul 1, 2015

Disregard. I was wrong on that, sorry for the bad advise. I did a couple quick tests to confirm, here's how it works.

1.) "true", "false", "True" and "true" are parsed and converted to strings.

2.) true and false (without quotes) are parsed and turned to booleans.

It seems like we should be using the latter (i.e. removing quotes). I think it's working now because the string "true" is truthy, i.e. if "true": is actually true. We should probably fix that at some point, because if you did a comparison to True boolean it would fail and be confusing.

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jul 1, 2015

Contributor

That's ok for me, so just to be sure i should set false without quote and without a capital "F" ?

Contributor

ArthurHlt commented Jul 1, 2015

That's ok for me, so just to be sure i should set false without quote and without a capital "F" ?

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jul 1, 2015

Contributor

I could include modification for the value "true" in options.json in this PR if you want.

Contributor

ArthurHlt commented Jul 1, 2015

I could include modification for the value "true" in options.json in this PR if you want.

@dmikusa-pivotal

This comment has been minimized.

Show comment
Hide comment
@dmikusa-pivotal

dmikusa-pivotal Jul 1, 2015

Member

Yes: false without quotes and all lower case.
No: don't change the "true" values in this PR. IMHO that'll confuse things. You could submit a second PR though, should be an easy fix :)

Member

dmikusa-pivotal commented Jul 1, 2015

Yes: false without quotes and all lower case.
No: don't change the "true" values in this PR. IMHO that'll confuse things. You could submit a second PR though, should be an easy fix :)

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jul 1, 2015

Contributor

Ok it's done and I've made the PR for fix boolean values: #85

Contributor

ArthurHlt commented Jul 1, 2015

Ok it's done and I've made the PR for fix boolean values: #85

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Jul 4, 2015

Contributor

Finally I've could run tests with travis-ci. I fixed all tests which needed to be.
You can see the result here: https://travis-ci.org/ArthurHlt/php-buildpack/builds/69508305
You can note that the branch use in travis is not what I PR but it was just for don't send my .travis.yml file.

I think it's ready to merge so :)

Contributor

ArthurHlt commented Jul 4, 2015

Finally I've could run tests with travis-ci. I fixed all tests which needed to be.
You can see the result here: https://travis-ci.org/ArthurHlt/php-buildpack/builds/69508305
You can note that the branch use in travis is not what I PR but it was just for don't send my .travis.yml file.

I think it's ready to merge so :)

@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones Jul 30, 2015

Member

Hi @ArthurHlt,

Sorry it's taken me so long to get back to this PR. Currently, this changeset is extremely large, possibly due to merge conflicts. Can you help me understand what the smallest patch you want to apply is?

Member

flavorjones commented Jul 30, 2015

Hi @ArthurHlt,

Sorry it's taken me so long to get back to this PR. Currently, this changeset is extremely large, possibly due to merge conflicts. Can you help me understand what the smallest patch you want to apply is?

@ArthurHlt

This comment has been minimized.

Show comment
Hide comment
@ArthurHlt

ArthurHlt Sep 30, 2015

Contributor

i will do it again in a fresh repo and send a new PR so.

Contributor

ArthurHlt commented Sep 30, 2015

i will do it again in a fresh repo and send a new PR so.

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