Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0.0
    • Component/s: build
    • Labels:
      None

      Description

      Right now, we apply changes/patches in the do-build-component script in the bigtop-packages/common/<package> part of the build process .

      Would be cleaner and easier to use the RPM / Deb functionality of RPMbuild / debuild to apply patches. But right now the gradle script is not flexible enough. Since make is now obsolete we can improve this situation.

        Issue Links

          Activity

          Hide
          cos Konstantin Boudnik added a comment -

          Please also update the wiki to document the patching instructions. Thanks

          Show
          cos Konstantin Boudnik added a comment - Please also update the wiki to document the patching instructions. Thanks
          Hide
          cos Konstantin Boudnik added a comment -

          I have committed and pushed it. Thanks Olaf!

          Also, I renamed the ticket to make it aligned with the commit message.

          Show
          cos Konstantin Boudnik added a comment - I have committed and pushed it. Thanks Olaf! Also, I renamed the ticket to make it aligned with the commit message.
          Hide
          cos Konstantin Boudnik added a comment -

          Actually, what I had in mind is some sort of unit tests for the logic inside of these tasks. I am not picking at the one you've changed, but we have a number of complex ones and it'd be nice to be able to tests them even before the build starts. E.g. look at org.apache.bigtop.TestBuildUtils which tests the logic of BOM parsing, etc.

          But I guess this wish is going over and above the scope of this ticket. I will commit this as the fix is proper and correctly executed. But it'd be nice to think about how we can test our build logic.

          Show
          cos Konstantin Boudnik added a comment - Actually, what I had in mind is some sort of unit tests for the logic inside of these tasks. I am not picking at the one you've changed, but we have a number of complex ones and it'd be nice to be able to tests them even before the build starts. E.g. look at org.apache.bigtop.TestBuildUtils which tests the logic of BOM parsing, etc. But I guess this wish is going over and above the scope of this ticket. I will commit this as the fix is proper and correctly executed. But it'd be nice to think about how we can test our build logic.
          Hide
          oflebbe Olaf Flebbe added a comment -

          The ultimate test case I can think of is to recompile the whole bigtop project. I did this on debian and Centos. Debian recompiled, but I happen to have an issue on crunch with RPM builds. Looking at the SPEC file I am wondering how it ever worked in the past. I may have broken this package. (Issue is filenames and directories within the SOURCE tar.gz and the directories the SPEC file %setup is using). Either way: it can be fixed

          Show
          oflebbe Olaf Flebbe added a comment - The ultimate test case I can think of is to recompile the whole bigtop project. I did this on debian and Centos. Debian recompiled, but I happen to have an issue on crunch with RPM builds. Looking at the SPEC file I am wondering how it ever worked in the past. I may have broken this package. (Issue is filenames and directories within the SOURCE tar.gz and the directories the SPEC file %setup is using). Either way: it can be fixed
          Hide
          cos Konstantin Boudnik added a comment -

          Thanks for the explanation. I agree that the original code was a bit messy - sorry about it. Although I have an excuse: I was trying to replicate the make stuff At any rate... I think the change you've made is in fact correct: thanks for standing your ground!

          I wish there's a way to have a test for tasks like this... IMO a test is the best way to document the logic like you've described above. Can you think of a way to test this stuff?

          +1 otherwise. I will a bit to see if anyone have any comments on how to test this type of build functionality. Will commit this later today or in the morning.

          Show
          cos Konstantin Boudnik added a comment - Thanks for the explanation. I agree that the original code was a bit messy - sorry about it. Although I have an excuse: I was trying to replicate the make stuff At any rate... I think the change you've made is in fact correct: thanks for standing your ground! I wish there's a way to have a test for tasks like this... IMO a test is the best way to document the logic like you've described above. Can you think of a way to test this stuff? +1 otherwise. I will a bit to see if anyone have any comments on how to test this type of build functionality. Will commit this later today or in the morning.
          Hide
          oflebbe Olaf Flebbe added a comment -

          Somehow replying to an jira email does not work as expected.

          The logic before was complex and it used misleading naming of variables.

          We have 3 cases :

          1) Non existing sources: We need to create a tar with LICENSE file

          2) ZIP Files: Have to be converted to tar files. With the corner case that it may have to move one level up.

          3) TAR Files. Which have simply to be renamed.

          In the case 1 and 2 with have to create a tar afterwards after creating/unpacking
          In case 3 a simple rename does the work.


          A code path with UNPACK = "tar -xzf" was never used before.

          instead the side effect of PATCHES equals empty was used to mark empty packets, which is misleading — at least.

          I didn't get this logic correct in the first attempts.

          Please reconsider.

          Show
          oflebbe Olaf Flebbe added a comment - Somehow replying to an jira email does not work as expected. The logic before was complex and it used misleading naming of variables. We have 3 cases : 1) Non existing sources: We need to create a tar with LICENSE file 2) ZIP Files: Have to be converted to tar files. With the corner case that it may have to move one level up. 3) TAR Files. Which have simply to be renamed. In the case 1 and 2 with have to create a tar afterwards after creating/unpacking In case 3 a simple rename does the work. A code path with UNPACK = "tar -xzf" was never used before. instead the side effect of PATCHES equals empty was used to mark empty packets, which is misleading — at least. — I didn't get this logic correct in the first attempts. Please reconsider.
          Hide
          cos Konstantin Boudnik added a comment -

          Olaf Flebbe looks like you've sent me some comments on this matter via email. I've deleted the email thinking that the comment is on this ticket. But when I went to the ticket I couldn't find the comments. Could you please post them here? Thanks!

          Show
          cos Konstantin Boudnik added a comment - Olaf Flebbe looks like you've sent me some comments on this matter via email. I've deleted the email thinking that the comment is on this ticket. But when I went to the ticket I couldn't find the comments. Could you please post them here? Thanks!
          Hide
          cos Konstantin Boudnik added a comment -

          Now I see that default unpack method, eg tar -xzf isn't there anymore. For some reason you have removed the logic around choosing how the archive is unpacked - tar vs unzip - and now only are using unzip command line.

          Also, the logic here

              if (TARBALL_SRC.empty || TARBALL_SRC.endsWith('.zip')) {
                if (TARBALL_SRC.empty) {
          

          seems to be overly complex, isn't it? Would it work if something like that is done:

              def UNPACK = "tar -xzf"
              if (TARBALL_SRC.endsWith('.zip')) {
                UNPACK = "unzip"
              }
                if (TARBALL_SRC.empty) {
                  // if no tar file needed (i.e. bigtop-utils)
                  // create some contents to pack later
                  copy {
                    from 'LICENSE'
                    into TAR_DIR
                  }
                } else {
                  exec {
                    workingDir TAR_DIR
                    commandLine "$UNPACK $DOWNLOAD_DST".split(' ')
                  }
          ....
          

          eg the entire if (TARBALL_SRC.empty || TARBALL_SRC.endsWith('.zip')) is gone
          Seems to be a bit less code and a simpler logic to follow, no?

          Show
          cos Konstantin Boudnik added a comment - Now I see that default unpack method, eg tar -xzf isn't there anymore. For some reason you have removed the logic around choosing how the archive is unpacked - tar vs unzip - and now only are using unzip command line. Also, the logic here if (TARBALL_SRC.empty || TARBALL_SRC.endsWith('.zip')) { if (TARBALL_SRC.empty) { seems to be overly complex, isn't it? Would it work if something like that is done: def UNPACK = "tar -xzf" if (TARBALL_SRC.endsWith('.zip')) { UNPACK = "unzip" } if (TARBALL_SRC.empty) { // if no tar file needed (i.e. bigtop-utils) // create some contents to pack later copy { from 'LICENSE' into TAR_DIR } } else { exec { workingDir TAR_DIR commandLine "$UNPACK $DOWNLOAD_DST" .split(' ') } .... eg the entire if (TARBALL_SRC.empty || TARBALL_SRC.endsWith('.zip')) is gone Seems to be a bit less code and a simpler logic to follow, no?
          Hide
          oflebbe Olaf Flebbe added a comment -

          I didn't get all the side effects correct and the patch was plain wrong and Konstantin Boudnik was right. The left out part was essential.

          Reworked patch and inserted comments.

          Hope that this is finally o.k. this time.

          Show
          oflebbe Olaf Flebbe added a comment - I didn't get all the side effects correct and the patch was plain wrong and Konstantin Boudnik was right. The left out part was essential. Reworked patch and inserted comments. Hope that this is finally o.k. this time.
          Hide
          oflebbe Olaf Flebbe added a comment -

          Oh I was wrong about PATCHES not empty every time. There is a corner case used by the bigtop-utils package.... Have to rework patch

          Show
          oflebbe Olaf Flebbe added a comment - Oh I was wrong about PATCHES not empty every time. There is a corner case used by the bigtop-utils package.... Have to rework patch
          Hide
          oflebbe Olaf Flebbe added a comment -

          Yep, sorry this part is from a different patch set of mine, forgot to fix layout.

          Right, will split off the Pig Patch.

          There is already another Patch waiting in the queue building up on patching: BIGTOP-1486 . Two machines of mine are right now doing a full rebuild for DEB and RPM right now, with some of my patches enabled.

          Show
          oflebbe Olaf Flebbe added a comment - Yep, sorry this part is from a different patch set of mine, forgot to fix layout. Right, will split off the Pig Patch. There is already another Patch waiting in the queue building up on patching: BIGTOP-1486 . Two machines of mine are right now doing a full rebuild for DEB and RPM right now, with some of my patches enabled.
          Hide
          cos Konstantin Boudnik added a comment -

          I see one more instance where named closure parameter is sitting on a separate line

                patches.each {
                  f -> def res = new File("$DEB_BLD_DIR/debian/$f").renameTo( "$DEB_BLD_DIR/debian/patches/$f")
          

          And could you please answer my question about a need to include getCount2getLength.diff into this patch? it seems to be a separate issue fixing Pig build, rather than doing anything about package patching.

          Show
          cos Konstantin Boudnik added a comment - I see one more instance where named closure parameter is sitting on a separate line patches.each { f -> def res = new File( "$DEB_BLD_DIR/debian/$f" ).renameTo( "$DEB_BLD_DIR/debian/patches/$f" ) And could you please answer my question about a need to include getCount2getLength.diff into this patch? it seems to be a separate issue fixing Pig build, rather than doing anything about package patching.
          Hide
          cos Konstantin Boudnik added a comment -

          Makes sense!

          Show
          cos Konstantin Boudnik added a comment - Makes sense!
          Hide
          cos Konstantin Boudnik added a comment -

          Olaf Flebbe, the Wiki permissions are granted. Lemme know if it doesn't work.

          Show
          cos Konstantin Boudnik added a comment - Olaf Flebbe , the Wiki permissions are granted. Lemme know if it doesn't work.
          Hide
          oflebbe Olaf Flebbe added a comment -

          Patch uploaded.

          Documentation about this would be useful. I found nothing where I could start, since do-component-build should be documented first. I would prefer to edit the Wiki

          https://cwiki.apache.org/confluence/display/BIGTOP/Bigtop+Packaging

          first. May I have Edit rights ?

          Alternativly, I am looking into providing some documentation of the building process, specifically in the DEB part.

          Show
          oflebbe Olaf Flebbe added a comment - Patch uploaded. Documentation about this would be useful. I found nothing where I could start, since do-component-build should be documented first. I would prefer to edit the Wiki https://cwiki.apache.org/confluence/display/BIGTOP/Bigtop+Packaging first. May I have Edit rights ? Alternativly, I am looking into providing some documentation of the building process, specifically in the DEB part.
          Hide
          oflebbe Olaf Flebbe added a comment - - edited

          Source formatting: Holy $hit ! Sorry, that mess TAB vs Whitespace was not intended. Cleaning up.

          Regarding specTmpFileName: I do not think I can directly dump on the specfile: Opening with newWriter() will empy the file before eachLine can pickup the contents of the file itself.

          I found that there is a bug in the original script hidden

                    def TOP_LEVEL_DIR = unpacked.list()[0]
          -          fileTree ("$TAR_DIR/$TOP_LEVEL_DIR") {
          -            include '**/*'
          -          }.copy { into TAR_DIR }
          -          delete(TOP_LEVEL_DIR)
          

          The upstream download for hue 3.7 contains a broken symbolic link. the gradle task dies while trying to copy this link.
          I changed it into renaming the individual files, rather copy and deleting it afterwards.

          Show
          oflebbe Olaf Flebbe added a comment - - edited Source formatting: Holy $hit ! Sorry, that mess TAB vs Whitespace was not intended. Cleaning up. Regarding specTmpFileName : I do not think I can directly dump on the specfile: Opening with newWriter() will empy the file before eachLine can pickup the contents of the file itself. I found that there is a bug in the original script hidden def TOP_LEVEL_DIR = unpacked.list()[0] - fileTree ( "$TAR_DIR/$TOP_LEVEL_DIR" ) { - include '**/*' - }.copy { into TAR_DIR } - delete(TOP_LEVEL_DIR) The upstream download for hue 3.7 contains a broken symbolic link. the gradle task dies while trying to copy this link. I changed it into renaming the individual files, rather copy and deleting it afterwards.
          Hide
          oflebbe Olaf Flebbe added a comment -

          One by one

          This part was never used before since PATCHES variable was never empty (Always "/dev/null")

                  copy {
                    from 'LICENSE'
                    into TAR_DIR
                  }
          

          so I removed it.

          Show
          oflebbe Olaf Flebbe added a comment - One by one This part was never used before since PATCHES variable was never empty (Always "/dev/null") copy { from 'LICENSE' into TAR_DIR } so I removed it.
          Hide
          cos Konstantin Boudnik added a comment -

          I guess I need to modify my comment about getCount2getLength.diff: as it fixes a broken Pig build - shall we commit it separately once this fix is in?

          Show
          cos Konstantin Boudnik added a comment - I guess I need to modify my comment about getCount2getLength.diff : as it fixes a broken Pig build - shall we commit it separately once this fix is in?
          Hide
          cos Konstantin Boudnik added a comment -

          Thanks Olaf. A couple of things:

          • in packages.gradle the following bit is missing in the else branch. Shall it be kept?
                    copy {
                      from 'LICENSE'
                      into TAR_DIR
                    }
            
          • named variable for closure parameter is normally sits on the same line as the closure name, e.g. not
                specFile.eachLine {
                   line ->
            

            but

                specFile.eachLine { line ->
            

            There are multiple occasions where this is broken. Let's keep the same style as in the rest of the code.

          • there are some formatting issues in lines 421-440. Please fix them
          • can we avoid adding .diff files into the source just for the testing purposes? E.g. patch0-getCount2getLength.diff. You might need it to test the functionality, but IMO it shouldn't be sitting there permanently as it isn't required to build Pig packages
          • I think it'd be great to document the patching process somewhere. Perhaps in the README.md?
          • it seems that specTmpFileName can be avoided all together: you just can directly dump the content to specFileName, no?
          Show
          cos Konstantin Boudnik added a comment - Thanks Olaf. A couple of things: in packages.gradle the following bit is missing in the else branch. Shall it be kept? copy { from 'LICENSE' into TAR_DIR } named variable for closure parameter is normally sits on the same line as the closure name, e.g. not specFile.eachLine { line -> but specFile.eachLine { line -> There are multiple occasions where this is broken. Let's keep the same style as in the rest of the code. there are some formatting issues in lines 421-440. Please fix them can we avoid adding .diff files into the source just for the testing purposes? E.g. patch0-getCount2getLength.diff . You might need it to test the functionality, but IMO it shouldn't be sitting there permanently as it isn't required to build Pig packages I think it'd be great to document the patching process somewhere. Perhaps in the README.md? it seems that specTmpFileName can be avoided all together: you just can directly dump the content to specFileName , no?
          Hide
          oflebbe Olaf Flebbe added a comment -
          Show
          oflebbe Olaf Flebbe added a comment - Roman Shaposhnik Konstantin Boudnik please review
          Hide
          oflebbe Olaf Flebbe added a comment -

          Fixed typo in RPM part and tested on Centos

          Show
          oflebbe Olaf Flebbe added a comment - Fixed typo in RPM part and tested on Centos
          Hide
          oflebbe Olaf Flebbe added a comment -

          Note: I could not verify the RPM part, only that it is compiles and should create correct SPEC files.

          Show
          oflebbe Olaf Flebbe added a comment - Note: I could not verify the RPM part, only that it is compiles and should create correct SPEC files.
          Hide
          oflebbe Olaf Flebbe added a comment -

          Work will be demoed with BIGTOP-1588

          Show
          oflebbe Olaf Flebbe added a comment - Work will be demoed with BIGTOP-1588
          Hide
          oflebbe Olaf Flebbe added a comment - - edited

          Basically the idea is to use automatic patching already provided by dpkg_source and rpmbuild

          Debian Patches are patch -p1 files in debian/patches/xxx and a debian/patches/series file stating the order of how to apply patches. This is conceptually very similar to the RPM spec file approach naming indiviual patches

          RPMbuild Patches can be applied with %patch -p1 (and declared with patch<number>: filename).

          I propose to use a naming convention patch0_foo.diff ... patch1_bar.diff files (of patch -p1 format) in bigtop-packages/src/common . This would be automagically moved to debian/patches/ and the series file generated in the debian way. In RPM these files will be moved to SOURCES directory and have to be integrated into the SPEC file (that's tricky) .

          Show
          oflebbe Olaf Flebbe added a comment - - edited Basically the idea is to use automatic patching already provided by dpkg_source and rpmbuild Debian Patches are patch -p1 files in debian/patches/xxx and a debian/patches/series file stating the order of how to apply patches. This is conceptually very similar to the RPM spec file approach naming indiviual patches RPMbuild Patches can be applied with %patch -p1 (and declared with patch<number>: filename). I propose to use a naming convention patch0_foo.diff ... patch1_bar.diff files (of patch -p1 format) in bigtop-packages/src/common . This would be automagically moved to debian/patches/ and the series file generated in the debian way. In RPM these files will be moved to SOURCES directory and have to be integrated into the SPEC file (that's tricky) .
          Hide
          oflebbe Olaf Flebbe added a comment -

          I'm working on this ... but I cannot change assignment

          Show
          oflebbe Olaf Flebbe added a comment - I'm working on this ... but I cannot change assignment
          Hide
          rvs Roman Shaposhnik added a comment -

          Agree with Cos. Olaf Flebbe feel free to assign this one to yourself if you plan to work on it.

          Show
          rvs Roman Shaposhnik added a comment - Agree with Cos. Olaf Flebbe feel free to assign this one to yourself if you plan to work on it.
          Hide
          cos Konstantin Boudnik added a comment -

          Good idea indeed! In fact that's exactly why the patching mechanism weren't implemented in the current gradle build - as the possibility about using the standard patching apparatus was discussed before.

          Show
          cos Konstantin Boudnik added a comment - Good idea indeed! In fact that's exactly why the patching mechanism weren't implemented in the current gradle build - as the possibility about using the standard patching apparatus was discussed before.

            People

            • Assignee:
              oflebbe Olaf Flebbe
              Reporter:
              oflebbe Olaf Flebbe
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development