- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.8k
 
Add local_program() again #15133
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
base: master
Are you sure you want to change the base?
Add local_program() again #15133
Conversation
17307ad    to
    e1a7ed3      
    Compare
  
    | 
           @jpakkane @eli-schwartz can you reviews this please?  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said previously:
Adding a new toplevel function which is an existing function with very minor behavior differences does not seem warranted to me. The main justification for a new function appears to be simply that it won't searchh PATH, but this is redundant with passing a files() to find_program(). I think this is a bad reason to add a new toplevel function.
Design wise, depends: and interpreter could just as easily be added to find_program directly, and there are potential use cases for the former even for programs found via PATH.
But I also think that this is unnecessary to allow interpreter as a kwarg at all. Existing users always pass an array around to things like
my_gen = [python, find_program(files('my_gen.py'))]
custom_target(command: my_gen + [....])We should work with that and allow array overrides directly to meson.override_find_program(). I believe that will be a lot more intuitive than having "split-brain" interfaces and new toplevel functions depending on whether the result is intended to be passed to override_find_program.
(As a bonus, it also means for example that you can do override_find_program in an if meson.version().version_compare, the same way override_dependency was "simple to support" in a backwards compatible manner. That doesn't necessarily mean that I think new design should be based on what's easier to write backports using -- I just think it's very convenient that the thing I believe is a better design anyway, is also easier to use with backports.)
Passing an array to override_find_program means we can easily support interpreter options, which your original design didn't allow at all. I see that this new version does allow it, but it looks forced in and you specifically have to document that it works despite the obvious assumption (and the kwarg name implies it doesn't work, which is awkward UX). You still don't support arguments after the program, which prevents some use cases that would "automatically work" when just allowing to export an array, and we wouldn't have to take an opinionated stance about whether users should be allowed to do it. I can think of reasons why it would be perfectly fine to do, but it seems like you want a top-level function with an interpreter kwarg in part just because you want to ban something that is a "niche power tool" that you are afraid people will use in ways you find inelegant, and they could already do except more difficult. I don't see the point of this limitation.
...
By the way I would still like to see this support CustomTarget first, which I think we all agree should be made to work, and which you said this refactoring could be in support of instead. That would allow us to merge the groundwork independently and make it easier to reason about the scope of this proposal.
          
 +1 on this, even though I find using find_program for local scripts confusing and I would rather avoid adding extra functionality to find_program, or relying on little known tricks like always passing files().  | 
    
| 
           TBH at this point I don't want to work on this anymore. I completely disagree and don't want to waste my time on bikeshedding again. Merge whatever you want.  | 
    
Reapply patchset from #15107.