search

mesonbuild / meson

The Meson Build System
http://mesonbuild.com
Apache License 2.0
5.59k stars 1.63k forks source link

install_man does not accept target output #1550

Open keszybz opened 7 years ago

keszybz commented 7 years ago 
  t1 = custom_target(
      'bootup.1',
      input : 'bootup.xml',
      output : 'bootup.1',
      command : [xsltproc, '-o', '@OUTPUT@'] + xsltproc_flags + [custom_man_xsl, '@INPUT@'])
  install_man(t1)
Meson encountered an error in file man/meson.build, line 210, column 2:
Arguments must be strings.

I would have expected this to work...

jpakkane commented 7 years ago

In Meson we try to keep as much information about targets in one place. Whether and how to install should be defined in the target declaration command as is done for all other target types. So something like this:

custom_target(..., install : true, install_dir : join_paths(get_option('mandir'), 'man1')

Otherwise you could get into strange cases such as having a target that has install : false but which is used in an explicit install command. The outcome of this is ambiguous.

keszybz commented 7 years ago

I don't see this as ambiguous: the outcome of a target can be used as input to another target or targets, and each can have a separate install, install_dir setting. I can do this with any generated file, and man pages should be no different.

Open-coding install_dir requires a reimplementation of the extracting of man page directory. It's possible, but annoying, especially when the man page name and section comes from some list and must be extracted by splitting strings and such.

keszybz commented 7 years ago

... in other words, install : true/false pertains to the rule, not to the (potentially one of many) output file.

nirbheek commented 7 years ago

For multiple outputs of a custom target, we have a PR open to implement that already: https://github.com/mesonbuild/meson/pull/1469

realh commented 6 years ago

AIUI this means you can't use install_mans if your manpages are generated eg from XML. Generated manpages are very common, and install_mans does useful stuff that the install parameter of custom_target doesn't. So it would help a lot if custom_target had some sort of install_function parameter.

bredelings commented 6 years ago

I ran into this issue generating manpages with pandoc. I guess differences are:

Should we add a note the documentation of install_man( ) that for generated man pages we should set install: true on a custom_target( ) instead of using install_man( )? This could also go in the 'How do I do X with meson' section, I suppose.

I suppose install_headers( ) has some of the same issue.

FWIW, here is my code for pandoc. Any suggestions or comments appreciated:

pandoc = find_program('pandoc', required: false)

if pandoc.found()
  foreach prog : all_progs
    markdown = files('man/'+prog+'.md')
    manpage = prog+'.1'
    custom_target(manpage,command:[pandoc,'-s','-t','man',markdown], output:manpage, capture: true, install: true, install_dir: join_paths(get_option('mandir'),'man1'))
  endforeach
else
  warning('Program "pandoc" not found!  Cannot generate man pages')
endif
dcbaker commented 5 years ago

I think we should implement this. We allow passing custom_targets to install_data, so we should do it for install_man as well.

eli-schwartz commented 4 years ago

This is even worse now, because if you try it you get a gigantic traceback:

Traceback (most recent call last):
  File "/usr/lib/python3.8/site-packages/mesonbuild/mesonmain.py", line 131, in run
    return options.run_func(options)
  File "/usr/lib/python3.8/site-packages/mesonbuild/msetup.py", line 245, in run
    app.generate()
  File "/usr/lib/python3.8/site-packages/mesonbuild/msetup.py", line 159, in generate
    self._generate(env)
  File "/usr/lib/python3.8/site-packages/mesonbuild/msetup.py", line 192, in _generate
    intr.run()
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreter.py", line 4359, in run
    super().run()
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 441, in run
    self.evaluate_codeblock(self.ast, start=1)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 466, in evaluate_codeblock
    raise e
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 459, in evaluate_codeblock
    self.evaluate_statement(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 482, in evaluate_statement
    return self.evaluate_if(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 563, in evaluate_if
    self.evaluate_codeblock(i.block)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 466, in evaluate_codeblock
    raise e
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 459, in evaluate_codeblock
    self.evaluate_statement(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 472, in evaluate_statement
    return self.function_call(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 829, in function_call
    return func(node, func_args, self.kwargs_string_keys(kwargs))
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 326, in wrapped
    return f(*wrapped_args, **wrapped_kwargs)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 212, in wrapped
    return f(*wrapped_args, **wrapped_kwargs)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreter.py", line 3881, in func_subdir
    self.evaluate_codeblock(codeblock)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 466, in evaluate_codeblock
    raise e
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 459, in evaluate_codeblock
    self.evaluate_statement(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 504, in evaluate_statement
    self.evaluate_foreach(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 737, in evaluate_foreach
    self.evaluate_codeblock(node.block)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 466, in evaluate_codeblock
    raise e
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 459, in evaluate_codeblock
    self.evaluate_statement(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 482, in evaluate_statement
    return self.evaluate_if(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 563, in evaluate_if
    self.evaluate_codeblock(i.block)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 466, in evaluate_codeblock
    raise e
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 459, in evaluate_codeblock
    self.evaluate_statement(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 472, in evaluate_statement
    return self.function_call(cur)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 829, in function_call
    return func(node, func_args, self.kwargs_string_keys(kwargs))
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 326, in wrapped
    return f(*wrapped_args, **wrapped_kwargs)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreterbase.py", line 212, in wrapped
    return f(*wrapped_args, **wrapped_kwargs)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreter.py", line 3834, in func_install_man
    m = Man(fargs, kwargs)
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreter.py", line 734, in __init__
    self.validate_sources()
  File "/usr/lib/python3.8/site-packages/mesonbuild/interpreter.py", line 743, in validate_sources
    num = int(s.split('.')[-1])
AttributeError: 'CustomTargetHolder' object has no attribute 'split'
eli-schwartz commented 3 years ago

I dislike the necessity of it, but... I have tried my hand at another solution: #9342

This time, the idea is to use:

install_man(
      'bootup.1',
      input : 'bootup.xml',
      command : [xsltproc, '-o', '@OUTPUT@'] + xsltproc_flags + [custom_man_xsl, '@INPUT@'])

Basically duplicating the custom_target interface inside install_man.

The purported benefit of not just using install_man(custom_target(...))is that we don't have the "inconsistency" of either defining install: true or passing its file output to an install_*() function.

So install_man naturally only supports command: but not install: true...

keszybz commented 3 years ago

So install_man naturally only supports command: but not install: true...

I think this is a step in an unnecessary direction. Based on my experience, having an boolean install option on all rules would be useful. Right now various targets like like executable, library, custom_target, … have it. But some other rules like install_data and install_man do not, and this leads to very verbose condtionals:

if get_option('conf-files') and get_option('enable-foobar'):
     install_data('foobar.conf', install_dir: …)
endif

It would like to replace this by

     install_data('foobar.conf', install_dir: …,
                        install: get_option('conf-files') and get_option('enable-foobar'))

to match what I can do for the executable itself.

And the same for man pages: an internal install switch would be very welcome.

Basically duplicating the custom_target interface inside install_man.

Hmm, this seems like a fairly heavyweight solution that is not very flexible.

The purported benefit of not just using install_man(custom_target(...))is that we don't have the "inconsistency" of either defining install: true or passing its file output to an install_*() function.

Well, if this is such a problem, why not check and refuse this particular scenario (custom_target with install:true passed to install_man or install_data) ?

eli-schwartz commented 3 years ago

To check and refuse your particular scenario would not appease the people (who are not me) who don't want the UI inconsistency of adding a second, alternative, route to install a custom target.

It would only error out if someone tried to use both at the same time, which was never the concern of those-people-who-are-not-me.

I'm pretty sure it's perfectly fine to install the same source file twice (to different locations which IIRC is enforced by the installdata rather than by functions). The design objection that was examined to me is that somehow built files are magical and all information (including install location), for philosophical purity reasons, has to be passed from meson.build into the interpreter by the narrow interface of a single function call.

Given those constraints, this seemed like the easiest and most logical route to bowing to the will of said design constraints from on high, and implementing both functionalities (build a built file, install a manpage) in a single function call interface.

eli-schwartz commented 3 years ago

Based on my experience, having an boolean install option on all rules would be useful.

Does your experience take into account the existence of install tags? It's a new feature so I expect not.

keszybz commented 3 years ago

Does your experience take into account the existence of install tags? It's a new feature so I expect not.

I wasn't aware of install tags. But it seems they won't work for this purpose: we don't want to force install --tags to be used in all cases, but instead want to completely skip some build targets (exes, man pages, data files, etc) based on configuration from the default build and installation. IIUC, we could do something like install_tag : get_option('conf-files') and get_option('enable-foobar') ? 'runtime' : 'skip', and ask people to do --install runtime,devel,man,doc,i18n,typelib, but that seems rather annoying.

(That said, install tags seem quite useful for other cases. We currently have a configuration option -Dinstall-tests=true/false to enable/disable the installation of test binaries and data. This was done this way only because meson had only one install target. I think it can be replaced nicely by install --tags tests.)

--

Returning to the main topic:

I'm pretty sure it's perfectly fine to install the same source file twice

Yes. It's not something that'd be done normally, but if one wants or needs to, why not.

OK, so I disliked the approach in #9342 because it duplicates a lot of complexity. I would prefer to be able to compile smaller functionality blocks in arbitrary ways. But both approaches would work…

@jpakkane can we have another opinion on the design here?

eli-schwartz commented 3 years ago

If you want to configure some targets to not be built at all, because they are disabled per configuration, then I think what you're getting at here is that you want to not generate the rules at all... you don't want to build them either. ;)

So, adding an if block around the definition is probably correct. Either that, or using the configuration to cause the build command to be a disabler object, which is semantically like an if false block but makes the conditional simpler.

That would mean the executables aren't built either, if they are disabled by a disabler. Currently it seems like you're suggesting they would be built by default, but not installed?