Skip to content

Conversation

janlazo
Copy link
Contributor

@janlazo janlazo commented Aug 15, 2017

Windows releases for Vim seem to always include all patches so it's safe to check only 1 patch to assume the rest are included.

Reference: #666 (comment)

@junegunn
Copy link
Owner

I don't think the change is needed for non-windows environments.

@janlazo
Copy link
Contributor Author

janlazo commented Aug 15, 2017

What about patch-8.0.45 for non-windows?

@junegunn
Copy link
Owner

What does patch 45 fix? I'm not sure why we should change the condition. We have confirmed that the Installer using job API works with patch 39 both on non-Windows and on Windows environment (see #534, #541). #594 not working as expected is a separate problem. Anyway, we can conditionally set up s:vim8 depending on the environment if that's required.

let s:vim8 = exists('*job_start') && (s:is_win && has('...') || !s:is_win && has('patch-8.0.87'))

@janlazo
Copy link
Contributor Author

janlazo commented Aug 16, 2017

I choose those patches for #594 which seems to work on the patch-8.0.921 release for Windows.

What does patch 45 fix?

From the patch description in :h patches-8, it fixes job_stop for Vim 8.
Based on the comment for requiring patch-8.0.39 for Vim 8 jobs, I think it's worth considering.

Anyway, we can conditionally set up s:vim8 depending on the environment if that's required.

Simpler to require one more patch for Windows

let s:vim8 = exists('*job_start') && has('patch-8.0.39') && (!s:is_win || has('patch-8.0.87'))

@junegunn
Copy link
Owner

Thanks for clarification. I tried to separate two issues; Vim 8 installer and s:system using job API (#594).

  1. Vim 8 installer works on Vim 8 with patch 39 or above
  2. s:system using job API seems to require patch 45 or above

Changing the version requirement from 39 to 45 will make Vim 8 installer suddenly unavailable on a set of Vim versions (patch 39 ~ 44) even they are capable of running it. That was the issue I wanted to address. However, I guess it's not worthwhile to put more if-else statements just for that small range of versions. I'm no longer against changing the requirement.

Having said that, this should be merged with #594 (or equivalent), we don't have to merge it before it.

@janlazo janlazo closed this Aug 23, 2017
@janlazo janlazo deleted the vim8-patch branch August 23, 2017 03:39
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