Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-68296

Broken sidebar icons in GitLab Branch Source plugin (regression in 2.341)

    XMLWordPrintable

Details

    • 2.348, 2.346.1

    Description

      Since 2.341, the sidebar icons of the gitlab branch source plugin are broken:

      Restoring to 2.340 and below resolves the problem.
      To reproduce:

      1. Create a new multibranch pipeline (workflow-multibranch-plugin)
      2. Branch Sources -> Add Source -> GitLab Project (make sure you have the gitlab branch source plugin installed)
      3.  In the "Owner" field add your GitLab username and the project you want to add in the "Projects" dropdown. Preferably you have a repository with a dummy Jenkinsfile, e.g. https://gitlab.com/NotMyFault/jenkinsfile to utilize the option "Jenkinsfile" on Build Configuration -> Script Path
      4. Save the configuration, wait until the scanning is done and you can see the broken image on the sidebar of the job overview.

      This issue is a copy of https://github.com/jenkinsci/gitlab-branch-source-plugin/issues/185 to track it on the ux regression dashboard on Jira.

      Attachments

        Issue Links

          Activity

            When the GitlabSCMSource and GitlabSCMNavigator retrieveActions methods are triggered, a GitlabLink action (IconSpec implementation) are added to the relevant output of the project.

            Those GitlabLink objects have a gitlab-X reference, which is used in the actions.jelly of core to resolve the icon to use (through a task.jelly which will be resolved as a icon.jelly).

            During the initialization of the plugin, icons are added into the IconSet "bank" with the id "gitlab-X".
            This provides the controller with a way to resolve "gitlab-X" -> URL to be able to fetch the image.

            The IconSet has:

            • a map which resolves the URL to an Icon
            • a map which resolves the CSS class to a Icon
            • a map which resolves the Class Spec to a Icon

            (not mentioning the core icons)

            When adding a Icon into the IconSet, the method addIcon adds it to the CSSSelector and URL resolver, but not to the ClassSpec. However, when the instance is completely initialized, we can run

            org.jenkins.ui.icon.IconSet.icons.iconsByClassSpec.each { key,val ->
              println key + " -> " + val.url;
            }
            

            and see that the gitlab-X are added but with the URL _.

            The actions.jelly tries to resolve the iconClassName using the IconSet#getIconByClassSpec only and find a IconSet#NO_ICON placeholder. In the end, the icons are not shown.

            However, and this shouldn't be the fix, but adding the icon- prefix to the icons class spec, on the GitlabLink objects and on the iconClassName of both GitlabSCMSource and GitlabSCMNavigator descriptors fix the problem, for new projects.

            alecharp Adrien Lecharpentier added a comment - When the GitlabSCMSource and GitlabSCMNavigator retrieveActions methods are triggered, a GitlabLink action ( IconSpec implementation) are added to the relevant output of the project. Those GitlabLink objects have a gitlab-X reference, which is used in the actions.jelly of core to resolve the icon to use (through a task.jelly which will be resolved as a icon.jelly ). During the initialization of the plugin, icons are added into the IconSet "bank" with the id "gitlab-X". This provides the controller with a way to resolve "gitlab-X" -> URL to be able to fetch the image. The IconSet has: a map which resolves the URL to an Icon a map which resolves the CSS class to a Icon a map which resolves the Class Spec to a Icon (not mentioning the core icons) When adding a Icon into the IconSet , the method addIcon adds it to the CSSSelector and URL resolver, but not to the ClassSpec. However, when the instance is completely initialized, we can run org.jenkins.ui.icon.IconSet.icons.iconsByClassSpec.each { key,val -> println key + " -> " + val.url; } and see that the gitlab-X are added but with the URL _ . The actions.jelly tries to resolve the iconClassName using the IconSet#getIconByClassSpec only and find a IconSet#NO_ICON placeholder. In the end, the icons are not shown. However, and this shouldn't be the fix , but adding the icon- prefix to the icons class spec , on the GitlabLink objects and on the iconClassName of both GitlabSCMSource and GitlabSCMNavigator descriptors fix the problem, for new projects.
            basil Basil Crow added a comment -

            I think there are three scenarios of relevance here:

            1. GitLab Branch Source plugin prior to JENKINS-68137 (working)
            2. GitHub Branch Source plugin after JENKINS-68137 (working)
            3. GitHub Branch Source plugin after JENKINS-68137 (failing)

            We have all the scenarios that are needed for analysis, so it is just a matter of doing the analysis. My suggestion is to write down, for each scenario, the entire flow of execution from when Jelly starts evaluating the relevant template to when the result of the evaluation is completed. Something like this (line numbers and notes are fictitious just to illustrate the format):

            Execution                                            Notes
            =========                                            =====
                -> _db_enter_ (index.jelly:40)                   Evaluating it.results
                  -> code_state (IconSet.java:803)               icons contains X, resultMap is empty
                    -> my_thread_var_dbug (IconSet.java:8003)    args: X=1, Y=2
                      -> pthread_getspecific (IconSet.java:904)
                      <- pthread_getspecific (IconSet.java:907)  return: false
                    <- my_thread_var_dbug (IconSet.java:8005)
            

            We can skip internal Stapler and Jelly evaluation stack frames, but we cannot skip actual lines of Jelly templates or the Java methods that are called as a result of evaluating those Jelly lines, since that is the core area of code that has changed. Yes, this process will be very laborious. It will take some time to figure out how to trace the flow of execution from Jelly evaluation to Java code and vice versa. Some patience is needed while single-stepping through the guts of Stapler and Jelly to reach some Java code. I suspect each one of these execution traces would be dozens of lines long.

            I went through a similar exercise when finding the root cause of JENKINS-68270, but I didn't write down the execution traces. Rather I memorized them as I was single-stepping. I commonly use this strategy when debugging. The strategy is this: single-step through the old (working) code, memorize the execution trace, single-step through the new (broken) code, then diff the two execution traces. It always works, but it does take some time.

            You will need a classpath with a recent version of core and all relevant plugins to do the proper single-stepping in the debugger. I suggest using a copy of gitlab-branch-source-plugin updated to the latest core and bom-weekly, attaching to a remote JVM of Jenkins core to do the single-stepping. I use IntelliJ but any IDE will suffice.

            Once these execution traces are written down, it is just a matter of diffing them to find the area of focus. It might take hours or even days to trace the execution and write the traces, but it will take minutes to understand the cause once the difference is identified. Once you have the traces, let me know and we can analyze them together.

            basil Basil Crow added a comment - I think there are three scenarios of relevance here: GitLab Branch Source plugin prior to JENKINS-68137 (working) GitHub Branch Source plugin after JENKINS-68137 (working) GitHub Branch Source plugin after JENKINS-68137 (failing) We have all the scenarios that are needed for analysis, so it is just a matter of doing the analysis. My suggestion is to write down, for each scenario, the entire flow of execution from when Jelly starts evaluating the relevant template to when the result of the evaluation is completed. Something like this (line numbers and notes are fictitious just to illustrate the format): Execution Notes ========= ===== -> _db_enter_ (index.jelly:40) Evaluating it.results -> code_state (IconSet.java:803) icons contains X, resultMap is empty -> my_thread_var_dbug (IconSet.java:8003) args: X=1, Y=2 -> pthread_getspecific (IconSet.java:904) <- pthread_getspecific (IconSet.java:907) return: false <- my_thread_var_dbug (IconSet.java:8005) We can skip internal Stapler and Jelly evaluation stack frames, but we cannot skip actual lines of Jelly templates or the Java methods that are called as a result of evaluating those Jelly lines, since that is the core area of code that has changed. Yes, this process will be very laborious. It will take some time to figure out how to trace the flow of execution from Jelly evaluation to Java code and vice versa. Some patience is needed while single-stepping through the guts of Stapler and Jelly to reach some Java code. I suspect each one of these execution traces would be dozens of lines long. I went through a similar exercise when finding the root cause of JENKINS-68270 , but I didn't write down the execution traces. Rather I memorized them as I was single-stepping. I commonly use this strategy when debugging. The strategy is this: single-step through the old (working) code, memorize the execution trace, single-step through the new (broken) code, then diff the two execution traces. It always works, but it does take some time. You will need a classpath with a recent version of core and all relevant plugins to do the proper single-stepping in the debugger. I suggest using a copy of gitlab-branch-source-plugin updated to the latest core and bom-weekly , attaching to a remote JVM of Jenkins core to do the single-stepping. I use IntelliJ but any IDE will suffice. Once these execution traces are written down, it is just a matter of diffing them to find the area of focus. It might take hours or even days to trace the execution and write the traces, but it will take minutes to understand the cause once the difference is identified. Once you have the traces, let me know and we can analyze them together.

            I could pin point the problem we have here. Let me try to explain.

            The icon.jelly is trying to find the source of the image using Functions#tryGetIconPath(String, JellyContext) method. That method then uses Functions#tryGetIcon(String) method. This method is trying to find the icon with the IconSet#getIconByClassSpec(String). Until this point, the icon was referred by gitlab-commit (or any other). The getIconByClassSpec method looks at the map called iconsByClassSpec, which is not populated when the icons are added into the IconSet but when the icons are requested (in this method). As the map doesn't contain the gitlab-commit, it tries to look into the iconsByCSSSelector map, which is populated when the icons are added into the IconSet.

            The problem is, when the icons are added, they are suffixed by the icon size class (icon-md, icon-sm, etc.). The same icon is added 4 times, to have all the size. This is coming from the time we had png files, and one for each size.

            However, when we are looking at the icon the first time, we are looking for gitlab-commit, without any CSS class for the size. So the icon is not found in the CSS class map. At this point, no icon is found and the code is adding the NO_ICON into the iconsByClassSpec map and null is returned by the getIconByClassSpec(String) method.

            Back to Functions#tryGetIcon, the null value makes the code goes again into the IconSet#getIconByClassSpec(String) but, this time, two modifications are made on the String used to referenced the icon: it's "normalized" using IconSet#toNormalizedIconNameClass(String) and is suffixed by icon-md. The toNormalizedIconNameClass using Icon#toNormalizedIconNameClass(String) method, which prefixed the icon name by icon- if it was not.

            When the IconSet#getIconByClassSpec(String) will look for the icon, it won't find the new reference icon-gitlab-commit icon-md into the iconsByClassSpec map (because it was never requested), so it will look for it into the CSS class map. However, when icons were added, the CSS classes where not prefixed with icon-, so again, we store the NO_ICON reference into the map and we return null.

            From here, there are two things I can see we need to do:
            1. we use the Icon#toNormalizedClassName(String) method when we create a new Icon for the classSpec attribute
            2. in order to not have to change the plugins, we need to make sure that the iconClassSpec parameter of the IconSet#getIconByClassSpec(String) method is transformed to be "normalized".

            The problems:
            1. Some icons are looked at by URL, which means that the iconClassSpec is something like /plugin/credentials/xxx, and this is prefixed by icon-, which is not found and stored like that into the iconsClassSpec map with a reference to the NO_ICON. Later the code is looking that the icon with the URL map, and is found. So the icons are served correctly, but this is suboptimal.
            2. we have a map which have a correspondance to icon-gitlab-commit with a reference to the NO_ICON, so the code is always taking the long road to find the icon. In my tests, I saw that it was already the case for the icon-folder icon

            So, the "solution" I have is basically a disaster in the making, growing the map with invalid reference to the same NO_ICON, forcing the icon lookup mechanism to use more memory (by populating the map) and taking more lines of code to find the icons we are looking for.

            alecharp Adrien Lecharpentier added a comment - I could pin point the problem we have here. Let me try to explain. The icon.jelly is trying to find the source of the image using Functions#tryGetIconPath(String, JellyContext) method. That method then uses Functions#tryGetIcon(String) method. This method is trying to find the icon with the IconSet#getIconByClassSpec(String) . Until this point, the icon was referred by gitlab-commit (or any other). The getIconByClassSpec method looks at the map called iconsByClassSpec , which is not populated when the icons are added into the IconSet but when the icons are requested (in this method). As the map doesn't contain the gitlab-commit , it tries to look into the iconsByCSSSelector map, which is populated when the icons are added into the IconSet . The problem is, when the icons are added, they are suffixed by the icon size class ( icon-md , icon-sm , etc.). The same icon is added 4 times, to have all the size. This is coming from the time we had png files, and one for each size. However, when we are looking at the icon the first time, we are looking for gitlab-commit , without any CSS class for the size. So the icon is not found in the CSS class map. At this point, no icon is found and the code is adding the NO_ICON into the iconsByClassSpec map and null is returned by the getIconByClassSpec(String) method. Back to Functions#tryGetIcon , the null value makes the code goes again into the IconSet#getIconByClassSpec(String) but, this time, two modifications are made on the String used to referenced the icon: it's "normalized" using IconSet#toNormalizedIconNameClass(String) and is suffixed by icon-md . The toNormalizedIconNameClass using Icon#toNormalizedIconNameClass(String) method, which prefixed the icon name by icon- if it was not. When the IconSet#getIconByClassSpec(String) will look for the icon, it won't find the new reference icon-gitlab-commit icon-md into the iconsByClassSpec map (because it was never requested), so it will look for it into the CSS class map. However, when icons were added, the CSS classes where not prefixed with icon- , so again, we store the NO_ICON reference into the map and we return null . From here, there are two things I can see we need to do: 1. we use the Icon#toNormalizedClassName(String) method when we create a new Icon for the classSpec attribute 2. in order to not have to change the plugins, we need to make sure that the iconClassSpec parameter of the IconSet#getIconByClassSpec(String) method is transformed to be "normalized". The problems: 1. Some icons are looked at by URL, which means that the iconClassSpec is something like /plugin/credentials/xxx , and this is prefixed by icon- , which is not found and stored like that into the iconsClassSpec map with a reference to the NO_ICON . Later the code is looking that the icon with the URL map, and is found. So the icons are served correctly, but this is suboptimal. 2. we have a map which have a correspondance to icon-gitlab-commit with a reference to the NO_ICON , so the code is always taking the long road to find the icon. In my tests, I saw that it was already the case for the icon-folder icon So, the "solution" I have is basically a disaster in the making, growing the map with invalid reference to the same NO_ICON , forcing the icon lookup mechanism to use more memory (by populating the map) and taking more lines of code to find the icons we are looking for.

            janfaracik maybe you have some idea for an elegant solution for this matter.

            BTW, addressing this on the plugin could be a fix, but the problem is that existing projects would need to be rescan completely to add the actions again and have the correct icon fetched.

            alecharp Adrien Lecharpentier added a comment - janfaracik maybe you have some idea for an elegant solution for this matter. BTW, addressing this on the plugin could be a fix, but the problem is that existing projects would need to be rescan completely to add the actions again and have the correct icon fetched.
            basil Basil Crow added a comment -

            This seems to accurately describe the failing scenario of GitLab Branch Source plugin after JENKINS-68137 (scenario 3) but I cannot see how it describes scenario 1 (working scenario of GitLab Branch Source plugin prior to JENKINS-68137) and scenario 2 (working scenario of GitHub Branch Source plugin after JENKINS-68137).

            basil Basil Crow added a comment - This seems to accurately describe the failing scenario of GitLab Branch Source plugin after JENKINS-68137 (scenario 3) but I cannot see how it describes scenario 1 (working scenario of GitLab Branch Source plugin prior to JENKINS-68137 ) and scenario 2 (working scenario of GitHub Branch Source plugin after JENKINS-68137 ).

            Side note, the Functions#tryGetIcon(String) method is, for the second lookup, using the IconSet#toNormalizedIconNameClass(String) method because in IconSet#initializeSVGs() method, icons are added with the icon- prefix, but plugins can / are still referring to older names, like the redo in workflow-cps-plugin (see ReplayAction#105).

            alecharp Adrien Lecharpentier added a comment - Side note, the Functions#tryGetIcon(String) method is, for the second lookup, using the IconSet#toNormalizedIconNameClass(String) method because in IconSet#initializeSVGs() method, icons are added with the icon- prefix, but plugins can / are still referring to older names, like the redo in workflow-cps-plugin (see ReplayAction#105 ).

            basil in the scenario 1, the actions.jelly (on line 40), is suffixing the action.iconClassName with the icon-md.

            This means that, when the IconSet#getIconByClassSpec function is looking for the icon in iconsByCSSSelector, it's looking for .gitlab-commit.icon-md (for example) which is present in the map. So the correct icon is stored in the iconsByClassSpec and returned.

            For scenario 2, it is as I described before: the icon is searched with gitlab-commit (only), which is not found in iconsByClassSpec, it's then transformed into CSS class format (.gitlab-commit) and searched in iconsByCSSSelector map, which contains the selector with the icon size class so it's not found and thus the gitlab-commit search returns null and the NO_ICON reference is stored in the iconsByClassSpec map.

            alecharp Adrien Lecharpentier added a comment - basil in the scenario 1, the actions.jelly (on line 40), is suffixing the action.iconClassName with the icon-md . This means that, when the IconSet#getIconByClassSpec function is looking for the icon in iconsByCSSSelector , it's looking for .gitlab-commit.icon-md (for example) which is present in the map. So the correct icon is stored in the iconsByClassSpec and returned. For scenario 2, it is as I described before: the icon is searched with gitlab-commit (only), which is not found in iconsByClassSpec , it's then transformed into CSS class format ( .gitlab-commit ) and searched in iconsByCSSSelector map, which contains the selector with the icon size class so it's not found and thus the gitlab-commit search returns null and the NO_ICON reference is stored in the iconsByClassSpec map.

            People

              alecharp Adrien Lecharpentier
              notmyfault Alexander Brandes
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: