Re: apt

[Jason Gunthorpe <jgg@ualberta.ca>]

On Thu, 31 Aug 2000, Alfredo Kengi Kojima wrote:

> The code still needs quite some cleaning, but here's the patch.
> I've also attached a dia/(simplified)UML diagram of the 
> classes in apt-pkg I made, so I could understand the code easier. 
> It contains most of the new classes.

Hm, the one in CVS is interesting, I should look at it closer soon..

> The patch is probably not very readable, but I've put the
> code in cvs, so you can take a look from there. As I've said,

The patch, is useless, you must have passed the wrong options to diff or
something, it contained two copies of every files.. Anyhow, I'm looking at
your CVS.

> the code still needs cleaning and it's under heavy development,
> so I don't expect you to accept it for merging into your tree

Well, lets see here, I'm just going to go over what you have and rattle
off what I see here..

* Root Dir:
  Dump the RPM specific stuff into an RPM toplevel dir. I like small
  directories.
* Build Dir:
   -CPPFLAGS+= -I$(INCLUDE) -I/usr/include/rpm
   +CPPFLAGS+= -I$(INCLUDE)
 Eek! nononono, use #include <rpm/foo.h>, do not do that. Especially 
 do not do that in the file where you have done that. 

-ONLYSTATICLIBS = yes
-#ifeq ($(HOST_OS),linux-gnu)

 Why? You do know to use the shared libaries you just use "export
 LD_LIBRARY_PATH=`pwd`" in the build dir.. Works with gdb, etc, etc.

* Methods:
-RPMLIBS=-lrpm -lpopt -static -lz -ldb1 -lbz2
 
 Bleck. shlibs, there for a reason. I had a better way to solve that
 general problem, but it escapes me now.

* Tools:
  + Needs to use proper make files
  + Is not C++
  + Much of it appears to belong in test/ 

* Cmdline:
  + I think your changes to apt-cache may be too extensive. I need to look 
    closer but it seems the majority of them need to be pushed into the
    rpm library code, particuarly the format-as-text bit.

-   if (1) {
-      ConectivaFactory *factory = new ConectivaFactory;
-   } else {
-      DebianFactory *factory = new DebianFactory;
-   }

  + Heh, that's just bad style :>

  Otherwise looks mostly OK, I have some more specific comments below..

* apt-pkg:
-   if (0) {//akk
-      StoreFilename = QuoteString(Version.ParentPkg().Name(),"_:") + '_' +
-       QuoteString(Version.VerStr(),"_:") + '_' +
-       QuoteString(Version.Arch(),"_:.") + ".deb";
-   } else {
-      StoreFilename = QuoteString(Version.ParentPkg().Name(),"_:") + '-' +
-       QuoteString(Version.VerStr(),"_:") + '.' +
-       QuoteString(Version.Arch(),"_:.") + ".rpm";
-   }
  Yuk, change this to grab the extension of the original file and
  preserve it instead of *that*. I'll accept a mini patch to do that
  immediately.

-      if (_config->FindB("APT::Ignore-Hold",false) == false)
+      if (_config->FindB("APT::Ingore-Hold",false) == false)
  Heh.. In my CVS version now :>

  + Your changes to cachefile I do not like. The locking mechanism needs
    to be abstracted properly into '_distro'.
  + (init.cc) Yuk, put an Init method in _distro and call the right one. 
    the Init function should create the proper _distro for the system the
    library was compiled for, overridable with a configuration setting.
  + pkgcachegen.cc has become badly misindented..
  + sourcelist sux in many horrible aweful ways, it needs to be redone
    somehow.. if you have some brainstorm, share :>
  + distrofactory is mostly OK, see comment below

**

General comments..

'distro' is not a name I like. I would prefer a name like environment, or
system, or something like that.

putting dep checking and version checking in 'distro' is something I
*hate*. I think it is much more appropriate to create a new field in the
cache header that indicates the version string style used within that
cache and then provide multiple version comparision functions that are
invoked based on that number. If you create a tidy patch to do this I will
apply it immediately to CVS APT.

Generally, most of your patches to apt are correct. The only reason you
don't already see something like a '_distro' is because I did not know
what would be needed to abstract :>

Basically, if you make me a nice patch that does the _distro thing with a
nicer name and makes deb use it then I will apply it and maintain it for
you. If you want, I would even build something based on what you have now,
but you'd have to wait a good while :|

The stuff in the RPM directory.. I am willing to consider including it -
however I will not do so until I can personally test it in detail. [I
haven't even looked at it yet] If you guys have an english version of your
distribution available on the internet that should not be a problem at
all.

Please base your work off CVS apt, the 'aliencode' branch which will
become APT 0.5.0 eventually, that is where I will apply your patches.

CVS is at :pserver:anonymous@cvs.debian.org:/cvs/deity module 'apt'

I'm terribly excited, your rpm directory is much smaller than I had
anticipated an RPM verison would be - and I'm surprised it is workable at
all, though I see you haven't fully delt with the filedepends issue..

I would appreciate a little file describing the key differences between
RPM and DEB from this perspective.

Thanks,
Jason