After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 782320 - No "ninjaargs" or equivalent
No "ninjaargs" or equivalent
Status: RESOLVED FIXED
Product: jhbuild
Classification: Infrastructure
Component: general
unspecified
Other Linux
: Normal minor
: ---
Assigned To: Jhbuild maintainers
Jhbuild QA
Depends on:
Blocks:
 
 
Reported: 2017-05-08 09:29 UTC by Xabier Rodríguez Calvar
Modified: 2018-04-21 04:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Allow passing makeargs to Ninja-based modules (1.81 KB, patch)
2017-11-23 15:20 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
Allow expressing arguments for Ninja (21.30 KB, patch)
2018-04-05 15:18 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
Allow expressing arguments for Ninja (24.28 KB, patch)
2018-04-05 15:52 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review

Description Xabier Rodríguez Calvar 2017-05-08 09:29:28 UTC
You can change makeargs and add a -j (which is useful when used with IceCC) but there is no way to do that when things are built with ninja.
Comment 1 Sébastien Wilmet 2017-11-23 15:02:26 UTC
It would be useful to me too to build WebKit with -j1, since CMake modules now use ninja by default and there is no way to change that in the jhbuildrc AFAIK, see:
https://bugzilla.gnome.org/show_bug.cgi?id=778784#c11
Comment 2 Emmanuele Bassi (:ebassi) 2017-11-23 15:19:57 UTC
Both the Meson and CMake modtypes collect makeargs and module_makeargs, but neither of them passes them to Ninja.

I'm not strongly sold on adding yet another variable, especially if you can use module_makeargs.
Comment 3 Emmanuele Bassi (:ebassi) 2017-11-23 15:20:40 UTC
Created attachment 364278 [details] [review]
Allow passing makeargs to Ninja-based modules
Comment 4 Michael Catanzaro 2017-11-23 15:23:38 UTC
It doesn't really matter either way, but I think it would be less confusing to add a ninjaargs variable than to reuse makeargs. Just be sure to update index.docbook, moduleset.dtd, and moduleset.rnc.

Reusing makeargs doesn't make (*heh*) as much sense to me, because make is not used at all. But I'm OK with that if that's your preference, Emmanuele.
Comment 5 Michael Catanzaro 2017-11-23 15:24:17 UTC
Review of attachment 364278 [details] [review]:

(accepted with the hesitation I mention above)
Comment 6 Emmanuele Bassi (:ebassi) 2017-11-23 15:50:15 UTC
The issue about adding a new `ninjaargs` configuration option (both global and per-module) means adding a whole lot of code to handle that new option and propagate it everywhere.

I'd rather not touch jhbuild *that* much, especially now that we're trying to move away from it.
Comment 7 Emmanuele Bassi (:ebassi) 2017-11-23 16:09:45 UTC
(In reply to Michael Catanzaro from comment #4)
> It doesn't really matter either way, but I think it would be less confusing
> to add a ninjaargs variable than to reuse makeargs. Just be sure to update
> index.docbook, moduleset.dtd, and moduleset.rnc.

The moduleset DTDs already specify the attributes. The documentation is also missing a bunch of things for Meson and CMake.

> Reusing makeargs doesn't make (*heh*) as much sense to me, because make is
> not used at all. But I'm OK with that if that's your preference, Emmanuele.

Meson and CMake can both use different backends; CMake defaults to Ninja, but it's entirely possible to run things without Ninja installed, and it'll fall back to make — which will then use the `makeargs` or `module_makeargs` configuration. Having an option being considered depending on whether you have a tool installed looks more like a spooky action at a distance to me, and would definitely be more confusing.

This whole thing makes more sense if you define `makeargs` as "the configuration for the tool that performs the build" — which usually is `make` but could also be `ninja`. After all, internally, both the MesonModule and CMakeModule classes inherit from MakeModule.

Renaming the option to `buildargs` would make more sense than adding `ninjaargs`, but it would also be a massive change.
Comment 8 Michael Catanzaro 2017-11-23 16:19:25 UTC
OK, makeargs it is!
Comment 9 Sébastien Wilmet 2017-11-24 07:33:16 UTC
The patch works fine for me, I'm able to pass -j1 to ninja.
Comment 10 Sébastien Wilmet 2017-12-13 20:45:12 UTC
Attachment 364278 [details] pushed as f8c0519 - Allow passing makeargs to Ninja-based modules
Comment 11 Philip Withnall 2017-12-15 10:40:25 UTC
This breaks use of default makeargs. For example, if I have this in my .jhbuildrc:

# Verbose build output
makeargs = 'V=1'

It will cause all ninja-based builds to fail with:

*** Building jsonrpc-glib *** [1/1]
ninja V=1 -j 2
ninja: error: unknown target 'V=1', did you mean 'all'?

I don’t think we should be using the same variable for make and ninja here.

Example log:

https://jenkins.freedesktop.org/view/GNOME%20Coverity/job/coverity/job/coverity-scan-jhbuild/2991/console
Comment 12 Michael Catanzaro 2017-12-15 15:33:09 UTC
We'll have to use a separate ninjaargs variable, and require that CMake always uses the Ninja generator.
Comment 13 Michael Catanzaro 2017-12-15 15:33:54 UTC
Please feel free to revert this. Sorry we didn't consider that it could break this way.
Comment 14 Philip Withnall 2017-12-18 19:17:00 UTC
Emmanuele reverted it as:

commit f54479328091b70e81b02f5ea8bd1becc614955b (HEAD -> master, origin/master, origin/HEAD)
Author: Emmanuele Bassi <ebassi@gnome.org>
Date:   Mon Dec 18 16:45:57 2017 +0000

    Revert "Allow passing makeargs to Ninja-based modules"
    
    This reverts commit f8c05196933f8c1010c5b9d163567c10c990dda7.
    
    This commit broke valid use cases of makeargs, so we're going to need a
    different option for Ninja.

Thanks, Emmanuele. Sorry for being a spanner in the works here.
Comment 15 Emmanuele Bassi (:ebassi) 2018-04-05 15:18:12 UTC
Created attachment 370554 [details] [review]
Allow expressing arguments for Ninja

Ninja-based modules, like MesonModule and CMakeModule, need a way to
control the arguments passed to Ninja — mostly, the ability to set the
`-j 1` argument to disable parallel builds.

We tried with commit f8c0519693 to overload makeargs for that, but it
had to be reverted because, unsurprisingly, make and ninja have
different ideas about what arguments are valid, and jhbuild has global
options for Make arguments.

Let's try again, this time introducing a NinjaModule base class, in
parallel to the existing MakeModule one; the NinjaModule class can do
all the Ninja discovery and execution, and control the configuration
options — both global and per module.

The MesonModule class can inherit straight from NinjaModule, thus
removing all the ad hoc code that used MakeModule for the set up, but
then ignored its `make()` method.

The CMakeModule is a bit of a nasty case, as CMake can have both a Make
and a Ninja backend, which means we need to inherit from MakeModule and
NinjaModule; luckily, both MakeModule and NinjaModule inherit from the
shared Package class, so we don't have conflicting initializations.
Comment 16 Emmanuele Bassi (:ebassi) 2018-04-05 15:20:15 UTC
A bit late, but this works for me when building Meson and CMake modules.
Comment 17 Daniel Boles 2018-04-05 15:22:41 UTC
Meson has `makeargs` in the DTD; is that unwanted?
Comment 18 Emmanuele Bassi (:ebassi) 2018-04-05 15:45:01 UTC
(In reply to Daniel Boles from comment #17)
> Meson has `makeargs` in the DTD; is that unwanted?

I haven't touched the various schemas.
Comment 19 Emmanuele Bassi (:ebassi) 2018-04-05 15:46:26 UTC
The `makeargs` was added in commit 28d866fdbedef9633c32da799abd642f34f43b3a, as a copy-paste from cmake.
Comment 20 Daniel Boles 2018-04-05 15:49:07 UTC
Oh, yeah, I didn't mean to blame you for that, was just wondering if it might be worth removing, since it seems not to be useful. I was thinking about this the other day and forgot until I got a notification here.
Comment 21 Emmanuele Bassi (:ebassi) 2018-04-05 15:52:48 UTC
Created attachment 370556 [details] [review]
Allow expressing arguments for Ninja

v2:
  - update the DTD and RNC schemas
Comment 22 Sébastien Wilmet 2018-04-11 13:05:52 UTC
I think you can just push the commit, before conflicts start to appear.
Comment 23 Emmanuele Bassi (:ebassi) 2018-04-11 14:19:44 UTC
Attachment 370556 [details] pushed as 57d00a9 - Allow expressing arguments for Ninja

Reviewed-by: nobody

Considering the lack of reviewers for Jhbuild, I went ahead and pushed the change. If somebody has any issue, feel free to re-open the bug and/or revert the commit.
Comment 24 Ting-Wei Lan 2018-04-21 04:30:13 UTC
I just pushed commit a8c7dcd to fix DTD and RNC schemas.