Re: [pkg-discuss] Code review for 13485
On 01/ 7/10 12:56 PM, Brock Pytlik wrote: I changed the code there because the function signature changed, so it seemed wise to change it in all the places it was called. I'm happy to a) just pretend the file doesn't exist and make no changes in it b) leave the changes in it in case we ever do make a trip through those lines c) delete the entire function in server/catalog.py. Just let me know which is the preferred way forward. (As a side note, I've just run the test suite and confirmed that it passes if the changes to that file are removed.) I'm fine with either path. I plan on nuking all the indexing code and some other bits in server/catalog.py very soon though, so you might as well just not change it at all. -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code review for 13485
Shawn Walker wrote: On 01/ 6/10 05:50 PM, Danek Duvall wrote: Shawn Walker wrote: On 01/ 5/10 01:04 PM, Danek Duvall wrote: Brock Pytlik wrote: http://cr.opensolaris.org/~bpytlik/ips-13485-v1/ indexer.py: - It could just be a slow brain, but this modifies the fmris in the catalog itself, right? Wait, fmris() uses the old catalog file, so how does this work at all? fmris() doesn't use the "old catalog file". Confusion here? Ignore server/catalog.py; it isn't used here at all. It's kind of hard to ignore; Brock is changing it! Will ServerCatalog.refresh_index() ever be called? If not, then can Brock just eliminate that change? If so, then it calls self.fmris(), which very definitely opens a file called "catalog" and expects it to have entries like "V pkg ..." in it. What else am I missing? You're not missing anything; there's no point in changing server/catalog.py. Most of that's essentially dead code. I swear I filed a cleanup bug for this, but can't find it at the moment. However, I intend to nuke most of that soon. I changed the code there because the function signature changed, so it seemed wise to change it in all the places it was called. I'm happy to a) just pretend the file doesn't exist and make no changes in it b) leave the changes in it in case we ever do make a trip through those lines c) delete the entire function in server/catalog.py. Just let me know which is the preferred way forward. (As a side note, I've just run the test suite and confirmed that it passes if the changes to that file are removed.) Brock ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code review for 13485
On 01/ 6/10 05:50 PM, Danek Duvall wrote: Shawn Walker wrote: On 01/ 5/10 01:04 PM, Danek Duvall wrote: Brock Pytlik wrote: http://cr.opensolaris.org/~bpytlik/ips-13485-v1/ indexer.py: - It could just be a slow brain, but this modifies the fmris in the catalog itself, right? Wait, fmris() uses the old catalog file, so how does this work at all? fmris() doesn't use the "old catalog file". Confusion here? Ignore server/catalog.py; it isn't used here at all. It's kind of hard to ignore; Brock is changing it! Will ServerCatalog.refresh_index() ever be called? If not, then can Brock just eliminate that change? If so, then it calls self.fmris(), which very definitely opens a file called "catalog" and expects it to have entries like "V pkg ..." in it. What else am I missing? You're not missing anything; there's no point in changing server/catalog.py. Most of that's essentially dead code. I swear I filed a cleanup bug for this, but can't find it at the moment. However, I intend to nuke most of that soon. -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code review for 13485
Shawn Walker wrote: > On 01/ 5/10 01:04 PM, Danek Duvall wrote: > >Brock Pytlik wrote: > > > >>http://cr.opensolaris.org/~bpytlik/ips-13485-v1/ > > >indexer.py: > > > > - It could just be a slow brain, but this modifies the fmris in the > > catalog itself, right? Wait, fmris() uses the old catalog file, so how > > does this work at all? > > fmris() doesn't use the "old catalog file". Confusion here? Ignore > server/catalog.py; it isn't used here at all. It's kind of hard to ignore; Brock is changing it! Will ServerCatalog.refresh_index() ever be called? If not, then can Brock just eliminate that change? If so, then it calls self.fmris(), which very definitely opens a file called "catalog" and expects it to have entries like "V pkg ..." in it. What else am I missing? Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code review for 13485
On 01/ 5/10 01:04 PM, Danek Duvall wrote: Brock Pytlik wrote: http://cr.opensolaris.org/~bpytlik/ips-13485-v1/ frmi.py: - line 302: let's call this "remove_publisher" and put this up with set_publisher(). Agree. indexer.py: - It could just be a slow brain, but this modifies the fmris in the catalog itself, right? Wait, fmris() uses the old catalog file, so how does this work at all? fmris() doesn't use the "old catalog file". Confusion here? Ignore server/catalog.py; it isn't used here at all. We have a bug open to clean this up. The server only uses modules/catalog.py; same as the client now. server/catalog.py is only used in specific cases where we are upgrading a repository to v1 or for a client communicating with a v0 repository. The changeset here looks fine otherwise. Cheers, -- Shawn Walker ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code review for 13485
Brock Pytlik wrote: > > - It could just be a slow brain, but this modifies the fmris in the > >catalog itself, right? Wait, fmris() uses the old catalog file, so > >how does this work at all? > From catalog.py, the comment from the fmris function says: > > A generator function that produces FMRIs as it iterates over the contents of > the catalog. Yeah, but then it opens a "catalog" file and proceeds to treat it as a v0 catalog. Which kinda worries me. But if you're getting valid data from this, then I guess it's working and I just don't understand how. Unless you can explain what's going on here, I'll want Shawn to weigh in. I do know there's still a bit of vestigial catalog code, and some which is used only for v0 catalogs, and I'm not sure how fmris() is classified. > fmris calls __parse_entry which creates a new pkg fmri and returns it. > Now, if there's a concern that we're caching these implictly somehow or > using a singleton pattern with them, I can have what will be called > "remove_publisher" return a copy of the fmri with the publisher removed, > but unless something fancy is going on under the covers, I don't see the > concern here. Assuming that fmris() is pulling from a useful data source, then I agree, there's no problem using the fmri objects it generates. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code review for 13485
Danek Duvall wrote: Brock Pytlik wrote: http://cr.opensolaris.org/~bpytlik/ips-13485-v1/ frmi.py: - line 302: let's call this "remove_publisher" and put this up with set_publisher(). Sure indexer.py: - It could just be a slow brain, but this modifies the fmris in the catalog itself, right? Wait, fmris() uses the old catalog file, so how does this work at all? From catalog.py, the comment from the fmris function says: A generator function that produces FMRIs as it iterates over the contents of the catalog. So, I think it's still perfectly valid... If it's not then shouldn't it be removed from the catalog module and an equivalent replacement possibly provided? As I don't have my head totally around the catalog code, I'll defer to Shawn's opinion on whether or not this function is still valid to call. fmris calls __parse_entry which creates a new pkg fmri and returns it. Now, if there's a concern that we're caching these implictly somehow or using a singleton pattern with them, I can have what will be called "remove_publisher" return a copy of the fmri with the publisher removed, but unless something fancy is going on under the covers, I don't see the concern here. Brock Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code review for 13485
Brock Pytlik wrote: > http://cr.opensolaris.org/~bpytlik/ips-13485-v1/ frmi.py: - line 302: let's call this "remove_publisher" and put this up with set_publisher(). indexer.py: - It could just be a slow brain, but this modifies the fmris in the catalog itself, right? Wait, fmris() uses the old catalog file, so how does this work at all? Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
