Details

    • Type: Sub-task Sub-task
    • Status: In Progress
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.8.0
    • Fix Version/s: 1.1.0
    • Component/s: build, debian
    • Labels:
      None

      Description

      Right now Maintainer is hardcoded,we need to make the DEBIAN file flexible Maintainer with variable substitutions, split this jira from BIGTOP-1251 due to debian have different variable substitutions with RPM

      1. BIGTOP-1264
        26 kB
        Wenwu Peng
      2. BIGTOP-1264.1.patch
        26 kB
        Wenwu Peng
      3. BIGTOP-1264.2.patch
        25 kB
        Wenwu Peng
      4. BIGTOP-1264.3.patch
        25 kB
        Wenwu Peng
      5. BIGTOP-1264.4.patch
        27 kB
        Wenwu Peng

        Activity

        Hide
        Wenwu Peng added a comment -

        Provide the patch include maintainer with variable substitutions by pass the maintainer variable in project.substvars, will replace the variable in pkg-gencontrol phase.

        please review this mechanism is ok or not. then will continual to next phase work

        need to improvement:
        1. where to put the more common script/file, which need to use in all sub-project/sub-directories(currently, put it under template)

        Show
        Wenwu Peng added a comment - Provide the patch include maintainer with variable substitutions by pass the maintainer variable in project.substvars, will replace the variable in pkg-gencontrol phase. please review this mechanism is ok or not. then will continual to next phase work need to improvement: 1. where to put the more common script/file, which need to use in all sub-project/sub-directories(currently, put it under template)
        Hide
        Konstantin Boudnik added a comment -

        the patch is ready for some time, I presume. Can anyone with good deb knowledge look at it so it can be cleared our of the way? Moving to 0.9.0 in the meanwhile.

        Show
        Konstantin Boudnik added a comment - the patch is ready for some time, I presume. Can anyone with good deb knowledge look at it so it can be cleared our of the way? Moving to 0.9.0 in the meanwhile.
        Hide
        Peter Linnell added a comment -

        +1 looks sane, but where is MAINTAINER set ?

        Show
        Peter Linnell added a comment - +1 looks sane, but where is MAINTAINER set ?
        Hide
        Wenwu Peng added a comment -

        Thanks a lot Peter for review.
        To keep consistent with RPM, so reuse the variable +VENDOR=Apache Bigtop in bigtop.mk
        and pass it --set-envvar=MAINTAINER="$(VENDOR)$(VENDOR_MAIL)" \ in package.mk

        Show
        Wenwu Peng added a comment - Thanks a lot Peter for review. To keep consistent with RPM, so reuse the variable +VENDOR=Apache Bigtop in bigtop.mk and pass it --set-envvar=MAINTAINER="$(VENDOR)$(VENDOR_MAIL)" \ in package.mk
        Hide
        Mark Grover added a comment -

        Thanks Wenwu!

        2 comments:

        -	  cp $(BASE_DIR)/bigtop-packages/src/templates/init.d.tmpl debian && \
        +	  cp -r $(BASE_DIR)/bigtop-packages/src/templates/* debian && \
        

        I'd really prefer if you selectively copy only the things you need from that directory. We may add more things later, and this may break it down the road.

        +				--set-envvar=MAINTAINER="$(VENDOR)$(VENDOR_MAIL)" \
         				-uc -us -b
        

        I thought there was a space between the VENDOR and the VENDOR_MAIL, no?

        Show
        Mark Grover added a comment - Thanks Wenwu! 2 comments: - cp $(BASE_DIR)/bigtop-packages/src/templates/init.d.tmpl debian && \ + cp -r $(BASE_DIR)/bigtop-packages/src/templates/* debian && \ I'd really prefer if you selectively copy only the things you need from that directory. We may add more things later, and this may break it down the road. + --set-envvar=MAINTAINER= "$(VENDOR)$(VENDOR_MAIL)" \ -uc -us -b I thought there was a space between the VENDOR and the VENDOR_MAIL, no?
        Hide
        Mark Grover added a comment -

        Also, can you please help me understand what this line does?

        +echo "misc:Depends=" >> debian/${PROJECT_NAME}.substvars
        

        Also, while we don't mandate it, for reviews like this, some times uploading the review on reviews.apache.org helps so I don't have to copy paste lines for context when I have a question for you. It's too late for this review anyways, but just fyi

        Show
        Mark Grover added a comment - Also, can you please help me understand what this line does? +echo "misc:Depends=" >> debian/${PROJECT_NAME}.substvars Also, while we don't mandate it, for reviews like this, some times uploading the review on reviews.apache.org helps so I don't have to copy paste lines for context when I have a question for you. It's too late for this review anyways, but just fyi
        Hide
        Peter Linnell added a comment -

        Good catch Mark.

        "I'd really prefer if you selectively copy only the things you need from that directory. We may add more things later, and this may break it down the road."

        Agreed. RPM stuff should always be declaritive. It is safer.

        Show
        Peter Linnell added a comment - Good catch Mark. "I'd really prefer if you selectively copy only the things you need from that directory. We may add more things later, and this may break it down the road." Agreed. RPM stuff should always be declaritive. It is safer.
        Hide
        Roman Shaposhnik added a comment -

        The patch looks OK baring the comments made by Mark.

        Also, this is the first time we're introducing pre-processing of Debian package files. Not sure how to feel about that – but on the other hand we already do it for RPMs.

        Show
        Roman Shaposhnik added a comment - The patch looks OK baring the comments made by Mark. Also, this is the first time we're introducing pre-processing of Debian package files. Not sure how to feel about that – but on the other hand we already do it for RPMs.
        Hide
        Konstantin Boudnik added a comment -

        Is it going to be updated anytime soon?

        Show
        Konstantin Boudnik added a comment - Is it going to be updated anytime soon?
        Hide
        Wenwu Peng added a comment -

        Thanks a lot for your great comments all, very busy recently,will update the patch follow comments and push to reviewboard ASAP

        Show
        Wenwu Peng added a comment - Thanks a lot for your great comments all, very busy recently,will update the patch follow comments and push to reviewboard ASAP
        Hide
        Roman Shaposhnik added a comment -

        One quick question: why bigtop-jsvc application of gen-control is commented out?

        Otherwise I like the patch. +1 provided that the question above gets answered.

        Show
        Roman Shaposhnik added a comment - One quick question: why bigtop-jsvc application of gen-control is commented out? Otherwise I like the patch. +1 provided that the question above gets answered.
        Hide
        Wenwu Peng added a comment -

        Thanks a lot for careful review Roman!. it is mistake to comment jsvc.
        And try to push patch to reviews.apache.org for review. however, I am failed due to some reason. (not patch show in reviews.apache.org after select patch file)

        Show
        Wenwu Peng added a comment - Thanks a lot for careful review Roman!. it is mistake to comment jsvc. And try to push patch to reviews.apache.org for review. however, I am failed due to some reason. (not patch show in reviews.apache.org after select patch file)
        Hide
        Roman Shaposhnik added a comment -

        Wenwu Peng don't worry about the reviewboard. Personally I only use it for really complicated stuff. Thanks for fixing the issue I mentioned and sorry about not mentioning another issue that'd be awesome to fix: gradle. Bigtop 0.9.0 is going to get rid of Makefile logic and use Gradle build exclusively. Can you please update your patch to also add copying logic to the gradle script?

        Show
        Roman Shaposhnik added a comment - Wenwu Peng don't worry about the reviewboard. Personally I only use it for really complicated stuff. Thanks for fixing the issue I mentioned and sorry about not mentioning another issue that'd be awesome to fix: gradle. Bigtop 0.9.0 is going to get rid of Makefile logic and use Gradle build exclusively. Can you please update your patch to also add copying logic to the gradle script?
        Hide
        Wenwu Peng added a comment -

        Add copying logic to the gradle script.
        look like will get error "dpkg-buildpackage: unknown option or argument <dev@bigtop.apache.org>" if we add space between "Bigtop <dev@bigtop.apache.org>"

        Show
        Wenwu Peng added a comment - Add copying logic to the gradle script. look like will get error "dpkg-buildpackage: unknown option or argument <dev@bigtop.apache.org>" if we add space between "Bigtop <dev@bigtop.apache.org>"
        Hide
        Konstantin Boudnik added a comment -

        Is anything happening here? Or shall we push it into the next version?

        Show
        Konstantin Boudnik added a comment - Is anything happening here? Or shall we push it into the next version?
        Hide
        Konstantin Boudnik added a comment -

        Looks like it isn't going to happen in 1.0 - moving out

        Show
        Konstantin Boudnik added a comment - Looks like it isn't going to happen in 1.0 - moving out

          People

          • Assignee:
            Wenwu Peng
            Reporter:
            Wenwu Peng
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:

              Development