Uploaded image for project: 'Bigtop'
  1. Bigtop
  2. BIGTOP-2536

tarball.destination is ignored when set

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.1.0
    • Fix Version/s: 1.2.0
    • Component/s: build
    • Labels:
      None

      Description

      There seems to be a bug in the current logic, assigning the destination filename of a component artifact. Current logic does this

              if (!config.bigtop.components[target].tarball.source.isEmpty())
                config.bigtop.components[target].downloaddst = DL_DIR + '/' + config.bigtop.components[target].tarball.source
      

      which effectively ignores the destination filename. The potential issues could come if we build more than one component of a git repo (say GH), and all of those are downloading the master branch archive. In this case, the first component artifact will be stored as dl/master.zip, but the consequent component builds will simply skip the download part (as the master.zip is already present) and try to proceed with building a wrong source code.

        Issue Links

          Activity

          Hide
          sekikn Kengo Seki added a comment -

          We have the logic to repack the archives if needed

          Oh, I didn't know that. I'll file a new JIRA and submit a revised patch. Thanks!

          Show
          sekikn Kengo Seki added a comment - We have the logic to repack the archives if needed Oh, I didn't know that. I'll file a new JIRA and submit a revised patch. Thanks!
          Hide
          cos Konstantin Boudnik added a comment -

          I think it looks reasonable, but are you positive we need to do the changes about .tar.gz versus .zip suffix? We have the logic to repack the archives if needed... And I would like to open a separate JIRA to address the issue, so we can move the discussion over there. Thanks for looking into the problem, man!

          Show
          cos Konstantin Boudnik added a comment - I think it looks reasonable, but are you positive we need to do the changes about .tar.gz versus .zip suffix? We have the logic to repack the archives if needed... And I would like to open a separate JIRA to address the issue, so we can move the discussion over there. Thanks for looking into the problem, man!
          Hide
          sekikn Kengo Seki added a comment -

          I think this addendum patch fix the problem. Konstantin Boudnik would you take a look? I confirmed ./gradlew deb -Dbuildwithdeps=true works.

          Show
          sekikn Kengo Seki added a comment - I think this addendum patch fix the problem. Konstantin Boudnik would you take a look? I confirmed ./gradlew deb -Dbuildwithdeps=true works.
          Hide
          cos Konstantin Boudnik added a comment -

          Damn, in fact it breaks qfs and data-fu builds as well. Weird why only these two are screwed up - will try to get to the bottom of this.

          Show
          cos Konstantin Boudnik added a comment - Damn, in fact it breaks qfs and data-fu builds as well. Weird why only these two are screwed up - will try to get to the bottom of this.
          Hide
          oflebbe Olaf Flebbe added a comment -
          Show
          oflebbe Olaf Flebbe added a comment - Konstantin Boudnik Could you please verify that this patch isn't a build breaker for ycsb ? See: https://ci.bigtop.apache.org/job/Bigtop-trunk-packages/BUILD_ENVIRONMENTS=centos-6,COMPONENTS=ycsb,label=docker-slave/305/console
          Hide
          cos Konstantin Boudnik added a comment -

          Pushed into the master.

          Show
          cos Konstantin Boudnik added a comment - Pushed into the master.
          Hide
          rvs Roman Shaposhnik added a comment -

          LGTM.

          Show
          rvs Roman Shaposhnik added a comment - LGTM.
          Hide
          cos Konstantin Boudnik added a comment -

          here's the patch. I would appreciate a second pair of eyes on this.

          Show
          cos Konstantin Boudnik added a comment - here's the patch. I would appreciate a second pair of eyes on this.

            People

            • Assignee:
              cos Konstantin Boudnik
              Reporter:
              cos Konstantin Boudnik
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development