Skip to main content

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

  • From: Edward Pilatowicz < >
  • To:
  • Cc: Yiteng Zhang < >
  • Subject: [pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant
  • Date: Thu, 15 Aug 2013 16:27:13 -0700

On Mon, Aug 12, 2013 at 02:20:59PM -0700, Shawn Walker wrote:
> On 08/12/13 10:49, Yiteng Zhang wrote:
> >On 08/11/13 01:40 PM, Shawn Walker wrote:
> >>On 08/09/13 18:54, Yiteng Zhang wrote:
> >>>On 08/ 9/13 02:32 PM, Shawn Walker wrote:
> >>>>On 08/09/13 12:54, Yiteng Zhang wrote:
> >>>>>Hi guys,
> >>>>>     I just fixed thsi bug and push my code review to the webrev.
> >>>>>Please
> >>>>>take a look on it.
> >>>>>     http://ips.java.net/webrev/yitezhan/16986557_prefix_1/
> >>>>>
> >>>>>     One thing to mention, after I run the test scripts, it reported
> >>>>>some mismatches failures. It is because the test scripts still
> >>>>>expect to
> >>>>>see the output with prefixes. Thanks.
> >>>>
> >>>>
> >>>>src/client.py:
> >>>>  Unfortunately, this set of changes will drop the 'variant.' and
> >>>>'facet.' prefixes from more than just the default output.  The
> >>>> original bug requests that the prefixes are only omitted from
> >>>> the default output.
> >>>>
> >>>> In other words, you should only omit the prefixes if the
> >>>> output_format != "default".
> >>>>
> >>>> Also, as a matter of style, we generally do not use '' to delimit
> >>>> strings or even single characters; we use "" instead.  (As seen
> >>>> on lines 4800, 4801, 4811, 4812, 4890, 4891.)
> >>>>
> >>>>
> >>>>You should be able to fix all of the remaining tests by simply
> >>>>changing __assert_facet_matches and __assert_variant_matches in
> >>>>src/tests/cli/t_pkg_varcet.py to remove the 'facet.' or 'variant.'
> >>>>prefix from the exp_def string variable.
> >>>>
> >>>>-Shawn
> >>>Hi,
> >>>     Sorry about that I misunderstood the meaning of default output.
> >>>Thanks for your advise. Now I only omit the prefixed if the
> >>>output_format == "default" (I think you mean equal). Also, I modified
> >>
> >>Yes, I meant output_format == "default"; sorry about that.
> >>
> >>>the t_pkg_varcet.py in order to pass the test cases. My code review can
> >>>be seen at:
> >>>     http://ips.java.net/webrev/yitezhan/16986557_prefix_2/
> >>
> >>src/client.py:
> >>  This is now correct.
> >>
> >>src/tests/cli/t_pkg_varcet.py
> >>  line 119: We always place a space before and after operators such as
> >>     '-', '+', etc.  However, read on.
> >>
> >>  lines 118-121:  This could be more simply written as:
> >>
> >>    expected = "".join(
> >>        (prefix + l)
> >>        for l in expected.splitlines(True)
> >>    )
> >>
> >>After you make these changes, be certain you perform a full test suite
> >>run to ensure nothing else has broken.
> >>
> >>You can run the test suite faster by using the -j option to have it
> >>run tests in parallel where possible, like this:
> >>
> >>  python tests/run.py -j8
> >>
> >>Assuming the test suite passes, you can push your changeset.
> >>
> >>-Shawn
> >Hi Shawn,
> >     Thanks, I have made these changes and a full test suite was passed.
> >I wish I can push the code, but I am not authorized to write to the
> >repository ips~pkg-gate.
>
> As soon as you've had a few successfully reviewed changesets,
> someone will nominate you for commit access and you'll be able to
> putback. Until then, another pkg team member can push your changes.
>
> In this particular case, Ed is right, there's unfortunately some
> logic in the kernel zone installation scripts that currently relies
> on the 'facet.' prefixes that Ed and I both missed during our
> previous review of the zone scripts.
>
> Until 17301112 is fixed, we won't be able to push these changes.
>
> So for now, just hold onto your workspace and sometime during ON
> build 30, we should be able to push them.
>

hey Yiteng,

fyi, i just pushed 17301112 into ON gate for build s12_30.  so once the
pkg gate closes for s12_29 (and opens for s12_30) you should be OK to
push this fix.

ed


[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

(continued)

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Yiteng Zhang 08/10/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Shawn Walker 08/11/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Yiteng Zhang 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Edward Pilatowicz 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Erik Trauschke 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Edward Pilatowicz 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Shawn Walker 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Edward Pilatowicz 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Shawn Walker 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Yiteng Zhang 08/12/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Edward Pilatowicz 08/15/2013

[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant

Yiteng Zhang 08/15/2013
 
 
Close
loading
Please Confirm
Close