New issue
Advanced search Search tips

Issue 878570 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Make osutils.ResolveSymlink more obvious

Project Member Reported by la...@chromium.org, Aug 28

Issue description

I don't see any benefit over os.path.realpath (plus os.path.relpath if you want its 'root' kwarg).
 
Cc: vapier@chromium.org
vapier@: Any theory on why this function exists?
it probably isn't useful if |root| isn't specified.  which means the mobmonitor/system/systeminfo.py code should be converted to os.path.realpath, and we can prob make it clearer by making the root non-optional.

otherwise, if root is specified, i'm not seeing how os.path helps us.  the example text in the func explains what this func is good for.  os.path.relpath will still resolve absolute paths outside of |root|, and relpath's |start| is purely for relative symlinks.
Oh its for resolving absolute chroot symlinks from outside the chroot. That docstring...does not convey that to me.
"""Resolve a symlink |file_name| relative to |root|.""" is obvious once you know ;)
Summary: Make osutils.ResolveSymlink more obvious (was: Remove osutils.ResolveSymlink)
Status: Assigned (was: Untriaged)
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/ad97af93ecfd2add10c7da60a3484691b9e2e74f

commit ad97af93ecfd2add10c7da60a3484691b9e2e74f
Author: Lann Martin <lannm@chromium.org>
Date: Thu Sep 06 04:04:32 2018

Rename osutils.ResolveSymlink to ResolveSymlinkInRoot

This function was confusing to me. Updated to make it more obvious that
this is for a special use case and made the `root` argument non-optional.

Replaced one usage of ResolveSymlink with os.path.realpath.

BUG= chromium:878570 
TEST=lib/osutils_unittest; mobmonitor/system/systeminfo_unittest

Change-Id: Iafd5fd44b12998a34c59f97918170c40ec46ee19
Reviewed-on: https://chromium-review.googlesource.com/1205631
Commit-Ready: Lann Martin <lannm@chromium.org>
Tested-by: Lann Martin <lannm@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/ad97af93ecfd2add10c7da60a3484691b9e2e74f/mobmonitor/system/systeminfo_unittest.py
[modify] https://crrev.com/ad97af93ecfd2add10c7da60a3484691b9e2e74f/cros/test/image_test.py
[modify] https://crrev.com/ad97af93ecfd2add10c7da60a3484691b9e2e74f/mobmonitor/system/systeminfo.py
[modify] https://crrev.com/ad97af93ecfd2add10c7da60a3484691b9e2e74f/lib/osutils_unittest.py
[modify] https://crrev.com/ad97af93ecfd2add10c7da60a3484691b9e2e74f/lib/osutils.py

Status: Fixed (was: Assigned)

Sign in to add a comment