On 07/14/10 05:37 PM, [email protected] wrote:
On Wed, Jul 14, 2010 at 03:59:32PM -0700, Shawn Walker wrote:
On 07/14/10 03:45 PM, [email protected] wrote:
...
On Tue, Jul 13, 2010 at 05:22:42PM -0700, Shawn Walker wrote:
On 07/13/10 11:53 AM, [email protected] wrote:
The webrev is here:
http://cr.opensolaris.org/~johansen/webrev-xpub-1/
...
======================================================================
src/modules/client/transport/repo.py:
======================================================================
line 598: missing spaces after '{' and before '}'. Also, it seems
like you could trivially add support for add to catalog here, by
(adding) and checking for X-IPkg-Add-To-Catalog.
I added the spaces, but I'm confused about the rest of this comment.
I re-implemented the transport parts here based upon what was in
FileTransaction and HTTPTransaction. The HTTPTransaction class didn't
support the add to catalog method. Are you saying that it should? Do I
need to make depot changes to enable this?
Right, the publication API didn't yet support add to catalog
(--no-catalog in pkgsend terms) but it's trivial to add this simply
by checking for the header in the transport system and in close_0 in
server/depot.py.
I know this is an RFE technically, but I felt since with very little
code we could add feature parity between file and http transport
access it was worth doing.
I'm not insisting you have to do this in this changeset, it could
certainly be deferred, but it seemed right to do this now while
reving the transport usage.
Note that for the same reasons we didn't rev /close/ (that is 0 ->
1) for X-IPkg-Refresh-Index, I don't think we need to for
X-IPkg-Add-To-Catalog.
Ok, this makes sense. All I need to do is pass the header from client
to server, and then on the depot if I see the header call
repo.close(add_to_catalog=True). Correct?
Correct.
======================================================================
src/modules/client/transport/transport.py:
======================================================================
...
line 2051: so why cycle through origins for publication operations?
it seems like we should never have more than one origin for
publication. Same for other publication opeartions on 2096, etc.
The __gen_origins code is also responsible for retrying at repositories
where there are failures. This is used so that we retry a destination
more than once. Multiple destinations probably isn't a good idea.
Ah, ok. A comment stating that the iteration was to ensure we retry
for failure as opposed to retrying different origins would be
helpful. Plus, there should probably be an assertion somewhere that
there is only one origin for publication operations.
I can make this change, but what I'll probably have to do is call a
separate routine that checks the length of the origins list for
publication operations that have supplied a Publisher object as an
argument to the transport. If that sounds too convoluted, let me know.
However you want to do it is fine with me.
======================================================================
src/util/distro-import/importer.py:
======================================================================
...
line 1406: replace 'lib/pkg.depotd' with 'bin/pkgrepo'; also update
docstring on line 1404
line 1407: replace with:
argstr = "%s -s %s refresh" % (cmdname, path_to_repo)
These two are non-trivial changes to validate. It took me a full day
just to get the importer working to the point where I could run a test.
Since I've already tested this with pkg.depotd, I'd prefer not to change
this since I would have to re-test it. That takes hours to get set up.
I suppose since the importer is going away any day now that's ok,
but we should eventually change this. If you want to file a bug and
move on that's ok with me too.
What's going to replace the importer? I wasn't aware of such plans.
Err, I was speaking of the day when everyone delivered their software in
IPS format so we no longer needed the importer ;)
I should have said (air quotes) "any day now" ;)
I'll see if I have time to do this. Maybe the import will go faster now
that I've done it a couple of times before.
As I said, you don't need to change this now; it's totally optional and
nothing will break. Feel free to file a bug and move on.
======================================================================
src/util/publish/merge.py:
======================================================================
lines 182-231: The code in these seems common to our publication
utilities. Any way to centralise these somewhere? (misc? a new
module? perhaps pkg.util?) I realise the globals make this
difficult.
There have been some subtle variations in publisher setup between merge,
importer, and pull. I thought about commonizing this code, but I was
concerned that it would lead to a weirder method shared amongst them
all.
Ok, if you can find a way to do it without burning much time, I'm
happy. If not, move on...
Since you and Danek both mentioned this I should probably try to take
care of it. There are essentially two variations of setup_publishsers,
and I can re-write them into one common rotuine. The create_transport
routine should probably just configure an empty GenericTransportConfig
and have the caller customize its attributes. I'm not sure if that's
any better than what we have today.
For setup_publishers, the caller would need to pass the transport and
transport config, as well as another argument. If you're ok with a
routine that has 5 arugments, then I'll go ahead and make it common.
I'm a bit uncertain about the best place to put this, somewhere common
to all clients would make sense.
If a routine with five arguments saves us 50 lines of duplicated code,
I'm all for it :P
I think, like Danek, it should probably live in the transport module (or
perhaps transport.util?) as that's the only applicable usage case.
Cheers,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
