Bug 321667 - Flaw in direct-repo-child filter prevents other valid repos from being recursively added
Summary: Flaw in direct-repo-child filter prevents other valid repos from being recurs...
Status: RESOLVED FIXED
Alias: None
Product: kdesrc-build
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Michael Pyne
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-27 10:56 UTC by David Faure
Modified: 2014-02-15 17:29 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
Removes problematic kde-project module filter (2.23 KB, patch)
2013-07-06 05:05 UTC, Michael Pyne
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Faure 2013-06-27 10:56:54 UTC
with use-modules kde, "module kdelibs" finds it, "module nepomuk-core" doesn't find it because it's kdelibs/nepomuk-core.


Reproducible: Always

Steps to Reproduce:
module-set
    repository kde-projects
    use-modules kde
end module-set
module nepomuk-core
    run-tests true
end module

Leads to kdesrc-build attempting to do a svn checkout of nepomuk-core !
(See what I meant about "module" being ambiguous.
I really think there should be a different keyword for "let me declare a brand new module here, from svn by default", and "let me give you options for a module that should have already be defined indirectly by a module-set before".)
Actual Results:  
$ kdesrc-build nepomuk-core

# kdesrc-build running: 'svn' 'co' '--non-interactive' 'svn+ssh://svn@svn.kde.org/home/kde/trunk/KDE/nepomuk-core' 'nepomuk-core'
# from directory: /d/kde/src/t
svn: E170000: URL 'svn+ssh://svn@svn.kde.org/home/kde/trunk/KDE/nepomuk-core' doesn't exist


Expected Results:  
Building nepomuk-core (1/1)
        Updating nepomuk-core (to branch master)
     ..

(which I get if I write "module kdelibs/nepomuk-core")

Thanks.
Comment 1 David Faure 2013-06-27 11:07:24 UTC
The workaround doesn't even really work. It works for "kdesrc-build nepomuk-core", but not for "kdesrc-build", which then says

Error updating kdelibs/nepomuk-core, removing from list of packages to build

due to trying to get it with svn

# kdesrc-build running: 'svn' 'co' '--non-interactive' 'svn+ssh://svn@svn.kde.org/home/kde/trunk/KDE/kdelibs/nepomuk-core' 'nepomuk-core'
Comment 2 Michael Pyne 2013-06-29 04:34:27 UTC
I see the need for a better semantic to this. What would you propose as being most useful and memorable?

Something like

module-options
    # Some scheme to select modules as a set, could be 'use-modules' also
    applied-to nepomuk-core
    cmake-options -DCMAKE_FOO=BAR
end module-options

It could also be made to override individual modules by specifying a name (perhaps would make the 'applied-to' option an error since it's implicit)?

The way I see it allowing for option sets would allow you to also "stack" option sets for a module multiple times that way you can set common options for a given subset and then more specific options for a narrower subset.

But the problem is what to name the construct to make it clear that such options should override *existing* modules, as otherwise we're back to the same syntax we have now.
Comment 3 David Faure 2013-07-01 09:14:23 UTC
(In reply to comment #2)
> I see the need for a better semantic to this. What would you propose as
> being most useful and memorable?
> 
> Something like
> 
> module-options
>     # Some scheme to select modules as a set, could be 'use-modules' also
>     applied-to nepomuk-core
>     cmake-options -DCMAKE_FOO=BAR
> end module-options
> 
> It could also be made to override individual modules by specifying a name
> (perhaps would make the 'applied-to' option an error since it's implicit)?
> 
> The way I see it allowing for option sets would allow you to also "stack"
> option sets for a module multiple times that way you can set common options
> for a given subset and then more specific options for a narrower subset.
> 
> But the problem is what to name the construct to make it clear that such
> options should override *existing* modules, as otherwise we're back to the
> same syntax we have now.

Yes, "module-options" seems clear and unambiguous to me. It's "options for an existing module", so if the module isn't known at that point, it will give an error.

I like the two possibilities: "module-options kdelibs"  to apply a set of options to kdelibs, and module-options with a list of modules that these options apply to (but I'm not sure about the name of the key there... you suggested applied-to or use-modules.... at least "applies-to" would be an improvement of the first one, no? Or even "apply-to", same grammar as "use-modules". Alternatively "options-for", just "modules"... OK nothing is really great. Let's say "apply-to"?
I'm afraid reusing "use-modules" with a slightly different meaning could be a bit confusing.

Stacking options is very useful indeed. But on that topic, it would be useful to be able to remove a cmake option or cxxflag in a given module-options... :)
My use case for that is that have -DQT_STRICT_ITERATORS in cxxflags in the global section, but I need to remove it for one bad module, calligra. Hmm tell me if I should open a separate report for that, I'm mixing 3 issues here (module search, module-options sections, and removing options for a specific module).

Cheers,
Comment 4 Michael Pyne 2013-07-02 03:28:49 UTC
Having a way to filter out options from cxxflags/cmake-options should probably be a separate ticket.

However, have you considered adding -UQT_STRICT_ITERATORS to the cxxflags for just calligra? That might work, and *should* do the right thing already.
Comment 5 David Faure 2013-07-02 09:06:10 UTC
Ah, I thought I tried it, but maybe that was with cmake-options, or maybe I was confused between adding and replacing cxxflags. It adds, indeed. Perfect.

So back to the main issue here: I can't compile attica, soprano, or nepomuk-core anymore because it's trying to get them from svn :)
Comment 6 David Faure 2013-07-02 12:16:57 UTC
Somehow kde/applications/kde-baseapps isn't getting updated nor built, either....
Comment 7 Michael Pyne 2013-07-04 16:14:43 UTC
If this is still a big problem, then a workaround that can be used until this is properly implemented is to mention the module you want to override the options for within the use-modules

e.g.,

module-set
    repository kde-projects
    use-modules nepomuk-core attica soprano kde
end module-set

module nepomuk-core
    # Override options as normal
end module

Unfortunately you have to make sure you mention the modules you wish to override before any parents due to bug 321883, but this syntax should work even now (only for a module-at-a-time though).
Comment 8 David Faure 2013-07-05 08:33:57 UTC
OK, thanks for the workaround for overriding options.

Please note that the main blocker now is comment #6 though:
when just specifying "use-modules kde", many modules are not being built at all (like kde-baseapps, but also many many more).
Tell me if I should start splitting this up into multiple reports; I've been overloading this one with all the things that break when using "use-modules kde".
Comment 9 Michael Pyne 2013-07-06 02:36:02 UTC
The issue here is a side-effect of an implementation decision to avoid recursing for repos under existing repos.

The canonical example were the "SuperBuild" style repositories where you could have a base module within kde-projects (something like kde/kdefoo) which would be a real git repository that had a CMake script to checkout the rest of the module as git submodules and run the build that way. The components of kdefoo would then be real git repositories in kde/kdefoo/*.

It doesn't make sense to build both so kdesrc-build stops at the higher-level repo (since the leaf repos can be manually requested with the kde/kdefoo/* syntax).

Calligra is another example. "calligra" itself points to a git repository for the whole office suite, but it also contains calligra/krita-extensions/* and calligra/calligra-history (though this is marked inactive, at least). Prior to the addition of the ignore-modules option there was no way to avoid pulling these repos-upon-repos in without stopping at the topmost repo level (it was still possible to be specific to ask for these subrepos).

ANYWAYS, it looks like even with all this in mind, that it is faulty not to fully recurse when the requested entry is not even a real repository at all. I.e. the current impl seems to improperly reset the 'do-not-exceed-repo-level' flag to always be the lowest of any found repository, even when not looking at a direct descendant chain.

In the example of looking for "kde" kdesrc-build does fine for kdeadmin, as all of its actual repos are 3-level (kde/kdeadmin/*). Next it hits kdeplasma-addons which is 2-level (kde/kdeplasma-addons) and from then on kdesrc-build will only allow 2-level git repos under kde/, which gives the remainder of the module list (kdepim, kdeexamples, kdelibs, kdepimlibs, kdepim-runtime, kde-runtime, kde-workspace, superbuild, kdegraphics). The modules are then put in order, filtered, etc.

I'll try to figure out the best way to solve tonight, but there you have the diagnosis.
Comment 10 Michael Pyne 2013-07-06 05:05:02 UTC
Created attachment 80986 [details]
Removes problematic kde-project module filter

This patch is actually sitting on my disk as a git commit and should be perfect for your use case.

However I would want to get the new "ignore-modules" option to *also* act fully recursively. Without that there's a lot of extraneous modules I seem to get from things like extragear/graphics (e.g. if I ignore 'digikam' I still get digikam-doc, likewise for kipi-plugins). I would like to fix that as well before pushing to master.

Also there is the newly-interesting question of "how do I build *just* kdelibs?" (try it, it's fun ;). But this should give something to experiment with while I'm off vacationing for half a day tomorrow.
Comment 11 Michael Pyne 2013-07-06 05:06:58 UTC
Updating bug metadata now that more info is available on the issue.
Comment 12 David Faure 2013-07-09 08:26:11 UTC
This patch helps indeed. I went from 92 modules to build, to 272.
Comment 13 David Faure 2013-07-10 14:34:17 UTC
Indeed, building just kdelibs isn't possible anymore. Can't "kdesrc-build kdelibs" do a recursive search for a module with that name?

Same thing for "module kdelibs" (to override some options).
Comment 14 Michael Pyne 2013-07-12 03:08:05 UTC
(In reply to comment #13)
> Indeed, building just kdelibs isn't possible anymore. Can't "kdesrc-build
> kdelibs" do a recursive search for a module with that name?
> 
> Same thing for "module kdelibs" (to override some options).

Both should be possible. The command line can prefer actual modules to guessed kde-projects paths; overriding module options would imply a single module if we adopt the syntax you proposed in your comment 3.

If you wanted just kdelibs (and not nepomuk-core, kactivities, etc.) then we can still use the single module notation:

module kdelibs
    repository kde:kdelibs
end module

But perhaps it would be prudent to have something like 'kdefoo/' (trailing slash) in use-modules ask for a given module and not any children? Seems like overkill, though it might be useful at the command-line (unless there is a better way to disambiguate between kdelibs/* and kdelibs.git).
Comment 15 Michael Pyne 2013-07-12 03:44:24 UTC
Git commit a033866429b62c29fd93aea843c2f21427634ec8 by Michael Pyne.
Committed on 06/07/2013 at 04:50.
Pushed by mpyne into branch 'fix-recursing'.

kde-projects: Don't auto-filter git-repo's direct children.

Kind of a mouthful, but remove a filter that was added to avoid
automatically including an actual git repository's children that were
also git repositories. E.g. think of something like a SuperBuild git
module that also had its component git modules as logical children
within projects.kde.org.

This filter is unnecessary now that kdesrc-build supports both kde.org
metadata (kde-build-metadata/build-script-ignore) and user-configurable
module filtering (ignore-modules option).

More importantly this filter precludes many other desirable types of
group syntax (e.g. including all of kdebindings just by asking for
kdebindings).

So we remove the filter. The trouble I've seen so far is that it is
now rather difficult to build *only* kdelibs, since kde/kdelibs is a
logical parent of nepomuk-core, nepomuk-widgets, and kactivities. This
can be worked-around by using a normal single module declaration. E.g.

    module kdelibs
        repository kde:kdelibs
    end module

On the other hand, the recently-added config file option
"ignore-modules" should now remove any kde-projects modules that contain
a path component(s) matching the ignored module. You may have to be
specific with a given ignore atom for this reason (e.g. ignore
playground/libs, not 'libs' otherwise you'll also ignore
kdegraphics/libs at this point... though that was also true before this
commit).

M  +24   -0    doc/index.docbook
M  +1    -1    modules/ksb/BuildContext.pm
M  +10   -13   modules/ksb/KDEXMLReader.pm

http://commits.kde.org/kdesrc-build/a033866429b62c29fd93aea843c2f21427634ec8
Comment 16 Michael Pyne 2013-07-12 03:49:08 UTC
I've pushed a more fleshed-out version of the patch to the 'fix-recursing' branch of kdesrc-build if you wish to try.

I think I'll have to treat the idea of selecting the module (instead of group of modules) from the command line as a separate issue. I don't think that should be too difficult to adjust but that whole portion of the code probably needs yet another refactoring to get the code to match the intent.
Comment 17 Michael Pyne 2013-07-24 04:28:12 UTC
Git commit 5d638acab096f17dd1d136868e674d36eed6f21e by Michael Pyne.
Committed on 23/07/2013 at 22:58.
Pushed by mpyne into branch 'fix-recursing'.

kde-projects: Accurately track parent module set.

This is unfortunately a giant change, as all of the functionality that
is encompassed into module-sets currently had to migrate over to
multiple separate classes, including the new ksb::ModuleSet class and
subclasses.

This was a long-overdue change, however, and should allow for accurately
tracking a source module-set for a given module.

On the other hand this migration of logic has made it easier to
understand each of the individual pieces where they stand (e.g. there is
no longer a separate expandXMLModules and expandModuleSets).

In addition we can properly handle ignore-modules with wildcards just as
we do with use-modules (they even use the same matching logic) which
means that it is safe to integrate this into master (assuming no extra
boogs get added, of course).

This will also help with fixing some of the extant module-selection bugs
(321883, 299415).
Related: bug 321275

M  +151  -403  kdesrc-build
M  +67   -1    modules/ksb/BuildContext.pm
M  +52   -19   modules/ksb/KDEXMLReader.pm
M  +4    -2    modules/ksb/Module.pm
A  +182  -0    modules/ksb/ModuleSet.pm
A  +222  -0    modules/ksb/ModuleSet/KDEProjects.pm
A  +33   -0    modules/ksb/ModuleSet/Null.pm

http://commits.kde.org/kdesrc-build/5d638acab096f17dd1d136868e674d36eed6f21e
Comment 18 David Faure 2013-12-19 13:15:42 UTC
Seems not fully fixed?

# GET ME ALL OF KDE SC!
module-set
    repository kde-projects
    # WORKAROUND FOR BUG 321667
    #use-modules kdesupport attica soprano kde
    use-modules kdesupport kde
    ignore-modules kdewin-installer kdewin emerge emerge-history
end module-set

module attica
  cmake-options -DATTICA_ENABLE_TESTS=TRUE
end module

kdesrc-build attica leads to an unexpected svn checkout:

# kdesrc-build running: 'svn' 'co' '--non-interactive' 'svn+ssh://svn@svn.kde.org/home/kde/trunk/KDE/attica' 'attica'

If I uncomment the workaround, it works.
Comment 19 Michael Pyne 2014-01-02 03:17:33 UTC
That behavior was actually on purpose; you must actually mention the module you want to override to try to avoid having your module declaration magically do something different because a new framework is added to kde_projects.xml (yet another reason to use a specific keyword for this, I think).

It would be easy enough to default to always using a matching KDE project, should I try for that?
Comment 20 David Faure 2014-01-06 19:08:44 UTC
Yeah, it's a bit annoying to have to specify the module in two places in order to change its options. Not very modular in terms of writing up the kdesrc-buildrc file.
Comment 21 Michael Pyne 2014-01-07 05:45:10 UTC
Is it also OK to use a specific keyword then? This is the time to ensure it's right before it needs to be supported til the end of time. ;)

I'd suggest a simple "options", e.g.

8<==============
module-set
    repository kde-projects
    use-modules tier1
end module-set

options kcoreaddons
    cmake-options -DFOO=BAR
end options
8<==============

The option-reading code is already module-declarator agnostic so this, at least, should not be very difficult to do.

Thoughts?
Comment 22 David Faure 2014-01-09 19:29:08 UTC
Yes! +2 for options.

It would remove the ambiguity of "module", which gives me svn checkout commands when misused :)

Thanks!
Comment 23 Michael Pyne 2014-01-11 21:09:17 UTC
Git commit 38ea997502e18313e6fe0f7120dee9ed8544bd82 by Michael Pyne.
Committed on 11/01/2014 at 20:28.
Pushed by mpyne into branch 'master'.

rc-file: Add specific "option override" entry type.

Add a specific config file grouping (which acts just like a module
declaration), to allow for specifying options to override a
previously-declared module.

The use case here is for a module-set: You can specify options which
apply to an entire module-set when declaring the module-set, and then
override those options with any changes in a later "options"
declaration.

These declarations can stack too, so this can also be useful for
multi-level file includes (but this is less useful since an "options"
declaration requires a specific module, it doesn't work on module-sets;
in that case you'd want to have the different module-sets in your
most-specific included config files instead of in a base file).

Tested on my personal test case for bug 321883, and on a --pretend run,
and with a bug 321883 test case modified to not pre-declare the
overridden module first.

Example:
    module-set kde-mm
        repository kde-projects
        use-modules kde/kdemultimedia
    end module-set

    options kmix # Not mentioned before this line
        branch KDE/4.11
    end options

In this case kmix would use KDE/4.11 branch while juk (and the rest of
kde-mm) would use whatever the global branch or branch-group was
(probably 'master').

M  +67   -1    doc/index.docbook
M  +30   -20   modules/ksb/Application.pm

http://commits.kde.org/kdesrc-build/38ea997502e18313e6fe0f7120dee9ed8544bd82
Comment 24 David Faure 2014-01-13 19:08:50 UTC
My hero!

This is so much more readable now, being able to differ actual external modules with options for kde git modules.

"As soon as" everything compiles (ok, so not very soon yet) I'll make a blog with more details, but here's what the "compile everything" rc file looks like now:
http://www.davidfaure.fr/2014/kdesrc-buildrc
Comment 25 Michael Pyne 2014-01-14 01:53:09 UTC
Sounds great! I've updated the website documentation for kdesrc-build (and made an apologetic note on its homepage).

If you don't mind my asking though, was it your intention to mark the bug as REOPENED?

Either way please continue to let me know how I can help. I don't get tons of time nowadays but I will keep chipping away at what I can.
Comment 26 David Faure 2014-02-15 17:29:23 UTC
Hmm can't remember why I reopened it. Sorry about that.