That's why there's https://git.gnome.org/browse/gtk-osx/

Three years ago when I brought that project into Gnome I asked if I should integrate the modulesets into jhbuild and Fred said that he didn't want jhbuild to become a super-repository for separate modulesets, so the Mac modulesets live over there.

You're wasting a lot of time, both yours and other people's, by reinventing that particular wheel.

Considering my main goal here is to get GNOME building regularly on the free software BSDs and maybe OpenIndiana, I'm not sure how gtk-osx helps... We could do this sort of separate repository for every platform that we're interested in supporting, but I don't think that's very helpful (or sustainable) in the long run.  The benefit of having things automatically kept in sync by using the same moduleset is substantial.

See https://wiki.gnome.org/Projects/Jhbuild/FreeBSD for an example of what I'm trying to avoid.

Created attachment 264662 [details] [review]
add 'conditions_set' to the config file

It is called 'set' because it is a set().

Also: pre-seed a couple of conditions into it, based on the platform
that we find ourselves running on (by way of sys.platform).

It is intended that people should update the conditions in their
jhbuildrc files like so:

  conditions_set.add('condition')

or

  conditions_set.remove('condition')

But they can also be replaced outright.

Future commits will introduce behaviour that depends on this setting.

Created attachment 264663 [details] [review]
Add --condition= commandline argument

This can be used like --condition=+doc,-wayland in order to modify the
condition set from the commandline.

This can be used from 'build', 'list', 'buildone' and 'make'.

Created attachment 264664 [details] [review]
Handle <condition> tags in moduleset files

If we encounter a <condition> tag, consult the conditions set in the
config in order to decide if we should include its content or not.  If
the condition is met, the child elements are added to the parent of the
<condition/> tag as if the condition tag were not there at all.  If the
condition is not met, the entire content is simply dropped.

We do the processing as a transformation on the DOM as a whole,
immediately after parsing the moduleset XML, before doing any additional
processing.  This allows <condition> to be used for anything and it
means we don't need to deal with it separately from each place.

Created attachment 264665 [details] [review]
autotools: refactor argument handling

Split out a tiny bit of common code used in collecting and expanding
autogenargs, makeargs and makeinstallargs.

There is a strict refactoring -- no functionality changes here.

The 'common code' will grow with the next commit, increasing the benefit
of having it factored out.

Created attachment 264666 [details] [review]
autotools: collect args from elements as well

The autotools modtype has support for specifying autogenargs, makeargs
and makeinstallargs as attributes on the <autotools/> tag.

Add support for specifying additional arguments via the elements
<autogenargs/>, <makeargs/> and <makeinstallargs/>.  The additional
arguments are concatenated to the list.

This is intended to be used with <conditional> to provide a way to
customise the arguments based on the condition set.

Note: this is the mechanism by which I believe we should make bootstrapping do nothing on Linux and more than it currently does on Mac OS.

(In reply to comment #8)
> Note: this is the mechanism by which I believe we should make bootstrapping do
> nothing on Linux and more than it currently does on Mac OS.

Again with Mac OS, yet in comment 2 you said your goal is BSD and this hasn't anything to do with Mac, notwithstanding the (incorrect) comment about x11 being undesirable in the Description.

What problem are you really trying to solve, and for what OS? 

Why do you think this approach is better than the long-standing customization facility?

The subject of this bug includes autogenargs, but this approach can't do that unless the entire module is made conditional because autogenargs is an attribute in the modules top element.

(In reply to comment #9)
> Again with Mac OS, yet in comment 2 you said your goal is BSD and this hasn't
> anything to do with Mac, notwithstanding the (incorrect) comment about x11
> being undesirable in the Description.

I appreciate that there is a history of confrontation between the people who work on Gtk/GLib and those that have worked on gtk-osx.  I just re-read the entire "call to sanity" thread as a refresher course.  I honestly don't want to get into the politics of that.  As it stands, these patches don't impact your ability (in any way) to continue doing what you're doing.

That said, it is my goal to improve portability of mainline GNOME.  That includes both the BSDs and Mac OS.  The vast majority of my efforts have been on FreeBSD, but it's true that I also care about Mac OS.  Both of these systems are currently second-class citizens and I'm trying to improve that situation.

It's true that I do intend to make some of the work that you're doing less necessary.  Again -- with the history -- I'm not sure exactly why the situation is the way that it is now, but having 3rd party modulesets and a pile of patches to get Gtk working on Mac OS is not my idea of a good time.  I want to get to a place where stock Gtk is working nicely.

For what it's worth, I've used gtk-mac-bundler and I love it.

> What problem are you really trying to solve, and for what OS? 

I'm trying to make it so that people who are not on Linux can download jhbuild from git and type 'jhbuild gtk+' and have it work.

I expect this to work properly on FreeBSD, OpenBSD, NetBSD, (maybe) OpenIndiana, Mac OS and (of course) Linux.

A big part of my reason for having this goal is because I want to get regular gnome-continuous-style builds of (at least) the lower stack of GNOME running on as many platforms as possible.  I don't think I need to tell you (of all people) how many times we've had GLib or Gtk tarball releases that don't even build on Mac OS due to some undetected problem.

A regular mingw cross-build (or even someone basing it on MSVC) would also be very interesting to me, but it's quite far outside of my scope at present and I don't know if jhbuild is the best idea for that.

> Why do you think this approach is better than the long-standing customization
> facility?

For about three reasons:

 1) it's less confusing to our users if they can just get jhbuild and use it

 2) it's easier for me to setup a builder based on it

 3) the modulesets automatically stay in sync

> The subject of this bug includes autogenargs, but this approach can't do that
> unless the entire module is made conditional because autogenargs is an
> attribute in the modules top element.

See the patch in comment 7.

The code here looks quite good offhand.  I would use this even on GNU/Linux systems for being able to conveniently flip on/off --enable-gtk-doc and such.

But I'd defer to Frédéric for review.

Comment on attachment 264666 [details] [review]
autotools: collect args from elements as well

This needs some easy updates to the schema validation files (modulesets/moduleset.*)

Comment on attachment 264665 [details] [review]
autotools: refactor argument handling

marked as accepted but it actually needs some pep8 fixes before going in.

+    instance.autogenargs = collect_args (instance, node, 'autogenargs')
should be
+    instance.autogenargs = collect_args(instance, node, 'autogenargs')

Comment on attachment 264662 [details] [review]
add 'conditions_set' to the config file

I would prefer if the logic part (conditions_sets...) would go in jhbuild/config.py.

Comment on attachment 264663 [details] [review]
Add --condition= commandline argument

+        if hasattr(options, 'conditions'):

Is the hasattr() really necessary?

Also there's already a --tags= parameter, perhaps it could be deprecated and merged with the conditions stuff?

Comment on attachment 264664 [details] [review]
Handle <condition> tags in moduleset files

Of course some PEP8 and the removal of print statements.

But short of that I actually appreciate (contrary to my initial idea) that the conditions processing all happens as a first step, instead of sticking it in many places.

This will also needs changes for schema validation in  modulesets/moduleset.*, and those will certainly arbitrarily restrict some possible usages of <condition/>)

@John, indeed I'm still of the opinion that specific modulesets should live with their respective projects (as it also happens to be the case for gstreamer, webkit, or sugar). However it looks like the approach Ryan is taking here won't (necessarily) complicate the modulesets, and it will also help in current pure-GNOME endeavours (to globally enable or disable wayland support, for example).

And while the approach would permit over-complex modulesets, I hope the arbitrary restrictions that will go in the schema validation files will serve as a guide on what we regard as sane usage of <condition> tags.

(In reply to comment #10)
> (In reply to comment #9)
> > Again with Mac OS, yet in comment 2 you said your goal is BSD and this hasn't
> > anything to do with Mac, notwithstanding the (incorrect) comment about x11
> > being undesirable in the Description.
> 
> I appreciate that there is a history of confrontation between the people who
> work on Gtk/GLib and those that have worked on gtk-osx.  I just re-read the
> entire "call to sanity" thread as a refresher course.  I honestly don't want to
> get into the politics of that.  As it stands, these patches don't impact your
> ability (in any way) to continue doing what you're doing.

I don't want to repeat that political noise either, and happily the outright hostility to OSX in the Gnome project has largely disappeared.

The problem I see is with going after bootstrap, which requires rather more on OSX than most other systems, particularly since Apple has dropped all autotools support from Xcode 5. I don't see a single common bootstrap moduleset that supports all platforms as being practical or maintainable: It would be like trying to redo GDK to replace the platform subdirectories with #ifdefs. It doesn't take much of that for a function to become unparsable by humans.

> 
> That said, it is my goal to improve portability of mainline GNOME.  That
> includes both the BSDs and Mac OS.  The vast majority of my efforts have been
> on FreeBSD, but it's true that I also care about Mac OS.  Both of these systems
> are currently second-class citizens and I'm trying to improve that situation.

I don't think that many Mac or Windows users are interested in mainline Gnome. They want to run a Gnome application or two, but few if any want a Gnome desktop or the various utilities that go along with it. The various BSDs may very well want a Gnome desktop. 

> 
> It's true that I do intend to make some of the work that you're doing less
> necessary.  Again -- with the history -- I'm not sure exactly why the situation
> is the way that it is now, but having 3rd party modulesets and a pile of
> patches to get Gtk working on Mac OS is not my idea of a good time.  I want to
> get to a place where stock Gtk is working nicely.

The patch issue is a bit skew to jhbuild aside from it being an extremely useful feature. It's more down to Gnome developers Linux-first and let someone else deal with the other OSes attitude. Unfortunately I don't see that changing any time soon.

> 
> For what it's worth, I've used gtk-mac-bundler and I love it.
> 
> > What problem are you really trying to solve, and for what OS? 
> 
> I'm trying to make it so that people who are not on Linux can download jhbuild
> from git and type 'jhbuild gtk+' and have it work.

Well, `jhbuild bootstrap && jhbuild build gtk+`. That almost works on Macs if the way you download jhbuild is spelt "gtk-osx-build-setup.sh".

> 
> I expect this to work properly on FreeBSD, OpenBSD, NetBSD, (maybe)
> OpenIndiana, Mac OS and (of course) Linux.
> 
> A big part of my reason for having this goal is because I want to get regular
> gnome-continuous-style builds of (at least) the lower stack of GNOME running on
> as many platforms as possible.  I don't think I need to tell you (of all
> people) how many times we've had GLib or Gtk tarball releases that don't even
> build on Mac OS due to some undetected problem.

Official multi-platform nightlies (although "nightly" has dubious meaning in a global project like Gnome) would indeed be wonderful, but setting that up doesn't require significant changes to jhbuild since it's pretty much a one-off for each supported OS.

> 
> A regular mingw cross-build (or even someone basing it on MSVC) would also be
> very interesting to me, but it's quite far outside of my scope at present and I
> don't know if jhbuild is the best idea for that.

It would help if it even worked on mingw. I've tried a couple of times and failed miserably. 

> 
> > Why do you think this approach is better than the long-standing customization
> > facility?
> 
> For about three reasons:
> 
>  1) it's less confusing to our users if they can just get jhbuild and use it

IMO users of jhbuild are developers. If they're confused by jhbuild, autotools is pretty much beyond them, and they're unlikely to be competent C programmers either.

> 
>  2) it's easier for me to setup a builder based on it

OK, if you say so.
> 
>  3) the modulesets automatically stay in sync

Not necessarily desirable. As I said earlier, Windows and OSX users of jhbuild are going to be people who want to build a single application, not the whole Gnome project. The main official modulesets are tailored to building a whole Gnome distribution.

With the political and goal stuff out of the way,

(In reply to comment #10)
> (In reply to comment #9)
> > The subject of this bug includes autogenargs, but this approach can't do that
> > unless the entire module is made conditional because autogenargs is an
> > attribute in the modules top element.
> 
> See the patch in comment 7.

Ah, sorry I missed that. Good.

In general I agree with Colin and Fred that this can be a useful feature for regular modulesets so long as it's used sparingly and we develop some style guidance for how much and when to use it.

(In reply to comment #17)
> @John, indeed I'm still of the opinion that specific modulesets should live
> with their respective projects (as it also happens to be the case for
> gstreamer, webkit, or sugar). 

Except that gtk-osx isn't a separate project like gstreamer, webkit, and sugar. Gtk-osx is about building the core dependencies used by those projects on MacOSX. Ryan is proposing effectively to fold those modules into the main modulesets with <conditional> elements. Are you OK with *that*?

I am, so long as the resulting modules don't get too big or hard to maintain. I do think that we want exactly one common way of build Gtk on OSX, so anything that goes into jhbuild should come out of gtk-osx.

Created attachment 265272 [details] [review]
autotools: collect args from elements as well

The autotools modtype has support for specifying autogenargs, makeargs
and makeinstallargs as attributes on the <autotools/> tag.

Add support for specifying additional arguments via the elements
<autogenargs/>, <makeargs/> and <makeinstallargs/>.  The additional
arguments are concatenated to the list.

This is intended to be used with <conditional> to provide a way to
customise the arguments based on the condition set.

(In reply to comment #15)
> (From update of attachment 264663 [details] [review])
> +        if hasattr(options, 'conditions'):
> 
> Is the hasattr() really necessary?

Yes.  It is.  Although we have a default value for the 'conditions' field in 'options' for commands that take a --conditions argument, we won't have it there for the other commands.  If you look at the other things in this file you see that they all use hasattr() for this reason.

> Also there's already a --tags= parameter, perhaps it could be deprecated and
> merged with the conditions stuff?

After a bit of digging, this seems to allow you to filter the build to only modules that are defined inside of a <moduleset name='tagname'/>.  We certainly don't use this feature in our modulesets, but I'll let Colin comment on if this feature is worth keeping (see bug 460360).

Created attachment 265282 [details] [review]
Handle <condition> tags in moduleset files

If we encounter a <condition> tag, consult the conditions set in the
config in order to decide if we should include its content or not.  If
the condition is met, the child elements are added to the parent of the
<condition/> tag as if the condition tag were not there at all.  If the
condition is not met, the entire content is simply dropped.

We do the processing as a transformation on the DOM as a whole,
immediately after parsing the moduleset XML, before doing any additional
processing.  This allows <condition> to be used for anything and it
means we don't need to deal with it separately from each place.

Although the tool itself will accept <condition> anywhere we use the
schemas to restrict its use to the purposes of conditionalising
dependencies (including suggests) and {autogen,make,makeinstall}args.

Created attachment 265324 [details] [review]
add 'conditions' to the config file

This is a set() that holds the condition flags for this run.

It is pre-seeded with some flags according to what platform you're
running on (by way of inspecting sys.platform)

It is intended that people should update the conditions in their
jhbuildrc files like so:

  conditions.add('condition')

or

  conditions.discard('condition')

But they can also be replaced outright, as long as it remains a set.

Future commits will introduce behaviour that depends on this setting.

Created attachment 265326 [details] [review]
Add --conditions= commandline argument

This can be used like --conditions=+doc,-wayland in order to modify the
condition set from the commandline.

The user's jhbuildrc is capable of inspecting conditions set from the
commandline by way of checking

    if 'flag' in conditions:
        ...

but the arguments on the commandline always override what is written in
jhbuildrc.  This is done by applying the modifiers twice -- once before
parsing the config and then again, after.

Created attachment 265327 [details] [review]
Handle <condition> tags in moduleset files

If we encounter a <condition> tag, consult the conditions set in the
config in order to decide if we should include its content or not.  If
the condition is met, the child elements are added to the parent of the
<condition/> tag as if the condition tag were not there at all.  If the
condition is not met, the entire content is simply dropped.

We do the processing as a transformation on the DOM as a whole,
immediately after parsing the moduleset XML, before doing any additional
processing.  This allows <condition> to be used for anything and it
means we don't need to deal with it separately from each place.

Although the tool itself will accept <condition> anywhere we use the
schemas to restrict its use to the purposes of conditionalising
dependencies (including suggests) and {autogen,make,makeinstall}args.

Some overall notes about changes:

 - I changed the name from 'conditions_set' to 'conditions'.  I just like
   it better.

 - the logic for setting the default conditions has been moved into config.py

 - the logic for overriding conditions with commandline arguments has been
   substantially improved -- it's now possible to inspect the overridden set of
   conditions from jhbuildrc and act on it

Thoughts:

I'm a bit concerned about the way that we define the default set of conditions from inside of jhbuild proper while we use the conditions by name from the moduleset.  We obviously hit compatibility issues between jhbuild and its moduleset files from time to time when we add features (like this one) but the addition of condition flags will be a large potential repeat-headache going foward.  We should either figure out a way to make the default-set-of-conditions determination part of the moduleset file itself or figure out a way that things will always build in a reasonable way even if a condition flag or two is missing from the default set.  I think a wait-and-see approach is probably best here.

If we ever decided that we want to define the condition flags in the modulset, we may want to use <condition> for that purpose, which means that using it for the purpose we have now (guarding conditional sections of the file) may have been a mistake.  <conditional> seems like it might be a nice name until you realise that

  <conditional if='...'>
    <dep/>
  </conditional>

kinda sounds strange.  In particular:

  <conditional unless='foo'>

sounds like the opposite meaning of what it intends.  It sounds like "this section is conditional, unless 'foo' is defined" which makes it sound like defining 'foo' is how you get this section non-conditionally included (which is the opposite of the truth).

Suggestions welcome.


I also considered that maybe instead of (or in addition to)

  --conditions=+wayland,-docs

we could have:

  --enable-wayland --disable-docs

or

  --with-wayland --without-docs

to match the autotools style.

(In reply to comment #27)

> I'm a bit concerned about the way that we define the default set of conditions
> from inside of jhbuild proper while we use the conditions by name from the
> moduleset.  We obviously hit compatibility issues between jhbuild and its
> moduleset files from time to time when we add features (like this one) but the
> addition of condition flags will be a large potential repeat-headache going
> foward.  We should either figure out a way to make the
> default-set-of-conditions determination part of the moduleset file itself or
> figure out a way that things will always build in a reasonable way even if a
> condition flag or two is missing from the default set.  I think a wait-and-see
> approach is probably best here.

If the point of the exercise is cross-platform module compatibility, jhbuild can easily figure out the build platform and set the appropriate condition. Other conditions, like whether building for X11 or Wayland on a Linux platform, could have a default in jhbuild that can be overridden in .jhbuildrc. You'd want to isolate those defaults in a separate file for easy maintainability.

If you set the defaults in the moduleset then the .jhbuildrc has to override them and you're back to having a complicated .jhbuildrc for everything except Linux.

> 
> If we ever decided that we want to define the condition flags in the modulset,
> we may want to use <condition> for that purpose, which means that using it for
> the purpose we have now (guarding conditional sections of the file) may have
> been a mistake.  <conditional> seems like it might be a nice name until you
> realise that
> 
>   <conditional if='...'>
>     <dep/>
>   </conditional>
> 
> kinda sounds strange.  In particular:
> 
>   <conditional unless='foo'>
> 
> sounds like the opposite meaning of what it intends.  It sounds like "this
> section is conditional, unless 'foo' is defined" which makes it sound like
> defining 'foo' is how you get this section non-conditionally included (which is
> the opposite of the truth).
> 
> Suggestions welcome.

How about changing the attribute from 'if' and 'unless' to a single 'condition' which understands 'not', or better yet, just passes the value of the attribute to python for interpretation? I'd also change the element name from 'conditional' to 'if' to avoid the redudancy of <conditional condition="foo">.

Comment on attachment 265324 [details] [review]
add 'conditions' to the config file

Fine.

Comment on attachment 265326 [details] [review]
Add --conditions= commandline argument

Fine but the additional mandatory argument to Config::__init__ will make the tests fail.

Could you update them so they are kept working?  (to be honest they were already broken and I just fixed them)

Comment on attachment 265327 [details] [review]
Handle <condition> tags in moduleset files

Fine, could you just copy the commit message as a docstring of _handle_conditions?

If I may, I'd also like two things:

# an update to the documentation to mention the new flag, configuration option, and moduleset xml element.

# an unrelated patch that would add versioning to modulesets, so in the event of similar changes, we can warn users that their jhbuild is not compatible with the moduleset they use. (in the current situation I'd just ask we keep an eye to keep <condition> usage so that existing users do not face major failures)

(In reply to comment #28)

> How about changing the attribute from 'if' and 'unless' to a single 'condition'
> which understands 'not', or better yet, just passes the value of the attribute
> to python for interpretation? I'd also change the element name from
> 'conditional' to 'if' to avoid the redudancy of <conditional condition="foo">.

Ryan, of course I'd be fine with that, too, if you prefer that over your current set of patches.

Attachment 264665 [details] pushed as fda1a54 - autotools: refactor argument handling
Attachment 265272 [details] pushed as de708b2 - autotools: collect args from elements as well
Attachment 265324 [details] pushed as e2f0f4e - add 'conditions' to the config file
Attachment 265326 [details] pushed as 29734bd - Add --conditions= commandline argument

Made the proposed changes and we decided to go with <if> but not the
evaluation of arbitrary python expressions from the moduleset.  The
generic nature of the name <if> leaves open the possibility that we may
expand the possible attributes later.