Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a tool to fix_symlinks: remove broken ones and relativize all of them #7178

Merged
merged 8 commits into from Jun 29, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Jun 10, 2020

Changelog: Feature: Adds tool to fix symlinks in the package_folder.
Docs: conan-io/docs#1751

Adds a tool tools.fix_symlinks(conanfile, raise_if_error=False) that works in the conanfile.package_folder to fix symlinks:

  • Converts every symlink into a relative one starting in the root of the package.
  • Removes (or raises) symlinks that point to files/directories outside the package.
  • Removes (or raises) broken symlinks.

Related to #7093

@jgsogo jgsogo self-assigned this Jun 10, 2020
@jgsogo jgsogo requested a review from uilianries Jun 11, 2020
@jgsogo jgsogo marked this pull request as ready for review Jun 11, 2020
@jgsogo jgsogo requested a review from memsharded Jun 11, 2020
" been removed.".format(item=offending_file, token=token))
os.unlink(fullpath)
elif link_target != link_rel_target:
os.unlink(fullpath)
Copy link
Member

@uilianries uilianries Jun 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about adding a info message, or even a log info? After running package, Conan reports how many files were copied, but we don't know how many are symbolic links. This method could empathize the symlink update.

Copy link
Member Author

@jgsogo jgsogo Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO that output is not needed: you are writing the recipe, you are creating the package, you know you are calling this tool and the message here will tell you only about updated symlinks (removed ones are already reported), IMO it would be more like a debug-level output (and we don't have that level in the output).

Copy link
Contributor

@SSE4 SSE4 Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to add an output, it doesn't belong here. conan might be running with different output formatters (e.g. human-readable, json, HTML, etc.). this function probably should just reference helpers from conan_command_output. otherwise we may run into weird situation receiving human-readable messages running commands on part of CI scripts via --json (or --html)

@jgsogo jgsogo added this to the 1.27 milestone Jun 12, 2020
fullpath = os.path.join(dirpath, element)
if os.path.islink(fullpath):
link_target = os.readlink(fullpath)
link_abs_target = os.path.join(dirpath, link_target)
Copy link
Contributor

@SSE4 SSE4 Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct thing to do? e.g.:

>>> os.path.join("C:\\Windows", "/dev/null")
'C:/dev/null'

returns broken result in case readlink returns /dev/null?

Copy link
Member Author

@jgsogo jgsogo Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Something to take into account: probably those absolute links to /dev/null (Unix) and ...nul is Windows is something we can identify and preserve the symlink. Do you know about any other special file paths?

Also, for Windows, I'm missing one if clause: if os.path.isabs(link_rel_target)

Copy link
Member Author

@jgsogo jgsogo Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this issue. I feel like we need to detect the special Unix /dev/null file, but for Windows I assume you will name the file NUL, LPT1, COM1,... instead of creating a symlink.

Copy link
Contributor

@SSE4 SSE4 Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are number of such special files or pseudo-files, e.g. /dev/zero, /dev/urandom, /dev/full, etc. same on Windows with LPT1, COM1, \\Device\\HarddiskVolume1\, etc. maybe also we don't things in /etc, /boot, /proc to name a few. but in general, I think they are just absolute links outside of the package folder.

offending_files.append(offending_file)
conanfile.output.error("{token} '{item}' links to a {token} outside the package, "
"it's been removed.".format(item=offending_file, token=token))
os.unlink(fullpath)
Copy link
Contributor

@SSE4 SSE4 Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is nice that we output some error message(s) here, but we never return status code to the caller. so, for instance, conan will unable to return appropriate exit code to the user.

Copy link
Member Author

@jgsogo jgsogo Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a raise_if_error argument to the tool. It will raise for any of these errors. Returning from tools this kind of information could be something to add to here #7105 and reach a consensus.

Copy link
Member

@memsharded memsharded left a comment

Please mark it experimental in the docs.

conans/client/tools/files.py Outdated Show resolved Hide resolved
conans/client/tools/files.py Outdated Show resolved Hide resolved
@jgsogo jgsogo assigned czoido and unassigned jgsogo Jun 29, 2020
@jgsogo jgsogo added this to Low priority in Symlinks Jun 29, 2020
czoido
czoido approved these changes Jun 29, 2020
@czoido czoido merged commit 82f2937 into conan-io:develop Jun 29, 2020
2 checks passed
Symlinks automation moved this from Low priority to Closed Jun 29, 2020
@jgsogo jgsogo deleted the feature/tool-inspect-symlinks branch Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Symlinks
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

6 participants