1 |
On Wed, Mar 24, 2021 at 08:48:41AM +0100, Michał Górny wrote: |
2 |
> On Wed, 2021-03-24 at 00:20 -0500, William Hubbs wrote: |
3 |
> > On Tue, Mar 23, 2021 at 10:23:11AM +0100, Michał Górny wrote: |
4 |
> > > On Sun, 2021-03-21 at 12:39 -0500, William Hubbs wrote: |
5 |
> > > > All, |
6 |
> > > > |
7 |
> > > > the following is a script which will migrate a Gentoo system to the usr |
8 |
> > > > merge layout. This is similar to the unsymlink-lib tool used to migrate |
9 |
> > > > a system from the 17.0 to the 17.1 profiles. |
10 |
> > > > |
11 |
> > > > I'm attaching it here to get some comments before I package it, so |
12 |
> > > > please let me know if I have missed something. |
13 |
> > > |
14 |
> > > To be honest, I don't think critical system modifications should be done |
15 |
> > > in shell script, and especially not via a fringe non-standards complaint |
16 |
> > > shell implementation in busybox. Even if you can assume you make no |
17 |
> > > mistakes, shell is unreliable by design. For example, your script may |
18 |
> > > start behaving in unexpected ways if you run out of space. |
19 |
> > |
20 |
> > I went with busybox shell in this case because busybox is a static |
21 |
> > binary and I figured with everything being moved around on the system it |
22 |
> > would be a good choice. |
23 |
> > |
24 |
> > I could pretty easily go with straight sh/bash, I just was focusing on |
25 |
> > bb since the commands are built in. |
26 |
> > |
27 |
> > You are right about shell behaving in weird ways if you run out of |
28 |
> > space, but that's true for anything that happens to be running when a |
29 |
> > system runs out of space. |
30 |
> |
31 |
> This is not true. Most of the programming languages can behave sanely |
32 |
> in out-of-space conditions. Sure, I/O fails one way or another but if |
33 |
> it's just a matter of proper error handling. Shells on the other hand, |
34 |
> tend to use temporary files under the hood and can misbehave |
35 |
> in unexpected ways. |
36 |
|
37 |
Another reason I went with bb is there are no deps outside of bb and the |
38 |
script. Also bb is on every gentoo system. |
39 |
In short, I went for something that I knew would be everywhere. |
40 |
If I'm going to do the work, it isn't going to be in python because I'm |
41 |
not a fan. I can write some in python, but I don't consider my skills |
42 |
there to be the best, and the indentation requirements make it tedious |
43 |
for me to debug. |
44 |
|
45 |
> > That is why I put the rollback directory in /var by |
46 |
> > default figuring there is more space there than in /, especially on |
47 |
> > systems with separate /var. |
48 |
> |
49 |
> What really can help is reflinking on filesystems supporting that. |
50 |
|
51 |
What really can help is more info instead of being terse like this. |
52 |
Which filesystems support it? |
53 |
|
54 |
> > The run_command wrapper in the script causes an immediate exit when the |
55 |
> > command it runs fails so things don't go any |
56 |
> > further than the failing command, so I'm not sure what else I can do |
57 |
> > with this situation. One option is to always echo the command we are |
58 |
> > about to run then just not run it if -d is specified. That means you |
59 |
> > would always see the commands the script is running. |
60 |
> |
61 |
> Did you see the verbose explanatory messages unsymlink-lib produces |
62 |
> on every error condition? Compared to this, your script is printing |
63 |
> Windows-level 'error figure-it-out-yourself'. |
64 |
|
65 |
This is early in development. Ideally I would handle most of the error |
66 |
conditions before people start using this on production systems; that is |
67 |
why I put it out here. |
68 |
|
69 |
> > > You don't seem to be handling file collisions at all. Even today we |
70 |
> > > have files like /bin/bzip2 and /usr/bin/bzip2, not to mention shared |
71 |
> > > libraries. Silently ignoring the problem or requiring the users to |
72 |
> > > manually ensure their system is clean is not going to solve it. |
73 |
> |
74 |
> > I have added -i to the cp commands in my latest version of this so I'll |
75 |
> > see when we have duplicate names, and yes that is an issue, even without |
76 |
> > the /usr merge. |
77 |
> |
78 |
> I don't think that's the best UI for this possible. I doubt that |
79 |
> a random user running the script will figure out why it is asking about |
80 |
> file replacements. |
81 |
|
82 |
You are probably right, that's why I put this out here, and |
83 |
listed my thoughts about it below to generate discussion. |
84 |
|
85 |
> > I'm curious why we are duplicating all of these names in the first |
86 |
> > place honestly. |
87 |
> > |
88 |
> > One example of this is in coreutils, and we even say in the ebuild |
89 |
> > comment that we need to figure out why we are doing it. |
90 |
> > |
91 |
> > There are a couple of ways forward for this. |
92 |
> > |
93 |
> > 1) attempt to open bugs on the packages and remove the duplicate names |
94 |
> > by dropping symlinks and putting the files in their cannonical |
95 |
> > locations. this could lead to breakages if one of the alternate names is |
96 |
> > expected by something until the /usr merge is done. |
97 |
> > |
98 |
> > 2) try to guess at resolving the duplicate names as part of the |
99 |
> > usr merge process. |
100 |
> > |
101 |
> > a. symmlinks in /bin or /sbin that point to the same name in /usr/bin or |
102 |
> > /usr/sbin should be removed. |
103 |
> > b. symlinks in /usr/bin or /usr/sbin that point to the same name in / |
104 |
> > should be removed. |
105 |
> > c. Other symlinks that I can think of should be preserved. |
106 |
> > |
107 |
> > Which path for handling this is best? Do you have any thoughts? |
108 |
> |
109 |
> This is a thing that should have been researched before writing |
110 |
> a script. |
111 |
|
112 |
With all respect, knock it off. I'm not going to sit and read through |
113 |
all of the ebuilds in the tree looking for every little thing. |
114 |
|
115 |
> One case I'm aware of is using symlinks to replace tools with |
116 |
> alternatives, e.g. /usr/bin/bzip2 -> lbzip2 (/bin/bzip2 remains |
117 |
> as the original). |
118 |
|
119 |
Your example doesn't exist here, so I wouldn't have known about it if |
120 |
you hadn't mentioned it. |
121 |
|
122 |
It looks like that specific situation is controlled by the |
123 |
symlink use flag in lbzip, so it should be ok. |
124 |
|
125 |
> I don't know what's the best way forward. |
126 |
> |
127 |
> > |
128 |
> > > You don't seem to provide any helpful messages. When things fail, user |
129 |
> > > will be left in the blue with an error message from some system tool (or |
130 |
> > > rather, cheap-ass busybox rewrite, I guess). |
131 |
> > |
132 |
> > If the rollback directory is populated, you can use -r to recover, |
133 |
> > and the script will not let you perform the /usr merge if the rollback |
134 |
> > directory is not populated. |
135 |
> > |
136 |
> > > Also, have you verified that busybox's cp(1) actually preserves all file |
137 |
> > > properties (including xattrs, ACLs, caps...)? |
138 |
> > |
139 |
> > The documentation for busybox states that -p preserves file attributes |
140 |
> > if possible, and by doing ls -l in the chroot I can see that |
141 |
> > ownership/permissions are preserved. So, logically I would guess it |
142 |
> > preserves the others. |
143 |
> |
144 |
> I don't see how you would come to this conclusion. Just because some |
145 |
> tool is doing the absolute minimum it doesn't mean it does anything |
146 |
> else, and especially operations that require additional syscalls. |
147 |
|
148 |
Hmm, I think a better response here would have been to point out how I |
149 |
could look at the other attributes of a file you are talking about. |
150 |
|
151 |
> > If switching back to non-busybox is fine for this operation, I have no |
152 |
> > problem doing that either. |
153 |
> > |
154 |
> > > Please don't forget to include tests with it. Docker's good for testing |
155 |
> > > stuff like this. |
156 |
> > |
157 |
> > Docker can't be used during src_test that I'm aware of, so I'm not sure |
158 |
> > how Docker could be used to test this. |
159 |
> |
160 |
> You can test it outside of ebuild. Some automated testing is better |
161 |
> than no testing at all. |
162 |
|
163 |
With all respect, I think most of this email was sarcasm, trolling etc |
164 |
instead of the type of constructive feedback we look for when sharing |
165 |
code on the list. I will do what I can to use parts of it, but I really |
166 |
have issues with your tone. |
167 |
|
168 |
William |