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 790238 - flatpak: guess_primary_module() function is not reliable
flatpak: guess_primary_module() function is not reliable
Status: RESOLVED FIXED
Product: gnome-builder
Classification: Other
Component: plugins
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME Builder Maintainers
GNOME Builder Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-11-12 10:46 UTC by larchunix
Modified: 2017-11-20 06:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
flatpak: fix module discovery with nested modules (3.39 KB, patch)
2017-11-20 00:42 UTC, Christian Hergert
committed Details | Review

Description larchunix 2017-11-12 10:46:45 UTC
GNOME-Builder is not able to find the right primary module in Remmina Flatpak manifest (https://raw.githubusercontent.com/FreeRDP/Remmina/next/flatpak/org.remmina.Remmina.json).

With the following module tree (see below), gnome-builder assumes the primary module is xprop whereas it should use remmina instead:

root
├─libappindicator                                                               
│ ├─libdbusmenu                                                                 
│ └─libindicator                                                                
├─avahi                                                                         
├─libxkbfile                                                                    
├─nxproxy                                                                       
├─freerdp                                                                       
│ └─xprop                                                                       
├─spice-gtk                                                                     
│ ├─python-pyparsing                                                            
│ ├─python-six                                                                  
│ ├─lz4                                                                         
│ └─spice-protocol                                                              
├─libssh                                                                        
├─libvncserver                                                                  
├─xephyr                                                                        
│ ├─libfontenc                                                                  
│ ├─libXfont                                                                    
│ └─xkbcomp                                                                     
└─remmina                                                                       

When moving xprop module in the tree like this (see below), gnome-builder uses remmina as primary module as expected:

root
├─libappindicator                                                               
│ ├─libdbusmenu                                                                 
│ └─libindicator                                                                
├─avahi                                                                         
├─libxkbfile                                                                    
├─nxproxy                                                                       
├─freerdp                                                                       
├─xprop                                                                       
├─spice-gtk                                                                     
│ ├─python-pyparsing                                                            
│ ├─python-six                                                                  
│ ├─lz4                                                                         
│ └─spice-protocol                                                              
├─libssh                                                                        
├─libvncserver                                                                  
├─xephyr                                                                        
│ ├─libfontenc                                                                  
│ ├─libXfont                                                                    
│ └─xkbcomp                                                                     
└─remmina                                                                       

The culprit seems to be the function guess_primary_module() in file src/plugins/flatpak/gbp-flatpak-configuration.c.

My understanding is that the recursion logic in this function is wrong. Base cases does not take into account if the module under analysis is nested or not.

In Remmina manifest case, I guess that when processing xprop module, the condition `json_array_get_length (modules) == 1` is true and the xprop module is returned whereas it shouldn't.
Comment 1 Christian Hergert 2017-11-12 11:05:05 UTC
Seems plausible. I don't think nested modules were a thing when that code was written.
Comment 2 larchunix 2017-11-12 16:27:17 UTC
According to git log, this function appeared first without nested modules in 

commit 457fc3d8f08a4a8e1a60c2f6bf9842fc38ca6694                                 
Author: Matthew Leeds <mleeds@redhat.com>                                       
Date:   Mon Nov 14 23:26:00 2016 -0800                                          
                                                                                
    flatpak: Discover flatpak manifests and add them as runtimes                
                                                                                
    When a repo includes a flatpak manifest, GNOME Builder should know          
    how to use it with flatpak-builder to build the project's dependencies      
    so the build will be more likely to work. This commit teaches               
    GbpFlatpakRuntimeProvider how to identify manifests (by file name and       
    doing a sanity check on the contents) and add them to the list of           
    runtimes.                                                                   
                                                                                
    https://bugzilla.gnome.org/show_bug.cgi?id=773764                           

Nested modules support (and the recursion logic) has been added later in

commit 46265261f3495b7c93d2c25ec4f72437f7f8b625                                 
Author: Matthew Leeds <mleeds@redhat.com>                                       
Date:   Wed Dec 21 16:46:55 2016 -0600                                          
                                                                                
    flatpak: Check nested modules for the primary one                           
                                                                                
    Flatpak manifests can have modules nested in other modules, so we should    
    check them all when looking for the primary one, just in case.              

I would suggest you to simply revert 4626526 because it does not work as expected and I can't think of a case where the primary module could be a nested one.
Comment 3 Christian Hergert 2017-11-20 00:42:50 UTC
Created attachment 364017 [details] [review]
flatpak: fix module discovery with nested modules

This fixes an issue where a dependency has nested modules and in turn
tricks Builder into which is the module for the project.
Comment 4 Christian Hergert 2017-11-20 00:44:02 UTC
This fixes the issue for Remmina so that it now builds in Builder.

However, since it is CMake, and our CMake support is limited, you still
cannot run the project when the Run button is clicked.

Attachment 364017 [details] pushed as f57ae2b - flatpak: fix module discovery with nested modules
Comment 5 Christian Hergert 2017-11-20 06:39:45 UTC
I also just changed how we extract targets for running, so that we can get the command from flatpak. So running Remmina should work now too.