From: Ales Kozumplik ales@redhat.com
So we get the history on the target. --- pyanaconda/packaging/dnfpayload.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py index b019723..7ee3344 100644 --- a/pyanaconda/packaging/dnfpayload.py +++ b/pyanaconda/packaging/dnfpayload.py @@ -135,7 +135,6 @@ class DNFPayload(packaging.PackagePayload): def _configure(self): self._base = dnf.Base() conf = self._base.conf - conf.persistdir = DNF_CACHE_DIR self._base.cache_c.prefix = DNF_CACHE_DIR self._base.cache_c.suffix = 'default' conf.logdir = '/tmp/payload-logs' @@ -144,8 +143,9 @@ class DNFPayload(packaging.PackagePayload): conf.errorlevel = 0 self._base.logging.setup_from_dnf_conf(conf)
- conf.installroot = constants.ROOT_PATH conf.releasever = self._getReleaseVersion(None) + conf.installroot = constants.ROOT_PATH + conf.prepend_installroot('persistdir')
# NSS won't survive the forking we do to shield out chroot during # transaction, disable it in RPM:
From: Ales Kozumplik ales@redhat.com
It's not in any comps yet but testers of the payload will expect it to be present. --- pyanaconda/packaging/dnfpayload.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py index 7ee3344..4af95de 100644 --- a/pyanaconda/packaging/dnfpayload.py +++ b/pyanaconda/packaging/dnfpayload.py @@ -124,6 +124,7 @@ class DNFPayload(packaging.PackagePayload): map(self._install_package, self._required_pkgs) map(self._select_group, self._required_groups) self._select_kernel_package() + self._install_package('dnf')
def _bump_tx_id(self): if self.txID is None:
On Mon, 2013-09-30 at 16:45 +0200, akozumpl@redhat.com wrote:
From: Ales Kozumplik ales@redhat.com
It's not in any comps yet but testers of the payload will expect it to be present.
pyanaconda/packaging/dnfpayload.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py index 7ee3344..4af95de 100644 --- a/pyanaconda/packaging/dnfpayload.py +++ b/pyanaconda/packaging/dnfpayload.py @@ -124,6 +124,7 @@ class DNFPayload(packaging.PackagePayload): map(self._install_package, self._required_pkgs) map(self._select_group, self._required_groups) self._select_kernel_package()
self._install_package('dnf')
These both look good to me.
On Mon, Sep 30, 2013 at 04:45:04PM +0200, akozumpl@redhat.com wrote:
From: Ales Kozumplik ales@redhat.com
It's not in any comps yet but testers of the payload will expect it to be present.
pyanaconda/packaging/dnfpayload.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py index 7ee3344..4af95de 100644 --- a/pyanaconda/packaging/dnfpayload.py +++ b/pyanaconda/packaging/dnfpayload.py @@ -124,6 +124,7 @@ class DNFPayload(packaging.PackagePayload): map(self._install_package, self._required_pkgs) map(self._select_group, self._required_groups) self._select_kernel_package()
self._install_package('dnf')
Does the order matter? If not, I'd rather see this added to self._required_pkgs at some point, either in preInstall or maybe as a default and then change preInstall to append to the list.
I also notice that some of the variable naming is different from yumpayload.py
For example, dnf is using:
self._required_groups = [] self._required_pkgs = []
and yumpayload uses:
self._requiredPackages = [] self._requiredGroups = []
It would be nice if we could use the same names where the functionality is the same between the packaging modules.
On Fri, 2013-10-04 at 10:14 -0700, Brian C. Lane wrote:
On Mon, Sep 30, 2013 at 04:45:04PM +0200, akozumpl@redhat.com wrote:
From: Ales Kozumplik ales@redhat.com
It's not in any comps yet but testers of the payload will expect it to be present.
pyanaconda/packaging/dnfpayload.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py index 7ee3344..4af95de 100644 --- a/pyanaconda/packaging/dnfpayload.py +++ b/pyanaconda/packaging/dnfpayload.py @@ -124,6 +124,7 @@ class DNFPayload(packaging.PackagePayload): map(self._install_package, self._required_pkgs) map(self._select_group, self._required_groups) self._select_kernel_package()
self._install_package('dnf')Does the order matter? If not, I'd rather see this added to self._required_pkgs at some point, either in preInstall or maybe as a default and then change preInstall to append to the list.
I also notice that some of the variable naming is different from yumpayload.py
For example, dnf is using:
self._required_groups = [] self._required_pkgs = []
and yumpayload uses:
self._requiredPackages = [] self._requiredGroups = []
It would be nice if we could use the same names where the functionality is the same between the packaging modules.
I'd rather see names following python variable-naming conventions in new code. Otherwise we would end up with ugly variable names for no reason once we drop yumpayload.
On Mon, 2013-10-07 at 10:11 +0200, Vratislav Podzimek wrote:
On Fri, 2013-10-04 at 10:14 -0700, Brian C. Lane wrote:
On Mon, Sep 30, 2013 at 04:45:04PM +0200, akozumpl@redhat.com wrote:
From: Ales Kozumplik ales@redhat.com
It's not in any comps yet but testers of the payload will expect it to be present.
pyanaconda/packaging/dnfpayload.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py index 7ee3344..4af95de 100644 --- a/pyanaconda/packaging/dnfpayload.py +++ b/pyanaconda/packaging/dnfpayload.py @@ -124,6 +124,7 @@ class DNFPayload(packaging.PackagePayload): map(self._install_package, self._required_pkgs) map(self._select_group, self._required_groups) self._select_kernel_package()
self._install_package('dnf')Does the order matter? If not, I'd rather see this added to self._required_pkgs at some point, either in preInstall or maybe as a default and then change preInstall to append to the list.
I also notice that some of the variable naming is different from yumpayload.py
For example, dnf is using:
self._required_groups = [] self._required_pkgs = []
and yumpayload uses:
self._requiredPackages = [] self._requiredGroups = []
It would be nice if we could use the same names where the functionality is the same between the packaging modules.
I'd rather see names following python variable-naming conventions in new code. Otherwise we would end up with ugly variable names for no reason once we drop yumpayload.
+1
On 10/04/2013 07:14 PM, Brian C. Lane wrote:
Does the order matter? If not, I'd rather see this added to self._required_pkgs at some point, either in preInstall or maybe as a default and then change preInstall to append to the list.
self._required_pkgs only exists to store the packages passed to preInstall() until install() is called. It is a minor flaw of the Payload interface itself that these two methods are not one since they are always called in succession (see install.py:161), but the DNFPayload has to respect that and the most straightforward way by far to pass the list on to nstall() is to store it in self._required_pkgs. That's the only reason why this instance variable exists and it might go away in the future. In no way would appending to it or even merging it with a new special instance variable, say _internally_required_pkgs result in a cleaner design and better maintainability than the single call to _install_package('dnf'), which is what I stress the most right after functionality.
I also notice that some of the variable naming is different from yumpayload.py
For example, dnf is using:
self._required_groups = [] self._required_pkgs = []
In Python, using underscore at the instance member start is used to indicate internal use in the instance. It does not really matter to the outisde world (like yumpayload.py) how such variable is called, it is part of the implementation details. Following the same name of a similar variable of different interface implementation can only be useful if the interface guaranteed its existence, which is hard to see for a private variable, or alternatively to breach the implementation encapsulation by the interface clients, which is definitely not desirable in this case, if ever.
Second, it can not be denied Anaconda is using both camelCase and underscore_convention in the code. Even the YumPayload itself is guilty of this:
self._repos_dir = "/etc/yum.repos.d,/etc/anaconda.repos.d,/tmp/updates/anaconda.repos.d,/tmp/product/anaconda.repos.d" ... self._requiredPackages = []
If Anaconda settled for one I would have no problem to follow the convention. Until then I decided to use underscores as I perceive it as more common in the current Python ecosystem.
Third, YumPayload will eventually get obsoleted and removed, so it makes little sense to be making the new module adhere to the old one in any way except the public interface.
It would be nice if we could use the same names where the functionality is the same between the packaging modules.
I perceive your comments to this trivial patch as somewhat bully-ish. Notice also that because of the time gap between us I will not be able to push this at least the following 24 hours. This is not really helping as the process of developing for Anaconda for (now) an outsider is already quite difficult as it is.
Ales
On Mon, Oct 07, 2013 at 10:38:50AM +0200, Ales Kozumplik wrote:
It would be nice if we could use the same names where the functionality is the same between the packaging modules.
I perceive your comments to this trivial patch as somewhat bully-ish. Notice also that because of the time gap between us I will not be able to push this at least the following 24 hours. This is not really helping as the process of developing for Anaconda for (now) an outsider is already quite difficult as it is.
Whoa! Slow down there. I in no way meant those comments to be 'bully-ish'. I was simply pointing out some things that I thought might improve understanding the code.
My comment about the dnf package was because I thought it would be good to keep things like that in one place, not spread through different methods in the module. We already have a mechanism for adding packages, why not take advantage of that.
As for naming conventions, I agree that underscores are more correct, and that private variables are private to a module -- but my motivation there was for readers of the code looking at what we currently have, not that either one of them is wrong. Anaconda has lots of inconsistent code, as you know. dnf is new, yum is the existing working module. When I'm looking at the two and both are doing similar things it is easier to figure out what's going on when things are named the same.
anaconda-patches@lists.fedorahosted.org
-
Ales Kozumplik -
Brian C. Lane -
Martin Kolman -
Vratislav Podzimek