1 |
On Tue, 2013-12-17 at 19:28 -0800, W. Trevor King wrote: |
2 |
> On Tue, Dec 17, 2013 at 05:07:27PM -0800, Brian Dolbec wrote: |
3 |
> > +target_mounts = { |
4 |
> > + "proc": "/proc", |
5 |
> > + … |
6 |
> > + "port_logdir": "/var/log/portage", |
7 |
> > + } |
8 |
> > … |
9 |
> > @@ -173,11 +188,12 @@ class generic_stage_target(generic_target): |
10 |
> > file_locate(self.settings,["portage_confdir"],expand=0) |
11 |
> > |
12 |
> > """ Setup our mount points """ |
13 |
> > + self.target_mounts = target_mounts.copy() |
14 |
> > if "SNAPCACHE" in self.settings: |
15 |
> > self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"] |
16 |
> > self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts", |
17 |
> > - "portdir":self.settings["snapshot_cache_path"]+"/portage",\ |
18 |
> > - "distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"} |
19 |
> > + "portdir":normpath(self.settings["snapshot_cache_path"]+"/" + self.settings["repo_name"]), |
20 |
> > + "distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"} |
21 |
> |
22 |
> Can we do this for the mountmap defaults too, and just override the |
23 |
> mountmap special cases in the SNAPCACHE branch? I think this would be |
24 |
> a good commit for that (or we can do it in a commit after this). |
25 |
> |
26 |
> I don't think the 'portage' → self.settings["repo_name"] replacement |
27 |
> should go in this commit. |
28 |
|
29 |
repo_name variable is already defined. This one got missed in the |
30 |
change. You want it in the next commit by itself? Can do. |
31 |
|
32 |
|
33 |
> It also looks like there is some leading |
34 |
> non-tab whitespace at the beginning of your new distdir line, which |
35 |
> was otherwise unchanged. |
36 |
|
37 |
|
38 |
Ah, yes, an errant space got in at the beginning of the line. I have |
39 |
rebased it away. I will repost the fixed patch when any other dust |
40 |
settles. |
41 |
|
42 |
|
43 |
> |
44 |
> > - self.mounts.append("/var/log/portage") |
45 |
> > - self.mountmap["/var/log/portage"]=self.settings["port_logdir"] |
46 |
> > - self.env["PORT_LOGDIR"]="/var/log/portage" |
47 |
> > + self.mounts.append("port_logdir") |
48 |
> > + self.mountmap["port_logdir"]=self.settings["port_logdir"] |
49 |
> > + self.env["PORT_LOGDIR"]=self.settings["port_logdir"] |
50 |
> |
51 |
> I don't know where we stand on 'x=y' vs 'x = y', but I'd prefer the |
52 |
> latter here. |
53 |
|
54 |
I do too, but I thought it was agreed to fix all that separately |
55 |
later :/ |
56 |
|
57 |
> I also think that the PORT_LOGDIR environment variable |
58 |
> should be: |
59 |
> |
60 |
> self.env["PORT_LOGDIR"] = self.target_mounts["port_logdir"] |
61 |
> |
62 |
|
63 |
meh, but I also prefer some uniformity. I prefer all-caps for |
64 |
constants. In this case, the end result later in a new defaults.py |
65 |
file... since it is internal, why mix case in keys. They are also |
66 |
pushed into the bash environment for the chroot scripts to use. So, |
67 |
again, uniformity can be your friend when making changes. Especially |
68 |
with a large complex app like catalyst. |
69 |
|
70 |
code snipit from defaults.py to come later. |
71 |
|
72 |
# please try to avoid using "-", "/", "." in key names |
73 |
# due to them not being compatible in the bash environment |
74 |
confdefaults={ |
75 |
"storedir": "/var/tmp/catalyst", |
76 |
"sharedir": "/usr/lib/catalyst", |
77 |
"shdir": "%(sharedir)s/targets", |
78 |
"PythonDir": "./catalyst", |
79 |
"archdir": "%(PythonDir)s/arch", |
80 |
"distdir": "/usr/portage/distfiles", |
81 |
"repo_name": "portage", |
82 |
"portdir": "/usr/portage", |
83 |
"packagedir": "/usr/portage/packages", |
84 |
"port_tmpdir": "/var/tmp/portage", |
85 |
"local_overlay": "/usr/local/portage", |
86 |
"port_conf": "/etc/portage", |
87 |
"make_conf": "%(port_conf)s/make.conf", |
88 |
"options": set(), |
89 |
"snapshot_name": "portage-", |
90 |
"snapshot_cache": "/var/tmp/catalyst/snapshot_cache", |
91 |
"hash_function": "crc32", |
92 |
} |
93 |
|
94 |
|
95 |
target_mounts = { |
96 |
"proc": "/proc", |
97 |
"dev": "/dev", |
98 |
"pts": "/dev/pts", |
99 |
"portdir": "/usr/portage", |
100 |
"distdir": "/usr/portage/distfiles", |
101 |
"packagedir": "/usr/portage/packages", |
102 |
"port_tmpdir": "/var/tmp/portage", |
103 |
"kerncache": "/tmp/kerncache", |
104 |
"ccache": "/var/tmp/ccache", |
105 |
"icecream": "/var/cache/icecream", |
106 |
"port_logdir": "/var/log/portage", |
107 |
} |
108 |
|
109 |
|
110 |
|
111 |
> Nothing in the chroot should care where the source comes from ;). I'd |
112 |
> considering renaming mountmap → source_mounts for clarity, using |
113 |
> existing settings to override source_mounts at initialization, and |
114 |
> using source_mounts thereafter. |
115 |
|
116 |
mounts and mountmap were existing variable names. With some of the |
117 |
flack I've gotten over my choice of variable name already, now you want |
118 |
me to go through even more??? Do it in a separate commit. (after the |
119 |
rewrite patches are done for.) |
120 |
|
121 |
> |
122 |
> > def bind(self): |
123 |
> > for x in self.mounts: |
124 |
> > - if not os.path.exists(self.settings["chroot_path"] + self.mountmap[x]): |
125 |
> > - os.makedirs(self.settings["chroot_path"]+x,0755) |
126 |
> > + #print "bind(); x =", x |
127 |
> > + target = normpath(self.settings["chroot_path"] + self.target_mounts[x]) |
128 |
> > + if not os.path.exists(target): |
129 |
> > + os.makedirs(target, 0755) |
130 |
> > |
131 |
> > if not os.path.exists(self.mountmap[x]): |
132 |
> > if not self.mountmap[x] == "tmpfs": |
133 |
> > - os.makedirs(self.mountmap[x],0755) |
134 |
> > + os.makedirs(self.mountmap[x], 0755) |
135 |
> > |
136 |
> > src=self.mountmap[x] |
137 |
> |
138 |
> Might as well shift this src declaration up and use it instead of |
139 |
> self.mountmap[x] earlier. Not exactly required for this commit, but |
140 |
> you are defining target, so it's not too much of a stretch ;). |
141 |
|
142 |
Yeah, I did target when trying to get all the path issues straightened |
143 |
out. I never really looked at src to be honest. That code was not |
144 |
causing me grief. So, yeah, do it later in a separate commit. |
145 |
|
146 |
> |
147 |
> > - if "SNAPCACHE" in self.settings and x == "/usr/portage": |
148 |
> > + #print "bind(); src =", src |
149 |
> > + if "SNAPCACHE" in self.settings and x == self.settings["portdir"]: |
150 |
> |
151 |
> Oops. x is the mountmap key, so I think this should be 'portdir'. |
152 |
|
153 |
Good catch, I missed this one when converting from paths as keys. |
154 |
|
155 |
> |
156 |
> > + #print "bind(); cmd =", cmd |
157 |
> |
158 |
> If it's not useful enough to print, I don't think we should commit it |
159 |
> ;). |
160 |
|
161 |
There are various of these added to the code. They are EXTREMELY |
162 |
helpful and even necessary to debug major code changes. Of which the |
163 |
mounts, mountmap changes were one of the toughest to debug. |
164 |
|
165 |
Both you and I have already stated that we want to convert to using |
166 |
python's logging. These are just a first step towards that goal. All |
167 |
that is needed is to search out the #print's and replace them with teh |
168 |
proper logging statement. |
169 |
|
170 |
> I'll be happy once we're using subprocess :p. |
171 |
> |
172 |
|
173 |
I have a number of improvements coming to the cmd(). but yeah, it'll |
174 |
need more. |
175 |
|
176 |
> A few typos, but I think this makes mount handling much more sane :). |
177 |
> |
178 |
> Cheers, |
179 |
> Trevor |
180 |
> |
181 |
|
182 |
-- |
183 |
Brian Dolbec <dolsen@g.o> |