Skip to main content

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

  • From: Erik Trauschke < >
  • 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: Mon, 21 Oct 2013 11:17:04 -0700



On 10/21/13 10:58 AM, Yiteng Zhang wrote:
On 10/ 2/13 12:02 PM, Yiteng Zhang wrote:
On 08/15/13 04:35 PM, Yiteng Zhang wrote:
On 08/15/13 04:27 PM, Edward Pilatowicz wrote:
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
Hi Edward,
I appreciate it. I will check the bug again when s12_30 opens. Thanks.

Yiteng

Hi all,

I think the pkg gate is open for s12_30 now. I have tested this fix
and would like to push it. Thanks.

Yiteng



Hi,

Can everyone take a look on it and help me to push this fix?

Where you ever able to actually install a kernel zone successfully? Even if there is a very minor risk of this breaking zones you should do at least a manual test.
I know you had some issues testing zones for a different bug so I was curious if you ever hashed that out. It seems like a good idea to just do a quick zone test with your chances before you put back.

Erik


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

Yiteng Zhang 10/02/2013

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

Yiteng Zhang 10/21/2013

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

Erik Trauschke 10/21/2013

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

Yiteng Zhang 10/21/2013

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

Erik Trauschke 10/21/2013

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

Xiaobo Shen 10/21/2013
 
 
Close
loading
Please Confirm
Close