Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13532003:
Installing Chrome OS fonts fails when repo is on filer (Closed)
|
Created:
7 years, 8 months ago by sschmitz Modified:
7 years, 8 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionInstalling Chrome OS fonts fails when repo is on filer
install-build-deps.sh runs "sudo /path/to/src/build/linux/install-chromeos-fonts.py".
If the repo is on a filer, sudo fails because root is not allowed to access files on
a filer. However, installing a Repo on a Filer is not recommended.
Solution:
install-build-deps.sh detects failure of "sudo install-chromeos-fonts.sh" and prints an
error message and instructions.
BUG=225543
R=derat@chromium.org
TEST=manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192485
Patch Set 1 #
Total comments: 2
Patch Set 2 : update #Patch Set 3 : update #
Total comments: 4
Patch Set 4 : reversed logic #Patch Set 5 : testing for failure #Messages
Total messages: 22 (0 generated)
+jorgelo because copying scripts to /tmp and then running them as root makes me awfully nervous. I'd rather we just said that keeping your repo on a filer is unsupported (isn't that already the case?). https://codereview.chromium.org/13532003/diff/1/build/install-build-deps.sh File build/install-build-deps.sh (right): https://codereview.chromium.org/13532003/diff/1/build/install-build-deps.sh#n... build/install-build-deps.sh:214: sudo chmod 0555 /tmp/install-chromeos-fonts.py why do you need to use sudo when chmod-ing and rm-ing this file? also, just do chmod +x /tmp/... instead of setting 0555.
Good points. Thanks. https://codereview.chromium.org/13532003/diff/1/build/install-build-deps.sh File build/install-build-deps.sh (right): https://codereview.chromium.org/13532003/diff/1/build/install-build-deps.sh#n... build/install-build-deps.sh:214: sudo chmod 0555 /tmp/install-chromeos-fonts.py On 2013/04/03 14:38:34, Daniel Erat wrote: > why do you need to use sudo when chmod-ing and rm-ing this file? > > also, just do chmod +x /tmp/... instead of setting 0555. Good point. Removed. And using chmod a+x instead.
On 2013/04/03 15:10:45, sschmitz wrote: > Good points. Thanks. > > https://codereview.chromium.org/13532003/diff/1/build/install-build-deps.sh > File build/install-build-deps.sh (right): > > https://codereview.chromium.org/13532003/diff/1/build/install-build-deps.sh#n... > build/install-build-deps.sh:214: sudo chmod 0555 /tmp/install-chromeos-fonts.py > On 2013/04/03 14:38:34, Daniel Erat wrote: > > why do you need to use sudo when chmod-ing and rm-ing this file? > > > > also, just do chmod +x /tmp/... instead of setting 0555. > > Good point. Removed. And using chmod a+x instead. Keeping the repo on Filer is insane, performance is terrible. Why do we care about this case?
+tburkard Timo, Could you please provide some info on the importance of keeping the Repo on Filer? I made a CL for Issue 225543, but there are security concerns. https://codereview.chromium.org/13532003/ Thanks, Stefan On Wed, Apr 3, 2013 at 8:25 AM, <jorgelo@chromium.org> wrote: > On 2013/04/03 15:10:45, sschmitz wrote: > >> Good points. Thanks. >> > > https://codereview.chromium.**org/13532003/diff/1/build/** >> install-build-deps.sh<https://codereview.chromium.org/13532003/diff/1/build/install-build-deps.sh> >> File build/install-build-deps.sh (right): >> > > > https://codereview.chromium.**org/13532003/diff/1/build/** > install-build-deps.sh#**newcode214<https://codereview.chromium.org/13532003/diff/1/build/install-build-deps.sh#newcode214> > >> build/install-build-deps.sh:**214: sudo chmod 0555 >> > /tmp/install-chromeos-fonts.py > >> On 2013/04/03 14:38:34, Daniel Erat wrote: >> > why do you need to use sudo when chmod-ing and rm-ing this file? >> > >> > also, just do chmod +x /tmp/... instead of setting 0555. >> > > Good point. Removed. And using chmod a+x instead. >> > > Keeping the repo on Filer is insane, performance is terrible. Why do we > care > about this case? > > https://codereview.chromium.**org/13532003/<https://codereview.chromium.org/1... >
Now that I know a workaround, it is not critical for me to get this fixed, so if people prefer to keep it the way it is, that's fine by me.
https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.sh File build/install-build-deps.sh (right): https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.... build/install-build-deps.sh:213: echo "Warning: Your Repo appears to be on a Filer and not on a local file" nit: no caps: "repo" https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.... build/install-build-deps.sh:213: echo "Warning: Your Repo appears to be on a Filer and not on a local file" "Filer" -> "remote file system" not everybody compiling Chromium works at Google.
On 2013/04/04 02:33:44, Jorge Lucangeli Obes wrote: > https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.sh > File build/install-build-deps.sh (right): > > https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.... > build/install-build-deps.sh:213: echo "Warning: Your Repo appears to be on a > Filer and not on a local file" > nit: no caps: "repo" > > https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.... > build/install-build-deps.sh:213: echo "Warning: Your Repo appears to be on a > Filer and not on a local file" > "Filer" -> "remote file system" not everybody compiling Chromium works at > Google. You might consider checking for NFS and failing, rather than checking for ext* and passing. I believe there are people out there who use local file systems other than ext2/ext3 (like zfs).
https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.sh File build/install-build-deps.sh (right): https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.... build/install-build-deps.sh:213: echo "Warning: Your Repo appears to be on a Filer and not on a local file" On 2013/04/04 02:33:44, Jorge Lucangeli Obes wrote: > nit: no caps: "repo" Done. https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.... build/install-build-deps.sh:213: echo "Warning: Your Repo appears to be on a Filer and not on a local file" On 2013/04/04 02:33:44, Jorge Lucangeli Obes wrote: > nit: no caps: "repo" Done.
On 2013/04/04 14:51:31, sschmitz wrote: > https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.sh > File build/install-build-deps.sh (right): > > https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.... > build/install-build-deps.sh:213: echo "Warning: Your Repo appears to be on a > Filer and not on a local file" > On 2013/04/04 02:33:44, Jorge Lucangeli Obes wrote: > > nit: no caps: "repo" > > Done. > > https://codereview.chromium.org/13532003/diff/13001/build/install-build-deps.... > build/install-build-deps.sh:213: echo "Warning: Your Repo appears to be on a > Filer and not on a local file" > On 2013/04/04 02:33:44, Jorge Lucangeli Obes wrote: > > nit: no caps: "repo" > > Done. lgtm
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
LGTM This might have some false positives, as I assume that not all FUSE filesystems prevent root access. Another option would be to try to stat the file as root and print the warning and/or skip the copy if it fails.
Because of "bash -e" any failure will exit the script immediately. So I cannot try and print instructions on failure :( On Thu, Apr 4, 2013 at 11:14 AM, <derat@chromium.org> wrote: > LGTM > > This might have some false positives, as I assume that not all FUSE > filesystems > prevent root access. Another option would be to try to stat the file as > root > and print the warning and/or skip the copy if it fails. > > https://codereview.chromium.**org/13532003/<https://codereview.chromium.org/1... >
I'm not a shell wizard, but I believe that only unhandled failures will exit. Local testing and the man page confirm this: "The shell does not exit if the command that fails is part of the command list immediately following a while or until keyword, part of the test following the if or elif reserved words, part of any command executed in a && or || list except the command following the final && or ||, any command in a pipeline but the last, or if the command's return value is being inverted with !." On 2013/04/04 18:16:23, sschmitz wrote: > Because of "bash -e" any failure will exit the script immediately. So I > cannot try and print instructions on failure :( > > > On Thu, Apr 4, 2013 at 11:14 AM, <mailto:derat@chromium.org> wrote: > > > LGTM > > > > This might have some false positives, as I assume that not all FUSE > > filesystems > > prevent root access. Another option would be to try to stat the file as > > root > > and print the warning and/or skip the copy if it fails. > > > > > https://codereview.chromium.**org/13532003/%3Chttps://codereview.chromium.org...> > >
thanks On Thu, Apr 4, 2013 at 11:28 AM, <derat@chromium.org> wrote: > I'm not a shell wizard, but I believe that only unhandled failures will > exit. > Local testing and the man page confirm this: > > "The shell does not exit if the command that fails is part of the > command > list immediately following a while or until keyword, part of the > test > following the if or elif reserved words, part of any command executed in > a && > or || list except the command following the final && or ||, any > command > in a pipeline but the last, or if the command's return value is being > inverted with !." > > > On 2013/04/04 18:16:23, sschmitz wrote: > >> Because of "bash -e" any failure will exit the script immediately. So I >> cannot try and print instructions on failure :( >> > > > On Thu, Apr 4, 2013 at 11:14 AM, <mailto:derat@chromium.org> wrote: >> > > > LGTM >> > >> > This might have some false positives, as I assume that not all FUSE >> > filesystems >> > prevent root access. Another option would be to try to stat the file as >> > root >> > and print the warning and/or skip the copy if it fails. >> > >> > >> > > https://codereview.chromium.****org/13532003/%3Chttps://codere** > view.chromium.org/13532003/ <http://codereview.chromium.org/13532003/>> > >> > >> > > > https://codereview.chromium.**org/13532003/<https://codereview.chromium.org/1... >
Thanks, Dan... now testing for failure and exiting with an error message and instructions.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13532003/24003
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/13532003/24003
Message was sent while issue was closed.
Change committed as 192485 |
