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

Run dlv from a temporary directory #3

Merged
merged 2 commits into from
Sep 12, 2017
Merged

Run dlv from a temporary directory #3

merged 2 commits into from
Sep 12, 2017

Conversation

arp242
Copy link

@arp242 arp242 commented Sep 11, 2017

This fixes two things:

  • My comments here:
    fatih@8d617ec#r138018608

    It's true that :lcd is only for the current window, but me
    personally I still wouldn't expect any user-visible changes in the
    current directory when using :GoDebugStart.

  • It prevents dlv from creating a binary in the directory. At best
    this is an annoying side-effect, but at worst it could cause people to
    lose files: consider what would happen if someone has a package named
    foo but also had a file named foo in that directory?
    If you always use go install (like I do) then this is fine, and
    :GoDebugStart creating this file can be rather surprising.

Note: this PR requires this patch to work correctly:
fatih#1435

@arp242
Copy link
Author

arp242 commented Sep 11, 2017

I think this approach is better @mattn? What do you think of it?

" current dir. We pass --wd= so the binary is still run from the current
" dir.
let original_dir = getcwd()
let tmp = fnamemodify(tempname(), ':h')
Copy link
Owner

Choose a reason for hiding this comment

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

let tempfile = tempname()
let tmpdir = fnamemodify(tempfile, ':h')
call delete(tempfile)

Copy link
Author

Choose a reason for hiding this comment

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

Alright, I'll change that in a minute 👍

Would you mind rebasing your godebug branch on the latest master btw? Then I have the bugfix for go#package#FromPath() so I can test it to make sure I didn't make a stupid mistake :-)

Copy link
Author

Choose a reason for hiding this comment

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

Looking at this again, I think your intent was to make sure that Vim creates a directory in /tmp/<random>? It already does that; the return value from tempname() is /tmp/<random>/<n>, where <random> seems stable for every Vim session and n is a sequential number.

So I think that the current version already does what you want? Write file to /tmp/<random>/<pkgname>?

Copy link
Owner

Choose a reason for hiding this comment

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

tempname() doesn't create directory (at least on Windows)

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry tempname() doesn't create file too.

Copy link
Owner

@mattn mattn Sep 11, 2017

Choose a reason for hiding this comment

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

How about this?

let tmpdir = filter([$TEMP, $TMP, $TMPDIR], '!empty(v:val)')[0] . '/' . sha256(fnamemodify(bufname(''), ":p"))
call mkdir(tmpdir, "p", 0700)

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I didn't know it behaves different on Windows. I updated it with something that should work on both platforms (but I didn't test on Windows, as I don't have a Windows computer!)

This fixes two things:

- My comments here:
  fatih@8d617ec#r138018608

  It's true that `:lcd` is only for the current window, but me
  personally I still wouldn't expect any user-visible changes in the
  current directory when using `:GoDebugStart`.

- It prevents `dlv` from creating a binary in the directory. At best
  this is an annoying side-effect, but at worst it could cause people to
  lose files: consider what would happen if someone has a package named
  `foo` but also had a file named `foo` in that directory?
  If you always use `go install` (like I do) then this is fine, and
  `:GoDebugStart` creating this file can be rather surprising.

Note: this PR requires this patch to work correctly:
fatih#1435
Since tempnam() seems to behave different on Linux and Windows.
function! go#util#tempdir(prefix) abort
" See :help tempfile
if go#util#IsWin()
let l:dirs = [$TMP, $TEMP, 'c:\tmp', 'c:\temp']
Copy link
Owner

Choose a reason for hiding this comment

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

please add $TMPDIR too.

Copy link
Owner

@mattn mattn Sep 12, 2017

Choose a reason for hiding this comment

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

Ah sorry. my mistake. Ignore this.

@mattn mattn merged commit 951b3be into mattn:godebug Sep 12, 2017
@arp242 arp242 deleted the debug-cd branch September 12, 2017 09:42
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.

2 participants