Skip to main content

[pkg-discuss] Re: review request: linked image facet inheritance

  • From: Shawn Walker < >
  • To:
  • Cc: Edward Pilatowicz < >
  • Subject: [pkg-discuss] Re: review request: linked image facet inheritance
  • Date: Sun, 04 Aug 2013 19:26:41 -0700

On 07/26/13 18:28, Edward Pilatowicz wrote:
thanks for the quick review bart.
replies are inline below.

a new webrev is available at:
        https://ips.java.net/webrev/edp/15743323.2/

doc/client_api_versions.txt:
  general: none of the sentences following the first
    in each paragraph are indented as the other
    entries in this file are

  line 16: s/so we're/we're/

  line 19: s/./;/


src/client.py:
  line 4745: I see you accounted for list_masked here, but
    we don't actually have any command line options for it
    or do anything with it.  Any need to keep this?

  line 4850: s/"values"/"value"/

src/man/pkg.1:
  line 1739: What made you choose to have it list all facets,
    instead of making it list only the masked ones or having
    it be an option that combined with '-a' or '-i' or the
    default output?  I'm not saying this approach is wrong,
    I'm just looking for reasoning and thoughts.


src/man/pkg.5:
  line 1203: s/.  /. /  (man page convention is to use one space)


src/modules/actions/generic.py:
  line 628: They're really unnecessary because we don't need them
    for conflict checking, right?  Perhaps it would be useful to
    state that as currently you might be left wondering why they're
    unnecessary.

  line 629: s/that actions has/those actions have/


src/modules/client/api.py:
  line 894: A *brief* comment explaining why it's ok to ignore wildcard
    matches and fallback to _match_src might be helpful.


src/modules/client/imageplan.py:
  line 352: I'd feel safer with range(2); we'd be guaranteed to never
    infinite loop.  Then you could put an assert after the loop to
    ensure we never retried more than we expected.

  line 398: extra newline

  line 462: s/Solve./Solve;/

  lines 572-601:
    What about pkg change-facet for a masked facet?

    In other words, if I make a local facet change, but all
    affected facets are already masked, and --reject wasn't
    used, then that should always be the fastpath.  That
    doesn't seem to be accounted for here.

    Likewise, if I make a local facet change, but I've already
    inherited that effective setting, I see no need to invoke
    the solver or load package data.

    If that looks too complicated (wildcards could be a problem?)
    then file an RFE for that.

  line 654: Is this another bug fix?  If so, oops.  Thanks for that.
    A separate bugdb entry might be nice to spotlight that.

  general: thanks for the fastpath cleanup; that's better now;
    wouldn't have thought of arranging it that way

  line 1906: s/=/ = /

  line 3545: Oh...how did this manifest itself?


src/modules/client/linkedimage/common.py:
  line 3213: s/package wide/package-wide/

  lines 3219-3220: Using the 'known' catalog from the image here
    should be faster and use less memory than using the manifest.
    I'd check; also, you really care about faceted actions not for
    the current variant?  Or was that just to avoid variant change
    issues?

  line 3268: Same as above; I'd expect it to be better to use catalog
    instead.  Also not thrilled about adding is_constrained() to
    Manifest; seems awfully specific to solver / linked image stuff.
    And the name is fairly generic for something specific to parent
    dependencies.  I'd suggest "is_own_grandfather()" but that would
    be silly :-)


src/modules/client/plandesc.py:
  line 547: No _() concerns?

  line 750: s/this the/this is the/ ?


src/modules/facet.py:
  lines 46-50: I want to make 'facet.debug' False by default in the
    future; do you foresee any issues with the changes made here
    with that?

  lines 278-280: You saw this right?  No concerns?

lines 357-380: Since this is only being used for incorporate dependency actions, this being written in Python is probably fine. But if we were to ever apply this to more than just those, I'd rather this be rewritten in CPython just as the existing facet matching is done today. It really does make a big difference.

lines 395, 408: No way to raise the exception directly instead of generating on synthetically?

  line 491: s/read only/read-only/

  line 497: Insert newline above please.


src/modules/manifest.py:
  lines 859-864: This is a hot path; it's unfortunate that we've had
    to add attribute filtering here.  To mitigate that, setting
    attr_filter=None and then doing a boolean check on attr_filter
    might be faster.  To test, you could add something to
    tests/perf/actionbench.py or create a new 'manbench.py'.

  line 870:   Do we really need to check for programmer error here?
    Wasn't this primarily useful for debugging?


src/tests/cli/t_pkg_linked.py:
  line 1302: s/childred/children/ ?

  lines 1634, 1688, 1698, 1711, 1763, 1773, 2324, 2394, 2403:
    s/we/when we/ ?

  lines 1245, 1312, 2532, 2869, 2871, 3131: s/it's/its/

  general: I did not look at every test in detail, I only looked
    to see if you had sufficient coverage.

  Also, what happens in the situation that an admin disables the
  version-lock facet for the cacao-incorporation and then updates
  entire with a specific version?  I'm guessing the install holds
  don't get relaxed.  If so, that's ok; same thing happens now.
  We should talk to Bart about whether install-hold relationships
  should always apply even if the incorporate dependency is faceted
  out.

src/tests/cli/t_pkg_varcet.py:
  line 624: s/uninstalled/uninstall/ ?

Eyes glazed over,
-Shawn


[pkg-discuss] Re: review request: linked image facet inheritance

Shawn Walker 08/05/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Edward Pilatowicz 08/06/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Shawn Walker 08/06/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Edward Pilatowicz 08/07/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Shawn Walker 08/07/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Edward Pilatowicz 08/08/2013
 
 
Close
loading
Please Confirm
Close