-
Notifications
You must be signed in to change notification settings - Fork 155
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
Same named directories in file path cause issue on re-install #76
Comments
could you please point to a public available module and a workflow how to reproduce the problem? |
Sure. First, take a look at this repo: https://bitbucket.org/cnanninga/magento-example You'll notice that the only model file is located at app/code/local/Nanninga/Test/Model/Widget/Widget/Instance.php You can test the bug by cloning this repo: . . . which contains the module files already copied into a Magento codebase. Once you've cloned this, run "composer install" from the root. A git status should show immediately that there is a new file at htdocs/app/code/local/Nanninga/Test/Model/Widget/Instance.php, which is a result of the Magento Composer Installer copying Widget/Widget/Instance.php here instead of the right location. |
Here the test case I'm using to debug this. The call to git status should result in nothing, but shows a new file, which is not present in the test magento-module repository. The fix in commit c812c5c actually introduces a different sort of replication bug when the modman paths differ, so I've got to chase that one down now… unless someone else has a solution. ;)
|
will not get fixed, as it is in original modman only possible with |
@Flyingmana I completely disagree with what you're saying, and for the record, that is completely unrelated to the issue at hand. This isn't an issue with linking failing because something is already there, the bug is that the copy (not link, it doesn't affect the link method) is putting the files in the wrong place. If you look at commit 7a9ff2d on my fork, you'll see that I have implemented a unit test which demonstrates the bug. Take a look if you will, but this is absolutely not due to having a broken modman file. Are you trying to say that you not accept a pull-request which resolves this bug? |
linux what I say is, that I dont accept the initial reported issue as a bug. |
This is why I don't like the --copy option, there are too many conflicts since you can't easily tell the difference between a file that is safe to delete and one that is not (whereas symlinks are pretty much always safe to delete). So for the people that want to use --copy, I think the only way is to essentially assume --force. That is if |
If I'm following this correctly, the root cause of this issue is:
So it's not technically speaking a bug in modman or the composer module, it's a limitation of the implementation of But @Flyingmana you don't want to merge that fix? I haven't reviewed the code in detail or tested it, but if the patch resolves the issue, I can't see why you wouldn't want to merge it? |
please comment the pull request related comments on pull request :) |
Oops sorry I was linked directly here from twitter - I can see on the PR that tests are failing and that seems to be the only thing holding it up! Thanks Daniel! |
@kalenjordan Yeah… still working on figuring out an actual solution. Forgot to run unit tests on my first pass. :/ I've got 2 failing tests still. @Flyingmana Ok. I think you're misreading the issue then. :) The root cause is not due to behavior of linux cp. I understand how that works, this is different. And I'll also note that implementing a test to reproduce the bug requires the use of the force flag too. I've created a 2nd branch on my fork "issue-76-tests" which has the unit test isolated from my modifications in order to demonstrate the bug, then I added calls to tree so it will print the output for debugging purposes. Here is the output from the test:
Note that in the 2nd tree which is printed, the file test.xml shows up twice. This should not be the case. In summary, the nodes in the path with the same name are causing trouble walking the path because |
@colinmollenhour - Totally agree. Love links and prefer them here too! But have to use copy in this case to build something we can wrap up in a tgz. It's complicated why we need the copy. Since most don't use it, I'm guessing it's less tested than the others too. |
ok, you are right, I really misunderstood the problem. It seemed to absurd actually having a naming of Model_Widget_Widget_... |
lets see, so you have a modman entry of does this test package match your case? |
Looked it over and I believe that matches the case, yes. I've pushed up more changes to the PR for this which I believe resolve the issue, and which pass on all the unit tests for 2.0.0-beta1. |
ok, on the first execution it generates:
on the second run we have:
So I now add a NotExists test for Something clearly is wrong here. |
With that directory output, it's not a correct test. The first execution is fine. With this bug, the second output would look like this:
The NotExists check should be for the file which shows up in the 2nd round. I.e. for You can reference the test I put together which reproduced the bug and correctly failed prior to the work I've done on this branch I'll have to see if I can dig up a public facing module which exhibits this behavior and post a link. I do have this test case one which reproduces the issue: https://github.com/davidalger/composer-basename-bug-case.git |
Here is the best way to mock the issue outside the confines of a unit test:
|
Resolves bug reported in Issue #76 does not break any existing unittests, currently failing unittests were already failing before
Unfortunately this change seems to have some negative side effects. Since 2.0.0-beta2 we are experiencing the following behaviour:
|
hi @bragento |
Ported from the comment made on a source-line: c812c5c#commitcomment-6045581 — Makes most sense to continue discussion here and keep it in one place. :) davidalger added a note a day ago bragento added a note a day ago
Now simply create the Magento-Root Dir. Add an empty "app" folder to it. Trigger composer install. |
@davidalger I added a testcase for this. Any Ideas how to solve this, as it seems Issued by your patch? Also, there seems to be a fullstack testcase missing for your changes. |
Not sure yet since I haven't yet had a chance to attempt reproducing it, but I will be doing that shortly. I need to get the full-stack tests working on my machine though… still getting this on each of the three modes when I run the full-stack tests:
Other artifacts are being created though:
The full-stack test would appear to be incorrect; the failing individual unit-test is the one I added ( Any ideas why the full-stack tests wouldn't be failing? I can run them in a CentOS VM just fine…but on my primary OS X setup not so much. |
Alright…since I got the full-stack tests running per the changes I made in pull request #90, I might actually get somewhere on this now. ;) |
I maybe fixed the problem, at least I could create a unittest for it, a fullstack test, and nothing fails now. |
Looks like a valid fix. @bragento, could you test this against your scenario and report back? |
Thanks a lot for your support so far. 2.0.0-beta3: Syntax error ==> PHP Parse error: syntax error, unexpected '[' in /home/brandung/www/test_beta3/vendor/magento-hackathon/magento-composer-installer/src/MagentoHackathon/Composer/Magento/Plugin.php on line 37 dev-plugin-rewrite 6618b9c: Fatal Error ==> PHP Fatal error: Call to a member function addPackage() on a non-object in /home/brandung/www/test_1x/vendor/magento-hackathon/magento-composer-installer/src/MagentoHackathon/Composer/Magento/Installer.php on line 267 |
the dev-plugin-rewrite branch is the correct one. I assume the error happens during the use of the composer command integrator? I dont have a working test solution for the command integrator yet, and looking at the source I think there could be some problematic parts. One way would be, you try yourself to change \MagentoHackathon\Composer\Magento\Command\DeployCommand till it executes all new actions of the installer you need. Or you have a few weeks time, so I refactor half the logic again, to make it easier to reuse from the command class. Does this sound ok for you? And could you please test again the dev-plugin-rewrite branch? |
Actually I just wanted to execute a regular "composer install" when I got the error. Currently we have no real use for the command line integrator since our processes are either triggered by events or our installer during the regular install/update actions. Thanks a lot so far! |
@bragento Could you post a composer.json which reproduces the issue? I am not able to get those errors to trigger. |
Hi @Flyingmana I think I'm facing the same or similar issue now. Here are the exact details on the issue: First I have my composer.json file under my magento root directory: { "require": { "magento-hackathon/magento-composer-installer":"*", "factoryx-developers/factoryx_contests":"dev-master", "factoryx-developers/factoryx_birthdaygift":"dev-master" }, "repositories": [ { "type": "vcs", "url": "https://github.com/magento-hackathon/magento-composer-installer" }, { "type": "vcs", "url": "https://**********@bitbucket.org/**********/factoryx_contests.git" }, { "type": "vcs", "url": "https://**********@bitbucket.org/**********/factoryx_birthdaygift.git" } ], "extra": { "magento-root-dir": "./", "magento-deploystrategy": "copy" } } And here is the content of my module composer.json files (simplified and both are the same): { "name": "**********/factoryx_contests", "type": "magento-module", "license":"OSL-3.0", "description":"Contests module", "authors":[ { "name":"Raphael Petrini", "email":"raphael@**********" } ], "extra": { "map": [ ["locale/*", "app/locale/en_AU"] ] } } Under my locale folder I have:
When I run composer update, everything goes fine (no error) but I end up with the following folders:
If I add a third module which another locale config, it'll add an third template subfolder in the previously created ones and thus, that totally breaks my module configuration. |
does this still happen with the most recent 3.x version? |
Any updates on that issue? |
I have the following situation: I've already run my install the first time, meaning all files from Magento modules have been copied into the codebase (copy, not symlink) and the composer.lock file is already present. On a new clone of this codebase where the vendor directory doesn't exist, I run "composer install" to pull down the components into vendor. This results in the modman-based copy operation being performed again, but a problem happens only in the specific case where the same directory name occurs twice in a row in one of the file paths. Example:
One of my dependency repos has the following line in modman:
app/code/community/CLS/Widgets app/code/community/CLS/Widgets
One of the files there is this one, which has already been copied into the codebase on the first install:
htdocs/app/code/community/CLS/Widgets/Model/Widget/Widget/Instance.php
Notice "Widget/Widget" in the file path. This copy operation worked fine on the first install, but when running "composer install" again with the files already in place, I end up with the following file in my codebase:
htdocs/app/code/community/CLS/Widgets/Model/Widget/Instance.php
i.e., the extra "Widget" in the file path was somehow removed in the copy destination, and the file was copied into the wrong place.
The text was updated successfully, but these errors were encountered: