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

install phpactor with phpactor-update function #60

Merged
merged 1 commit into from
Oct 28, 2018

Conversation

kermorgant
Copy link
Contributor

@kermorgant kermorgant commented Sep 16, 2018

The recent evolutions in phpactor's rpc protocol made me realize that non BC changes could also break the correction operation of this package (ie version and force_reload new parameters).

Thus, I believe the simplest and safest approach to install phpactor for its use from phpactor.el is to have it installed from phpactor.el (with its version controlled in composer.json).

I would also qualify the use of an externally installed phpactor as risky, and therefore require the user to configure phpactor-executable manually in order to draw attention on this.

solves #59

@@ -84,11 +84,21 @@
"Return Phpactor command or path to executable."
(or (when phpactor-executable
(php-project--eval-bootstrap-scripts phpactor-executable))
(executable-find phpactor-command-name)
(let ((vendor-executable (f-join (phpactor-get-working-dir) "vendor/bin/phpactor")))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was a bit puzzled by this way of installing phactor (inside a php project), but even so, I'd rather recommend everyone to either install it from phpactor.el or define it manually (hopefully after having considered the risks it brings).

how does that sound ?

@kermorgant kermorgant changed the title add phpactor-install function and change phpactor-find-executable install phpactor with phpactor-install function Sep 16, 2018
@kermorgant
Copy link
Contributor Author

kermorgant commented Sep 16, 2018

Maybe we should clone phpactor's git repo before in order to provide a more accurate information to phpactor-status

image 3

EDIT: I asked @dantleech on slack and he's thinking of using composer for the versioning instead of git so maybe it's ok to keep the current way of doing

@kermorgant kermorgant changed the title install phpactor with phpactor-install function wip: install phpactor with phpactor-install function Sep 16, 2018
@kermorgant kermorgant force-pushed the installation branch 2 times, most recently from 1bc6edb to a2e6a2d Compare September 18, 2018 11:38
@kermorgant
Copy link
Contributor Author

renamed phpactor-install to phpactor-update. That way, it mimics the vim plugin and in the long run, the name will make more sense as user will have to re-run regularly.

@kermorgant kermorgant changed the title wip: install phpactor with phpactor-install function install phpactor with phpactor-update function Sep 19, 2018
@kermorgant
Copy link
Contributor Author

I'd like to have a way to run phpactor-update upon after installation or upgrade of this package, but I've not found anything in that regard.

But I suspect that if something existed, it would be something related to some package manager and that we should limit ourself to some configuration suggestion.

@kermorgant kermorgant requested a review from zonuexe September 19, 2018 07:39
phpactor.el Outdated
(setq default-directory package-folder)
(call-process "composer" nil (get-buffer-create phpactor-action--buffer-name) nil "install" "--no-dev")))

(defun phpactor-package-folder ()
Copy link
Member

Choose a reason for hiding this comment

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

Use directory instead of folder.

(error "Phpactor uses rpc protocol %s whereas this package requires %s" version phpactor--supported-rpc-version))
(if (phpactor-executable)
(error "Phpactor uses rpc protocol %s but this package requires %s" version phpactor--supported-rpc-version)
(error "Phpactor should be upgraded. Please run phpactor-update")))
Copy link
Member

Choose a reason for hiding this comment

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

These lines can be modified as follows, but what do you think?

@@ -469,10 +469,11 @@ function."
 ;; Dispatcher:
 (cl-defun phpactor-action-dispatch (&key action parameters version)
   "Execite action by `NAME' and `PARAMETERS'."
-  (when (and version (not (equal phpactor--supported-rpc-version version)))
-    (if (phpactor-executable)
+  (unless (or (null version)
+              (version<= phpactor--supported-rpc-version version))
+    (if (phpactor-find-executable)
         (error "Phpactor uses rpc protocol %s but this package requires %s" version phpactor--supported-rpc-version)
-        (error "Phpactor should be upgraded. Please run phpactor-update")))
+      (error "Phpactor should be upgraded.  Please run phpactor-update")))
   (phpactor--add-history 'phpactor-action-dispatch (list :action action :parameters parameters :version version))
   (let ((func (cdr-safe (assq (intern action) phpactor-action-table))))
     (if func

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, I'm not yet used to unless so it takes me a little bit more effort to read...

version<= phpactor--supported-rpc-version version

Not sure I see what you mean here. if versions are equal, no issue but if version is different than phpactor--supported-rpc-version version, I think we should stop with an error

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I see.

(when (file-exists-p vendor-executable)
vendor-executable))))

(defun phpactor-update ()
Copy link
Member

Choose a reason for hiding this comment

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

How about naming phpactor-setup instead of phpactor-update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I tend to like the semantic that comes with the term update as it conveys the idea that sooner or later, you'll have to run it again to stay up to date.

But I agree that for the first time you use it, it feels a bit wrong. Would that justify having both name, one aliasing towards the other ?

@zonuexe
Copy link
Member

zonuexe commented Sep 19, 2018

@kermorgant I think that this change is almost no problem. 👍

Perhaps it may be helpful for you, I just made this change to composer.el: https://github.com/emacs-php/composer.el/pull/4/files#diff-b6acbc3fae6805c6382033710ab67382R226

@kermorgant
Copy link
Contributor Author

@zonuexe thanks for the review. Indeed, ensuring composer is installed via composer.el seems good !

phpactor.el Outdated
(interactive)
(let ((package-folder (phpactor-package-directory)))
(setq default-directory package-folder)
(composer-ensure-exist-managed-composer-phar)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zonuexe I thought this would work but composer.phar was not installed in the expected folder (~.emacs.d/var)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just had a look at it and seems like installing composer programmatically on arch linux can be problematic (opened this issue)

=> This needs a bit different approach and a some error checking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not solve my issue about installing composer programmatically but I opted to use existing composer first and try to install it if it is not found.

@zonuexe I believe I answered all your comments. However, I'm not sure if dependency towards composer.el is properly defined

@ejmr
Copy link

ejmr commented Sep 23, 2018

Am I understanding correctly that the plan moving forward is for composer.el and phpactor.el to themselves install the appropriate versions of Composer and Phpactor?

@kermorgant
Copy link
Contributor Author

kermorgant commented Sep 24, 2018

@ejmr

Am I understanding correctly [...] ?

Yes.

I currently have some concerns about composer.el which sould be installed and enabled transparently as a dependency but that's not the case with my setup (maybe because I'm working with the sources of phpactor.el). Without that, I doubt composer.el should be used.

Regarding the installation of phpactor, I noticed that even minor changes in the rpc protocol (such as the addition of a 'version' information) would break some of phpactor.el's operations.

Having phpactor.el installing phpactor avoids such issues. Installation is also simpler that way imo.

@ejmr
Copy link

ejmr commented Sep 24, 2018

That all sounds reasonable and understandable. I feel like we should consider offering users a way to opt-out and use a global installation of Composer and/or Phpactor if possible, if only because I know some people will be certain to complain about their installation despite all good intentions.

@kermorgant
Copy link
Contributor Author

kermorgant commented Sep 25, 2018

There's already an opt-out solution for phpactor by defining the phpactor-executable variable.

And for composer, if existing composer executable is found, it will be used in priority.

@kermorgant
Copy link
Contributor Author

@zonuexe I changed small bits (fix for default directory during composer invocation) and added an explicit opt-out variable for composer installation. Do you see something still needing rework ?

@kermorgant
Copy link
Contributor Author

Having thought a bit more about the installation of composer through composer.el, I find it a bit questionable as phpactor's ideal use-case is for composer based projects.

So I'd assume a phpactor user to have composer installed globally. Therefrom, I'd just make the phpactor-update function fail if composer executable is not found in $PATH.

How does that sound ?

zonuexe
zonuexe previously approved these changes Oct 26, 2018
@zonuexe
Copy link
Member

zonuexe commented Oct 26, 2018

@kermorgant
I'm sorry for the late reply. We can merge this branch into master.

Add a phpactor-update function in order to manage Phpactor's
installation from this package.

As an alternative, let the user configure `phactor-executable` but
remove other ways of detecting phpactor.
@kermorgant
Copy link
Contributor Author

@zonuexe no worry, thx

@kermorgant kermorgant merged commit 08a81a1 into emacs-php:master Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants