Add a tool to fix_symlinks: remove broken ones and relativize all of them - #7178
Conversation
| " been removed.".format(item=offending_file, token=token)) | ||
| os.unlink(fullpath) | ||
| elif link_target != link_rel_target: | ||
| os.unlink(fullpath) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
| 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
memsharded
left a comment
There was a problem hiding this comment.
Please mark it experimental in the docs.
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 theconanfile.package_folderto fix symlinks:Related to #7093