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

Direct Install lacks options and cleanup #1389

Closed
OleVik opened this issue Mar 29, 2017 · 14 comments
Closed

Direct Install lacks options and cleanup #1389

OleVik opened this issue Mar 29, 2017 · 14 comments

Comments

@OleVik
Copy link
Contributor

OleVik commented Mar 29, 2017

Thought I'd tag @akeif for this, since he's been very involved in getting the directinstall command working. Whilst the command works well, with only minor infidelities I thus far have blamed on Windows and haven't tracked down fully in debug, two points are worthy of immediate consideration: The command does not register common options, and folders are not deleted on failure or success.

For the former, force, all-yes, and destination are not available. Just like the rest of GPM, having them available cuts down on development-time significantly. For the latter, I noticed that regardless of whether the installation halts or succeeds, the temporary folders in /tmp/ are not deleted - at least none have been thus far.

@gnufred
Copy link
Contributor

gnufred commented Mar 29, 2017

Thanks @OleVik 😄. I'm definitely interested in tackling this issue.

My understanding

If I get you right, here what should be done before solving this issue.

  1. files created in /tmp/ while running directinstall should be removed
  2. folders must be deleted whether the command succeed or fail
  3. command force should be available and implemented properly
  4. command all-yes should be available and implemented properly
  5. command destination should be available and implemented properly

Request for more information on point 2

I get you are saying that this might happens only on Windows.
Also, what do you mean by folders?

@OleVik
Copy link
Contributor Author

OleVik commented Mar 29, 2017

Folders, as in directories. After running the command, whether it fails or not, it leaves behind a copy of the extracted extension in /tmp/Grav-randomhash/ for each attempt, but they are not subsequently removed by the process.

What happens on Windows is not related to point 1-5, they are merely current deficiencies of the implementation. On Windows two things happen on occasion: mkdir fails, or Composer's Autoload fails with the name is already in use in /tmp/Grav-randomhash/pluginname/pluginname.php. I suspect this is more an issue with Windows, though, and shouldn't be tackled unless it can be consistently reproduced.

@gnufred
Copy link
Contributor

gnufred commented Mar 29, 2017

All right. So I'll stick to this:

  1. files created in /tmp/ should be removed whether the command succeed or fail
  2. command force should be available and implemented properly
  3. command all-yes should be available and implemented properly
  4. command destination should be available and implemented properly

@gnufred
Copy link
Contributor

gnufred commented Mar 29, 2017

I'll take a look and make a plan before coding. I'd like create a merge request for each point but we'll see how this goes.

I'll try to do this tonight but will do Friday or Saturday at worst.

@gnufred
Copy link
Contributor

gnufred commented Mar 29, 2017

Changes to consider after this issue has been resolved

  • Add bin/gpm direct-install command to the API doc.
  • Add to doc: how to run unit tests
  • Fix found issues 1, 2
  • Get tests to work

@gnufred
Copy link
Contributor

gnufred commented Mar 29, 2017

What the command does

Install

  • Grav
  • plugin (package)
  • theme (package)

from (.zip only?)

  • path
  • URL

@gnufred
Copy link
Contributor

gnufred commented Mar 29, 2017

@gnufred
Copy link
Contributor

gnufred commented Mar 29, 2017

@OleVik In the bin/gpm direct-install context, what would be the difference between --yes-all and --force. To me, both would do exactly the same, so one would be an alias.

@OleVik
Copy link
Contributor Author

OleVik commented Mar 29, 2017

The aliases of force and yes-all are f and y, respectively. force will "Force re-fetching the data from remote", whilst yes-all "Assumes yes (or best approach) instead of prompting".

Essentially, you can force retrying/refreshing data from the GPM and/or pass the generic yes to all asked questions, such as "Dependencies are not met, proceed? y|n?". See InstallCommand.php.

@gnufred
Copy link
Contributor

gnufred commented Mar 31, 2017

Look likes theres two var I need to check out in system/src/Grav/Console/Gpm/DirectInstallCommand.php.
$tmp_zip is downloaded or copied package folder containing the file $zip and $tmp_source is the extracted zip.

@gnufred
Copy link
Contributor

gnufred commented Mar 31, 2017

I don't think direct-install hit a cache. To what I see on GPM::downloadPackage and GPM::copyPackage in DirectInstallCommand.php it's copied and downloaded each time.
So the --force option will not be useful as of now.

Let's see what come out of this.

@gnufred
Copy link
Contributor

gnufred commented Mar 31, 2017

Installing an older Grav package delete the tmp folder and fails.

di https://github.com/getgrav/grav/releases/download/1.1.17/grav-v1.1.17.zip
#!/usr/bin/env php
Are you sure you want to direct-install https://github.com/getgrav/grav/releases/download/1.1.17/grav-v1.1.17.zip [y|N] y

Preparing to install https://github.com/getgrav/grav/releases/download/1.1.17/grav-v1.1.17.zip
  |- Downloading package...   100%
  |- Extracting package...    ok
  |- Checking destination...  ok
  |- Installing package...  
                                    
  [RuntimeException]                
  Cannot move non-existing folder.  
                                    

direct-install <package-file>

De-packaging 1.1.17 on branch develop probably cause this error.

This is probably cause by trying to install a stable release on the develop branch.

@gnufred
Copy link
Contributor

gnufred commented Mar 31, 2017

Following my tests with packages from this comment successful installation clean temporary files.
See Folder::delete here and here.

@gnufred
Copy link
Contributor

gnufred commented Mar 31, 2017

I can't get my functional test to work. Instantiating a null output when I call the Grav command is giving me trouble.

This doesn't help me: http://stackoverflow.com/questions/18237519/symfony2-console-output-without-outputinterface

I get the error

[Symfony\Component\Console\Exception\InvalidArgumentException] The "package" argument does not exist.

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

No branches or pull requests

3 participants