Fix TRAMP issues #1129

Silex · 2017-03-23T11:16:09Z

This should fix #523 and #835.

Don't merge this yet (it lacks changelogs, etc). Right now the idea is for people having the same problem to test if my fix actually fixes all scenarios.

The idea is pretty simple: only resolve project-root for local files, or when the remote files are connected.

So far this seems to resolve all the issues for me, but I only tested emacs 25.1.

Things to test:

/scpx:user@host:somefile
/sudo:root@localhost:somefile

Note: by me, the issues only happened when projectile-rails-global-mode was turned on. Projectile alone worked pretty well. This is because projectile-rails-mode uses projectile to determine wether it'd turn on or not, and this was just too much for TRAMP.

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (make test)
The new code is not generating bytecode or M-x checkdoc warnings
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the readme (if adding/changing user-visible functionality)

Silex · 2017-03-23T11:21:56Z

@timvisher, @2mc, @tonycpsu, @KGZM, @ackerleytng, @IvanMalison, @spaceotter, @stardiviner, @puffnfresh, @seagle0128, @bhrgunatha, @binarin: please test and report.

timvisher · 2017-03-23T12:04:32Z

Whoa! Nice! :)

I'll try to test this out today!

bbatsov · 2017-03-23T12:24:14Z

projectile.el

-         projectile-project-root-files-functions)
+  (let* ((dir default-directory)
+         (is-local (not (file-remote-p dir)))
+         (is-connected (file-remote-p default-directory nil t)))


What doest this mean?

huh? is-local is true if the file is local, is-connected is true if the file is remote and we are already connected to the remote.

TRAMP's problems occur when we try to get file informations but we are just not quite connected yet.

Ah, the second default-directory should be dir, I'll make a fixup commit

@bbatsov: I don't really understand your original question, can you clarify?

bhrgunatha · 2017-03-23T13:21:24Z

I'm on emacs 25.1 now - tramp is working correctly for me now with projectile-mode (also looks OK with projectile-rails-mode too)

Silex · 2017-03-23T13:40:37Z

@bhrgunatha: with or without my patch? Also, to be exact it's projectile-rails-global-mode that triggers the projectile bug, both with /sudo: and /scpx:.

timvisher · 2017-03-23T14:28:58Z

$ git log -1
commit 6dd3cca8c5d3b321c9c206967cf32985103eff73
Author: Philippe Vaucher <[email protected]>
Date:   Thu Mar 23 13:39:15 2017 +0100

    fixup! Fix TRAMP issues

GNU Emacs 25.1.2 (x86_64-apple-darwin15.6.0) of 2016-09-19

Cab's running fine… Purring like a kitten…

(Supergreen)

(Meaning it works great for me :) )

tonycpsu · 2017-03-24T01:20:57Z

Looks good here on Emacs 24.5.

bhrgunatha · 2017-03-24T04:31:46Z

@Silex
Sorry I wasn't clear and I'm afraid this won't be very helpful either.
Without any patches applied after moving to 25.1, tramp is working without any other direct intervention needed from me.

Silex · 2017-03-25T09:37:33Z

Okay, I discovered that it's possible to update TRAMP itself. I'm building a test matrix based on:

Projectile with or without my fix
Emacs 24.5 or 25.1
Projectile-Rails on or off
TRAMP default or latest version

I'll report when it's done.

seagle0128 · 2017-03-25T10:04:19Z

Works well on Emacs 25.1.

Acked-by: Vincent Zhang <[email protected]>

Silex · 2017-03-25T20:21:22Z

Okay, here are my tests so far:

Version	Projectile branch	Projectile Rails?	TRAMP latest?	Failed
24.5	master	no	no	0
24.5	master	no	yes	0
24.5	master	yes	no	1
24.5	master	yes	yes	1
24.5	repair-tramp	no	no	0
24.5	repair-tramp	no	yes	0
24.5	repair-tramp	yes	no	1
24.5	repair-tramp	yes	yes	0
25.1	master	no	no	0
25.1	master	no	yes	0
25.1	master	yes	no	1
25.1	master	yes	yes	1
25.1	repair-tramp	no	no	0
25.1	repair-tramp	no	yes	0
25.1	repair-tramp	yes	no	0
25.1	repair-tramp	yes	yes	0

We see that my branch repairs 3 tests out of 4, and that by updating TRAMP to latest version we get the last failing test.

The tests in their current form are a bit of a pain to run (they need manual intervention), and I doubt they can actually become part of projectile's test suite due to the heavy setup required. Maybe a second travis-ci just for them might work tho. For the brave who like to look at work in progress, look at https://github.com/Silex/projectile/tree/tramp-tests/bugs. One of the issues I have is that with-timeout actually stops working when the bug happen, so I cannot use that to detect failure. Then when I start using bash's timeout function everything starts failing (docker does not start). Also, I'm not sure how to feed the password to TRAMP programmatically. Oh well.

p.s: one surprising finding is that by running emacs with --batch almost all the tests passes! TRAMP is a little sensitive drama queen 😄

Silex · 2017-03-30T07:40:01Z

@bbatsov: I'd like to hear from you. I don't think I'll progress much more on this issue.

Here is the current status:

I don't think my patch can really hurt anyone. Maybe performances a bit?
More than one person reported it fixed the problem for them.
Writing tests for this is complicated, I'm not sure it's automatable. Also as I said I'm not even sure you'd want these tests inside the test suite, so I think I'll stop working on this.

For information, here is the latest update with the TRAMP guys: http://lists.gnu.org/archive/html/bug-gnu-emacs/2017-03/msg00979.html

Basically they say it makes sense to test for what I test, but that there's not much they can do. Emacs 26 might fix the issue, but so far with my tests the bug is still present.

Silex · 2017-04-05T19:47:14Z

@bbatsov: ping :-)

bbatsov · 2017-04-06T06:04:48Z

I'm extremely busy, so I don't really have time to go through the linked issues. What exactly is this supposed to fix. Frankly, I'm getting a bit annoyed from having to complicate the code base more and more and more on behalf of TRAMP, which I don't even use. I still don't get while someone would be developing over TRAMP...

So, briefly, what is this supposed to fix?

P.S. Just looking at the patch I find it extremely odd there's a branch in the logic where we don't even attempt the find the project root.

Silex · 2017-04-06T08:24:55Z

So, briefly, what is this supposed to fix?

It fixes the behavior where Emacs hangs because of projectile when you open a file over TRAMP. When Emacs hangs, very often the only solution is to kill emacs from another terminal.

The fix can be summarized as follow:

"Stop trying to find information about files for which it's not possible to get informations from."

P.S. Just looking at the patch I find it extremely odd there's a branch in the logic where we don't even attempt the find the project root

This is exactly what the fix does: don't try to find the project root for remote files, if we are not connected to the remote host yet.

Here is what currently happens:

Open a file
An empty buffer is created (and TRAMP starts connecting)
Projectile want to display project name and asks questions about the file (way too soon)
TRAMP is confused and hangs

Here is what my fix changes:

Open a file
An empty buffer is created (and TRAMP starts connecting)
Projectile want to display project name (and gets told that no project root exists)
TRAMP eventually connects
File is displayed
Projectile want to display project name (now that it is connected it gets the right answer)

bbatsov · 2017-04-06T08:37:17Z

projectile.el

-                 value))))
-         projectile-project-root-files-functions)
+  (let* ((dir default-directory)
+         (is-local (not (file-remote-p dir)))


I'd add what you answered to me in the issue here as a comment. Otherwise people are going to have a hard time figuring out what does this do.

Good idea! Doing it now.

bbatsov · 2017-04-06T08:38:46Z

Apart from my small remark - this needs a changelog entry.

Silex · 2017-04-06T08:52:10Z

Does the changelog entry and the fixup commit that adds comments work for you?

I'm not sure how detailed the comments needs to be.

bbatsov · 2017-04-06T09:33:22Z

CHANGELOG.md

@@ -9,6 +9,7 @@

 ### Changes

+* [#1129](https://github.com/bbatsov/projectile/pull/1129) Fix TRAMP issues.


There's a missing : here.

bbatsov · 2017-04-06T09:33:39Z

projectile.el

-                 value))))
-         projectile-project-root-files-functions)
+  ;; The `is-local' and `is-connected' variables are used to fix the behavior where Emacs hangs
+  ;; because of Projectile when you open a file over TRAMP. It basically prevents projectile from


projectile -> Projectile

bbatsov · 2017-04-06T09:34:11Z

projectile.el

-         projectile-project-root-files-functions)
+  ;; The `is-local' and `is-connected' variables are used to fix the behavior where Emacs hangs
+  ;; because of Projectile when you open a file over TRAMP. It basically prevents projectile from
+  ;; trying to find information about files for which it's not possible to get informations from.


informations from -> information

bbatsov · 2017-04-06T09:34:56Z

Added a couple of tiny remark - squash and rebase after you apply them.

Silex · 2017-04-06T13:07:40Z

Alright, reworded a little bit more, this should be good to go.

Silex · 2017-04-07T12:02:49Z

👍

timvisher · 2017-04-10T12:25:58Z

@bbatsov The biggest reason to develop over tramp is to develop on ephemeral resources like vagrant boxes or ec2 instances without loosing any of your context should you need to replace the ephemeral resource.

see: bbatsov/projectile#1129

Silex force-pushed the repair-tramp branch from d783c69 to cb9b2f5 Compare March 23, 2017 11:25

Silex changed the title ~~Fix tramp issues~~ Fix TRAMP issues Mar 23, 2017

bbatsov reviewed Mar 23, 2017

View reviewed changes

seagle0128 added a commit to seagle0128/.emacs.d that referenced this pull request Mar 25, 2017

Fix tramp issues: bbatsov/projectile#1129

7ebb236

Acked-by: Vincent Zhang <[email protected]>

Silex force-pushed the repair-tramp branch from 6dd3cca to d2748dc Compare March 30, 2017 07:33

Silex mentioned this pull request Apr 3, 2017

projectile-global-mode prevents new processes from working over TRAMP #523

Closed

bbatsov reviewed Apr 6, 2017

View reviewed changes

Silex force-pushed the repair-tramp branch from 313b92f to 0ef9963 Compare April 6, 2017 08:43

bbatsov reviewed Apr 6, 2017

View reviewed changes

Fix TRAMP issues

814e97a

Silex force-pushed the repair-tramp branch from f958fed to 814e97a Compare April 6, 2017 13:04

bbatsov merged commit 323fb84 into bbatsov:master Apr 7, 2017

Silex deleted the repair-tramp branch April 7, 2017 12:02

seagle0128 added a commit to seagle0128/.emacs.d that referenced this pull request Apr 8, 2017

Already merged: tramp issues bbatsov/projectile#1129

4c59e03

djr7C4 mentioned this pull request Mar 23, 2018

smart-mode-line breaks tramp in term-mode Malabarba/smart-mode-line#222

Closed

mjhoy added a commit to mjhoy/dotfiles that referenced this pull request Apr 3, 2018

emacs: projectile: enable caching; tramp issues appear to be fixed

83b1f4e

see: bbatsov/projectile#1129

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TRAMP issues #1129

Fix TRAMP issues #1129

Silex commented Mar 23, 2017 •

edited

Loading

Silex commented Mar 23, 2017

timvisher commented Mar 23, 2017

bbatsov Mar 23, 2017

Silex Mar 23, 2017 •

edited

Loading

Silex Mar 23, 2017

Silex Mar 23, 2017

bhrgunatha commented Mar 23, 2017 •

edited

Loading

Silex commented Mar 23, 2017

timvisher commented Mar 23, 2017 •

edited

Loading

tonycpsu commented Mar 24, 2017

bhrgunatha commented Mar 24, 2017

Silex commented Mar 25, 2017

seagle0128 commented Mar 25, 2017

Silex commented Mar 25, 2017 •

edited

Loading

Silex commented Mar 30, 2017

Silex commented Apr 5, 2017

bbatsov commented Apr 6, 2017

Silex commented Apr 6, 2017 •

edited

Loading

bbatsov Apr 6, 2017

Silex Apr 6, 2017

bbatsov commented Apr 6, 2017

Silex commented Apr 6, 2017

bbatsov Apr 6, 2017

bbatsov Apr 6, 2017

bbatsov Apr 6, 2017

bbatsov commented Apr 6, 2017

Silex commented Apr 6, 2017

Silex commented Apr 7, 2017

timvisher commented Apr 10, 2017

		@@ -9,6 +9,7 @@

		### Changes

		* [#1129](https://github.com/bbatsov/projectile/pull/1129) Fix TRAMP issues.

Fix TRAMP issues #1129

Fix TRAMP issues #1129

Conversation

Silex commented Mar 23, 2017 • edited Loading

Silex commented Mar 23, 2017

timvisher commented Mar 23, 2017

Choose a reason for hiding this comment

Silex Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhrgunatha commented Mar 23, 2017 • edited Loading

Silex commented Mar 23, 2017

timvisher commented Mar 23, 2017 • edited Loading

tonycpsu commented Mar 24, 2017

bhrgunatha commented Mar 24, 2017

Silex commented Mar 25, 2017

seagle0128 commented Mar 25, 2017

Silex commented Mar 25, 2017 • edited Loading

Silex commented Mar 30, 2017

Silex commented Apr 5, 2017

bbatsov commented Apr 6, 2017

Silex commented Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Apr 6, 2017

Silex commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Apr 6, 2017

Silex commented Apr 6, 2017

Silex commented Apr 7, 2017

timvisher commented Apr 10, 2017

Silex commented Mar 23, 2017 •

edited

Loading

Silex Mar 23, 2017 •

edited

Loading

bhrgunatha commented Mar 23, 2017 •

edited

Loading

timvisher commented Mar 23, 2017 •

edited

Loading

Silex commented Mar 25, 2017 •

edited

Loading

Silex commented Apr 6, 2017 •

edited

Loading