On 06/24/14 15:00, Yiteng Zhang wrote:
On 06/23/14 04:08 PM, Danek Duvall wrote:
Yiteng Zhang wrote:
- line 242: doesn't fix now get all the usual BE management flags
(--no-backup-be, etc)? If not, shouldn't it?
- line 1627: if _op not in (PKG_OP_REVERT, PKG_OP_FIX, ...):
- line 1851ff: this seems like the wrong place for this; it should be
done in the dehydrate/rehydrate API calls, or possibly deeper.
Worry that users/machines will interrupt the execution of dehydrate/
rehydrate. If so, the property of "dehydrated" should not be saved
because rehydrate depends on this property. So this is probably
a good place to put the code -- test the return code of __api_execute_plan
and save the property.
- There are a lot of changes here which are unrelated to hydration;
and why did they get combined?
Alta answered this: these changes went into 11.2 but not into 12. I agree
to push these changes to 12 via a different changeset first.
- line 404: a couple of formatting and wording changes:
This attribute is used to prevent a file from being removed
a \fBdehydrate\fR operation. The value of the \fBdehydrate\fR
attribute can be \fBtrue\fR (the default) or \fBfalse\fR. If
\fBfalse\fR, \fBpkg dehydrate\fR will not remove the file.
- line 2221: "... to dehydrate on" -> "... to dehydrate"
- line 524, 697, 702: self.uncofigured_pubs -> self.unconfigured_pubs
- line 700: " on." -> "."
Frankly, I'd rather see a boolean dehydrated property on each
I think that it makes more sense -- when listing the properties of a
publisher, this is logically one of them, and having to make the
look in one place (the publisher object) for most of the properties
in another (the image) for the last one seems like bad architecture to
When do we want to mark the publisher, at the time we check the publisher
object before pkgplan creation of dehydrate/rehydrate, or after api
execution of dehydrate/rehydrate and create the publisher object again?
the latter one makes more sense, and it also relates to the above problem:
where to put the code of saving property.
That said, I still have comments on the code here.
- line 2371: using a mutable object as a default argument is bad form,
since once you change it, it's changed, even for future calls. Use
None -- check_publishers() already does a boolean test on it, so
should just work (unless check_publishers() can return its input
instead of an empty list).
- line 2374: "'publisher'" -> "'publishers'". Also, this doesn't
actually perform the dehydrate/rehydrate operation, so you should
that the publishers are being marked, not that that's the action
- line 2380: I'd do this with a try/except as well.
- line 2383: I'd use ast.literal_eval() here, instead; I think it'd be
- line 2384ff: I'd use a generator expression or sets here:
pubs.extend((p for p in publishers if p not in pubs))
pubs = list(set(pubs) + set(publishers))
Similarly for the removal, but you'd have to use sets there.
- line 2400: I don't understand -- "manifest files"? Perhaps just
"remove the property entirely"?
- line 2407: "yes" -> "so"; "exceptions" -> "exception"
- line 2409: strike "added".
- line 2412: Use a multi-line form:
blah = [
for var in iter
- line 2415ff: I'd probably do something like this:
if not publishers:
unconf_pubs = [ pub for pub in publishers if pub not in
I'm not sure I really like the way these two functions are
more generally, but I'm also not certain I have a better way of
- line 4490, 4491: why is this check here?
I knew api will have this check in execute(), but the old pkg fix had
this check at the
very beginning of method __repair() and __repair() also invoked
execute(). I inclined
to remove this check here though.
- line 1324: do we want to count the number of installed packages,
number of installed packages that will get dehydrated?
- line 1331: use iterkeys() instead of keys(); it'll take less memory.
- line 1332: is this a fast enough loop that it doesn't need any
tracking while we're in it?
I wonder if __gen_hydrate_actions() should be a generator, that
its dictionary entries, rather than returninga dict. That way,
save some memory by not having that entire dict in memory at the
time as the list of package plans; and we can update the progress
tracker once for each manifest generation / pkgplan creation step.
-line 1324, line 1332
I will use a generator to yield fmri, manifest and its dictionary entries.
To count the number of installed packages that will get dehydrated at
the first place,
we have to call __gen_hydrate_actions to set the value of "goal" for
And we will call __gen_hydrate_actions again for pkgplan creation.
So I think it is better to: count the number of all installed packages and
update the progress tracker once for each package's fmri retrieval;
also add progress tracking without incrementing tracker's nitems in the
of pkgplan creation.
- line 1389: I'd rename "pt" to "tracker" or "progtrack" or
bit more mnemonic.
- line 1364ff: this is another place that could stand to be de-looped:
pubs = [pub for pub in pubs if pub not in dehydrated_pubs]
And the idiom for copying a list is to use a slice -- l[:] -- not a
- line 1390: need a space before the open parenthesis.
- line 1391: quotes around "pubs". Is the mention of zone config
(here and on line 1406) relevant anymore? You're not checking for
zones config files, you're checking for actions with
which may be more than just those.
- line 1416, 1428: why do you need this test for file existence?
racy. Is it faster to do it this way than to just try to remove the
file during plan execution and ignore failure if it's already gone?
Without this test for file existence, dehydrate will always propose actions
to add and return an exit code 0. But if all the files are gone, dehydrate
should not add any action to the dictionary and should return an exit
which stands for "nothing to do".
I realized another point you guys raised in the meeting: if a publisher is
dehydrated, installation of future packages should be installed dehydrated.
Please note that I implemented and prefer that installation of future
are installed normally and its publisher is still dehydrated.
As we said in the meeting, the "principle of least surprise" applies here. All packaging operations for a dehydrated publisher should also be automatically dehydrated. We really need to change the behaviour here.
[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate
Message not available
Message not available