On Thu, Jul 12, 2012 at 08:46:51AM -0500, David A. Marlin wrote:
> Martin Gracik wrote:
> >See the comments...
> >
> >
> >Signed-off-by: David A. Marlin <dmarlin@xxxxxxxxxx>
> >---
> >diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
> >index 8fe818d..b0d9adc 100644
> >--- a/pyanaconda/iutil.py
> >+++ b/pyanaconda/iutil.py
> >@@ -621,6 +621,42 @@ def getPPCMacBook():
> > return 1
> > return 0
> >+## Get the ARM processor variety.
> >+# @return The ARM processor variety type, or 0 if not ARM.
> >+def getARMMachine():
> >+ if not isARM():
> >+ return 0
> >+
> >+ armMachine = None
> >+ machine = None
> >+
> >+ # ARM machine hash
> >+ armType = {
> >+ 'OMAP' : 'omap',
> >+ 'trimslice' : 'tegra',
> >+ 'Marvell Armada XP' : 'mvebu',
> >+ 'Marvell GuruPlug' : 'kirkwood',
> >+ 'Efika MX' : 'imx',
> >+ 'Highbank' : 'highbank',
> >+ }
> >+
> >+ f = open('/proc/cpuinfo', 'r')
> >+ lines = f.readlines()
> >+ f.close()
> >+ for line in lines:
> >+ if line.find('Hardware') != -1:
> >+ machine = line.split(':')[1]
> >+ break
> >+
> >+ if machine is not None:
> >+ for type in armType.items():
> >+ if machine.find(type[0]) != -1:
> >+ armMachine = type[1]
> >+ break
> >
> >You could change this to:
> >
> >with open('/proc/cpuinfo', 'r') as fobj:
> > for line in fobj:
> > if 'Hardware' in line:
> > machine = line.split(':')[1]
> > # we got the machine here, so we can get the armMachine as well...
> > # shouldn't the value we got in machine be the same as the keys in the arm machine hash? is the find here necessary?
>
> The 'machine' from '/proc/cpuinfo' is not always an exact match for
> the keys in the arm machine hash, for example 'OMAP' matches 'OMAP3'
> and 'OMAP4'. I used 'find' in order to minimize the hash map size
> and provide the flexibility to support additional variants from a
> given vendor without modifying the code.
I still dislike having this hash in anaconda. Why can the information
provided by the kernel not match the kernel package? This is not knowledge
the installer should own. I understand you've declared this as temporary
and not the ideal solution, but something we can do now to make progress.
The problem with that is that there's no such thing as temporary here.
We'll carry the hash forever and then a year+ down the road, something won't
work with arm kernel installation and it'll be declared an anaconda problem
and we won't have any idea why. And it'll come back to this hash being out
of sync or wrong or whatever. In fact, the reporters will likely refer to
it as a "regression" or something "anaconda does that just doesn't make
sense". So I want to stop that from ever happening in the first place.
The kernel packages need to match the architecture names provided by the
kernel.
There's no reason the ARM kernel packages can't carry dozens of Provides to
account for the name variations in the hardware so that the code in anaconda
can be generic and work for all cases. The problem of package name to
name-from-cpuinfo is then correctly owned by the kernel (though the RPM
rather than in the kernel itself, but I'm ok with that).
The other major problem with embedding this hash in the installer is that
any time it changes because the kernel changed, anaconda will have to be
rebuilt.
We should be able to take the value after Hardware: and then do something
like .replace(' ', '_') and have that be sufficient to select the kernel
package. i.e., The kernel-kirkwood package should have a Provides for
kernel-Marvell_GuruPlug.
>
> d.marlin
> ========
>
> > armMachine = armType.get(machine, None)
> > break
> >
> >I think it is clearer this way. I don't like the find() method. :)
> >
> >+
> >+ return armMachine
> >+
> >+
> > cell = None
> > ## Determine if the hardware is the Cell platform.
> > # @return True if so, False otherwise.
> >diff --git a/pyanaconda/platform.py b/pyanaconda/platform.py
> >index 8870374..1611797 100644
> >--- a/pyanaconda/platform.py
> >+++ b/pyanaconda/platform.py
> >@@ -335,6 +335,7 @@ class Sparc(Platform):
> > return start+1
> > class ARM(Platform):
> >+ _armMachine = iutil.getARMMachine()
> > _bootloaderClass = bootloader.GRUB2
> > _boot_stage1_device_types = ["disk"]
> > _boot_mbr_description = N_("Master Boot Record")
> >@@ -343,6 +344,10 @@ class ARM(Platform):
> > _disklabel_types = ["msdos"]
> >+ @property
> >+ def armMachine(self):
> >+ return self._armMachine
> >+
> > def getPlatform(anaconda):
> > """Check the architecture of the system and return an instance of a
> > Platform subclass to match. If the architecture could not be determined,
> >diff --git a/pyanaconda/yuminstall.py b/pyanaconda/yuminstall.py
> >index 0ca534b..63d077d 100644
> >--- a/pyanaconda/yuminstall.py
> >+++ b/pyanaconda/yuminstall.py
> >@@ -1465,6 +1465,11 @@ reposdir=/etc/anaconda.repos.d,/tmp/updates/anaconda.repos.d,/tmp/product/anacon
> > if selectKernel("kernel-PAE"):
> > foundkernel = True
> >+ if not foundkernel and iutil.isARM():
> >+ if anaconda.platform.armMachine is not None:
> >+ selectKernel("kernel-" + anaconda.platform.armMachine)
> >+ foundkernel = True
> >+
> > if not foundkernel:
> > selectKernel("kernel")
> >
> >----- Original Message -----
> >>Attached is my latest version of this patch. It was made against the
> >>current anaconda/master and applied to 17.29 for testing on F17.
> >>
> >>d.marlin
> >>========
> >>
> >>David A. Marlin wrote:
> >>>Attached is my latest version of the ARM processor variety patch.
> >>> It
> >>>incorporates suggestions made previously, and was made against
> >>>anaconda/master (but applied to 17.29 for testing).
> >>>
> >>>I simplified the hash/lookup by consolidating some machine types,
> >>>but
> >>>I understand this may still be unacceptable. I am currently
> >>>tracking
> >>>master and carrying these patches locally. I keep the rpms in a
> >>>local
> >>>repo for testing.
> >>>
> >>>We are working on a method of exposing the platform in userspace,
> >>>but
> >>>that is not in place yet. Would it be possible to accept this as
> >>>an
> >>>interim solution with the expressed intent that the hash would be
> >>>removed as soon as an alternative solution is available? If not,
> >>>would a version of this patch with the hash/lookup stubbed out be
> >>>acceptable? I would like to get as much of this upstream as
> >>>possible,
> >>>to minimize the patch(es) I need to carry locally and simplify
> >>>keeping
> >>>up with master. Any suggestions are appreciated.
> >>>
> >>>Note: We successfully completed a test kickstart install on an ARM
> >>>system today using the modified anaconda-17.29.
> >>>
> >>>
> >>>d.marlin
> >>>
> >>>------------------------------------------------------------------------
> >>>
> >>>_______________________________________________
> >>>Anaconda-devel-list mailing list
> >>>Anaconda-devel-list@xxxxxxxxxx
> >>>https://www.redhat.com/mailman/listinfo/anaconda-devel-list
> >>
> >>[Text
> >>Documents:Add-support-to-determine-the-ARM-processor-variety.patch]
> >>
> >>_______________________________________________
> >>Anaconda-devel-list mailing list
> >>Anaconda-devel-list@xxxxxxxxxx
> >>https://www.redhat.com/mailman/listinfo/anaconda-devel-list
> >
> >------------------------------------------------------------------------
> >
> >_______________________________________________
> >Anaconda-devel-list mailing list
> >Anaconda-devel-list@xxxxxxxxxx
> >https://www.redhat.com/mailman/listinfo/anaconda-devel-list
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
--
David Cantrell <dcantrell@xxxxxxxxxx>
Supervisor, Installer Engineering Team
Red Hat, Inc. | Westford, MA | EST5EDT
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
