1 |
On Tue, 2013-05-28 at 00:34 -0400, Rick "Zero_Chaos" Farina wrote: |
2 |
> It has come to light that this patch breaks binpkg use in stage1. |
3 |
... |
4 |
> Thanks, |
5 |
> Zero |
6 |
> |
7 |
> On 03/26/2013 11:09 PM, Jorge Manuel B. S. Vicetto (jmbsvicetto) wrote: |
8 |
> > From: "Jorge Manuel B. S. Vicetto (jmbsvicetto)" <jmbsvicetto@g.o> |
9 |
> > |
10 |
> > + elif [ -n "${clst_PKGCACHE}" -a -z "${clst_update_seed}" ] |
11 |
|
12 |
As it turns out, this one change was incorrect to evaluate all possible |
13 |
scenarios. My bash skills are poor, I originally had used nested if's , |
14 |
but that got optimized by others to the above line. |
15 |
|
16 |
That line should be changed to: |
17 |
|
18 |
elif [ -n "${clst_PKGCACHE}" ] && ( [ -z "${clst_update_seed}" ] || [ "${clst_update_seed}" == "no" ] |
19 |
|
20 |
Which breaks down to (for those that can't follow it's logic): |
21 |
|
22 |
# NOTE: clst_update_seed is currently only used for a stage1, so does not exist in the environment for other targets. |
23 |
# spacing was added to the line so it lines up with the plain English text. |
24 |
|
25 |
# if clst_PKGCACHE is not null and either clst_update_seed doesn't exist or clst_update_seed is exactly equal to "no" |
26 |
#if [ -n "${clst_PKGCACHE}" ] && ( [ -z "${clst_update_seed}" ] || [ "${clst_update_seed}" == "no" ] ) |
27 |
|
28 |
|
29 |
I have 3 reasons for wanting this change done this way |
30 |
|
31 |
1. If a spec or config enables the PKGCACHE option. It will add |
32 |
all the binpkg options to the emerge command. Due to the |
33 |
possible problem with using/creating binpkgs during the |
34 |
update_seed process. These options should not be set. Currently |
35 |
they are being set, but only --binpkg was being reset to =n in |
36 |
the stage1-chroot.sh update_seed command. The other binpkg |
37 |
options were not being removed or reset. This is poor coding. |
38 |
It is far better to just not add the unwanted options in the |
39 |
first place. |
40 |
2. While it is possible to work around not having the logic in |
41 |
setup_myemergeoptions(), it is hacky and complicates the |
42 |
stage1-chroot code more than neccessary. It is also poor |
43 |
programming style. In total it would mean the |
44 |
setup_myemergeoptions() would be called three times in total for |
45 |
the stage1 run. Or temporarily saving the emerge options, |
46 |
unsetting clst_PKGCACHE, re-running setup_myemergeoptions() to |
47 |
get the correct options for update_seed, then later restoring |
48 |
the the two variables correctly. Both these coding options are |
49 |
poor programming style and more prone to bugs. With the |
50 |
proposed change above it would be run twice. Once keeping the |
51 |
PKGCACHE options off for the update_seed run, then reset |
52 |
according to the spec for the target emerge run. |
53 |
3. Placing the clst_update_seed logic to toggle the PKGCACHE |
54 |
options in the setup_myemergeoptions() simplifies the stage1 |
55 |
code. Plus it leaves the option to run an update_seed |
56 |
option/command for any future or current target without |
57 |
complicating/duplicating code. |
58 |
|
59 |
I have attached a small test program I used to come up with the |
60 |
correctly working patch change above. It can be used to evaluate how |
61 |
the line's logic is processed. I have also attached the terminal output |
62 |
it produced for all the possible conditions that could occur. |
63 |
|
64 |
Also I have slightly different PKGCACHE options in my rewrite branch. I |
65 |
have added --binpkg-respect-use to them. It was brought to my attention |
66 |
early in testing to put them in a config to eliminate the problem of |
67 |
using binpkgs that were not compiled with them set correctly. |
68 |
|
69 |
I will be pushing an updated rewrite branch with this and the other |
70 |
changes made to master soon. So "git pull --force" to update your |
71 |
checkout with the rewritten commits when I do. |
72 |
-- |
73 |
Brian Dolbec <dolsen@g.o> |