Skip to main content

[pkg-discuss] Re: review request for 17173472 - pkg change-variant should allow variant removal

  • From: Xiaobo Shen < >
  • To:
  • Subject: [pkg-discuss] Re: review request for 17173472 - pkg change-variant should allow variant removal
  • Date: Mon, 21 Oct 2013 10:41:00 -0700

On 10/ 4/13 04:37 PM, Xiaobo Shen wrote:
On 10/ 4/13 02:26 PM, Edward Pilatowicz wrote:
On Fri, Oct 04, 2013 at 01:08:51PM -0700, Xiaobo Shen wrote:
On 10/ 3/13 03:47 PM, Danek Duvall wrote:
Xiaobo Shen wrote:

Alta helped modifying the man page. here is a new webrev. Please take a
look. Thanks.
https://ips.java.net/webrev/xiaoshen/Bug_17173472_pkgvariant3/
Your webrev is against a very old version of the gate, which doesn't
include all the changes I synced in recently. Please pull, merge, and post
a new webrev.

Thanks,
Danek
Ok. Here is an updated one. Thanks.
https://ips.java.net/webrev/xiaoshen/Bug_17173472_pkgvariant5/

out of curiosity i took a quick look at this.

- in image_config_update() you look in new_variants for variants with a
   value of "NONE".  i may be mistaken, but i don't think you'll ever
   find any because in make_change_varcets_plan() you removed them all.
   (the "variants" variable in make_change_varcets_plan() becomes the
   new_variants variable you see later in image_config_update().

- you've changed the behavior of make_change_varcets_plan() such that it
   could have side effects.  previously, it never changed the "variants"
   dictionary that was passed in (it created a new dictionary with
   different contents).  with your changes you're now modifying the
   dictionary that was passed in by the caller.

ed

- in image_config_update() you look in new_variants for variants with a
  value of "NONE".  i may be mistaken, but i don't think you'll ever
  find any because in make_change_varcets_plan() you removed them all.
  (the "variants" variable in make_change_varcets_plan() becomes the
  new_variants variable you see later in image_config_update().

Nope. Only non-existing variants which is about to be set to none was removed in make_change_varcets_plan().

- you've changed the behavior of make_change_varcets_plan() such that it
  could have side effects.  previously, it never changed the "variants"
  dictionary that was passed in (it created a new dictionary with
  different contents).  with your changes you're now modifying the
  dictionary that was passed in by the caller.

I created a local variable for this case. Please see the new webrev:
https://ips.java.net/webrev/xiaoshen/Bug_17173472_pkgvariant6/

Thanks,
Xiaobo

Need more comments for this one. Thanks a lot.

xiaobo


[pkg-discuss] Re: review request for 17173472 - pkg change-variant should allow variant removal

Xiaobo Shen 10/03/2013

[pkg-discuss] Re: review request for 17173472 - pkg change-variant should allow variant removal

Danek Duvall 10/03/2013

[pkg-discuss] Re: review request for 17173472 - pkg change-variant should allow variant removal

Xiaobo Shen 10/04/2013

[pkg-discuss] Re: review request for 17173472 - pkg change-variant should allow variant removal

Edward Pilatowicz 10/04/2013

[pkg-discuss] Re: review request for 17173472 - pkg change-variant should allow variant removal

Xiaobo Shen 10/04/2013

[pkg-discuss] Re: review request for 17173472 - pkg change-variant should allow variant removal

Edward Pilatowicz 10/04/2013

[pkg-discuss] Re: review request for 17173472 - pkg change-variant should allow variant removal

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