Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for waf build system. #16

Merged
merged 3 commits into from
Feb 1, 2017
Merged

Conversation

haaspors
Copy link
Contributor

This patch also adds extra checking for bundled waf script.

This patch also adds extra checking for bundled waf script.
Copy link
Owner

@johnsyweb johnsyweb left a comment

Choose a reason for hiding this comment

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

Thanks for this submission. I have some questions around the bundled waf discovery.

Could you also please wscript to the README and online documentation?

@@ -19,6 +19,7 @@ function! s:build_defaults()
\'mix.exs': 'mix',
\'pom.xml': 'mvn',
\'build.ninja': 'ninja',
\'wscript': 'waf',
Copy link
Owner

Choose a reason for hiding this comment

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

if filereadable(l:found)
let l:program = l:found
endif
endif
Copy link
Owner

Choose a reason for hiding this comment

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

I have some questions around this:

  • Should finding a bundled waf be a concern of Makeshift or should it be handled by the OS path?
  • If finding a bundled program is a concern of this plugin, should this functionality to be specific to waf or could we remove that condition?
  • If finding a bundled program is a concern of this plugin, does it deserve its own function?
  • If finding a bundled program is a concern of this plugin, should there be a configuration option to disable it?

Copy link
Owner

Choose a reason for hiding this comment

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

I've given this some more thought and can see the benefits of finding (e.g) /path/to/project/bin/rake for a Ruby project that bundles rake. I can also see a use-case where somebody working on a large C++ project might want to disable the search for make.

So... I think (to answer my own questions) what we want here is something like (untested)...

diff --git a/plugin/makeshift.vim b/plugin/makeshift.vim
index c5ba840..ca5a59e 100644
--- a/plugin/makeshift.vim
+++ b/plugin/makeshift.vim
@@ -58,9 +58,21 @@ function! s:determine_build_system(dir)
     return ''
 endfunction
 
+function! s:find_bundled(program)
+    let l:bundled = globpath(b:makeshift_root, a:program)
+    if filereadable(l:bundled)
+        return l:bundled
+    endif
+    return a:program
+endfunction
+
 function! s:set_makeprg(program)
     if len(a:program)
-        let &l:makeprg=a:program
+        if exists('g:makeshift_find_bundled') && g:makeshift_find_bundled
+            let &l:makeprg = s:find_bundled(a:program)
+        else
+            let &l:makeprg = a:program
+        endif
     endif
 endfunction
 
@@ -88,12 +100,6 @@ function! s:makeshift()
     if len(l:program) == 0
         let l:program = s:determine_build_system(expand('%:p:h'))
     endif
-    if l:program == 'waf'
-        let l:found = globpath(b:makeshift_root, l:program)
-        if filereadable(l:found)
-            let l:program = l:found
-        endif
-    endif
     call s:set_makeprg(l:program)
     call s:set_makedir(len(l:program) > 0)
 endfunction

I don't have the capacity to test or document this right now. Do you want to have a go at this, @ieei?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback! A bit of background; I'm working on multiple repositories/libraries with different build systems. Most waf based libraries bundle waf, but I also have waf installed on my system and in some cases waf isn't bundled.

Anyways, I found makeshift to increase my productivity switching between repositories, rather than modifying makeprg myself. And I just quickly hacked this in to make it work for me. I do like your approach better, and will test it today. Thanks again!

This patch removes the specific find bundled waf functionality and
rather adds generic support for finding bundled build program/script.

Whether to find bundled make program is disabled by default, but can be
enabled by adding `let g:makeshift_find_bundled = 1` to `vimrc`.
@haaspors
Copy link
Contributor Author

haaspors commented Feb 1, 2017

Added your patch and updated README. It works as expected. Tested manually and works for me at least 😄

Also add WAF to the supported build systems list.
See https://waf.io
@johnsyweb johnsyweb merged commit e2ab206 into johnsyweb:master Feb 1, 2017
@johnsyweb
Copy link
Owner

Thanks very much for this, @ieei. I've released 0.15.0.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants