On Tue, 12 Dec 2017, at 07:00 PM, Felipe Sateler wrote:
> On Sun, Dec 10, 2017 at 4:16 AM, Arun Raghavan <arun at arunraghavan.net>
> wrote:
> > On Sat, 9 Dec 2017, at 10:58 PM, Felipe Sateler wrote:
> >> On Sat, Dec 9, 2017 at 2:59 AM, Arun Raghavan <arun at arunraghavan.net>
> >> wrote:
> >> > +
> >> > +apiversion = '1.0'
> >> > +soversion = 0
> >> > +# maintaining compatibility with the previous libtool versioning
> >> > +# current = minor * 100 + micro
> >> > +libversion = '@0 at .@1 at .0'.format(soversion, pa_version_minor.to_int() * 100 + pa_version_micro.to_int())
> >>
> >> This gives 0.9901.0 instead of 0.20.2
> >
> > Yeah, we need to fix it up to do the right thing. Let's fix this in a
> > subsequent patch.
>
> Well, then the comment should be prefixed with FIXME or removed, since
> the code does not do what the comment says :)
I'm not sure what the right fix is, so in the mean time I'm going to
make a FIXME of it.
> > My intention is to not have this be complete, but to have it land in
> > some usable state and incrementally make it better. I expect we'll be
> > supporting autofoo + meson for a while until there is parity between the
> > two (across platforms).
>
> I agree that perfect is enemy of the good. Merging the meson support
> as incomplete is fine.
>
> I'm struggling to find a way to phrase this, but I think outputting a
> different artifact (as opposed to a less featureful, or missing
> artifact) is a bug, and not a missing feature.
I agree that it's a bug, and I think that's okay to fix subsequently if
it's something non-trivial. In the case of the above, we should figure
out what's needed to not break clients, and how other projects are
handling this.
> >> > +
> >> > +if cc.has_function('SYS_memfd_create', prefix : '#include <sys/syscall.h>')
> >> > + cdata.set('HAVE_MEMFD', 1)
> >> > +endif
> >>
> >> configure has the option to enable or disable this. This meson script
> >> autodetects only. I think the ability to explicitly disable/enable
> >> (and error out if dependencies are not found) is nice to keep when
> >> moving to meson. That is, this should become something like:
> >>
> >> want_memfd = get_option('memfd')
> >> if want_memfd != 'false'
> >> has_memfd = cc.has_function('SYS_memfd_create', prefix : '#include
> >> <sys/syscall.h>')
> >> if has_memfd
> >> cdata.set('HAVE_MEMFD', 1)
> >> elif want_memfd == 'true'
> >> error('Memfd was requested but it was not found')
> >> endif
> >> endif
> >>
> >> plus a meson_options.txt:
> >>
> >> option('memfd', type: 'combo', choices: ['auto', 'true', 'false'],
> >> description: 'Enable Linux memfd shared memory')
> >>
> >> This applies to a lot of the checks.
> >
> > This may be a hard, but in the long run, I actually would like to remove
> > automagic dependencies. This makes builds more consistent, and removes a
> > lot of (imo unnecessary) if/else-ery.
>
> I'm not at all opposed to this. But then the basic pattern should be
> in place. This way, when new features are ported from autotools to
> meson, the pattern can be copied.
Exactly.
> >
> > So we would have want_memfd on by default (maybe conditional on OS ==
> > Linux), and then you could disable it at configure time if you don't
> > want it.
>
> Unfortunately I don't think meson allows OS-conditional options so it
> will have to be implemented as if-else too (although it is likely it
> is less than autodetection).
>
> >> > +pax11publish_sources = [
> >> > + 'pax11publish.c',
> >> > +]
> >> > +
> >> > +# FIXME: man pages
> >> > +executable('pax11publish',
> >> > + pax11publish_sources,
> >> > + install: true,
> >> > + include_directories : [configinc, topinc],
> >> > + link_with : [libpulsecommon, libpulse],
> >> > + dependencies : [x11_dep],
> >> > + c_args : pa_c_args,
> >> > +)
> >>
> >> Shouldn't this be conditional on x11_dep being found?
> >
> > Yes, that makes sense. I wonder if meson automatically won't build a
> > target whose dep is not found.
>
> I don't think so. Otherwise conditionally enabling features would be
> very annoying:
>
> exe_deps = [required_dep]
> if optional_dep.found()
> exe_deps += [option_dep]
> endif
> # etc
So we'd basically wrap the entire executable() in if x11_dep.found(), I
guess.
-- Arun
