mbrukman / autogen

Automatically generate boilerplate license comments.
Apache License 2.0
98 stars 27 forks source link

Resolve symlinks when determining the src directory. #3

Closed bramp closed 8 years ago

bramp commented 8 years ago

For example I had autogen.sh symlink'd from a bin directory:

$ ls -l ~/bin/
autogen.sh@ -> ../vendor/autogen/autogen.sh

When running autogen.sh it assumed the licences are in ~/bin. After this change, it correctly finds the licences in vendor/autogen/.

mbrukman commented 8 years ago

Thanks for the PR, @bramp!

Unfortunately, OS X does not have the realpath command (see this GitHub project which attempts to add it), and the other version of this that I'd used in the past, readlink -f is also not available on OS X (specifically, readlink itself does exist on OS X, but does not support the -f flag which is what I really needed).

Would you mind making this change conditional, based on the OS it's running on? E.g., something like:

# Resolve symlinks to find the right directory for licenses.
# Note that only Linux has `realpath` command available.
# TODO: find an appropriate solution for non-Linux OSes.
if [[ "$(uname -s)" == "Linux" ]]; then
  declare -r ... # your proposed version
else
  declare -r ... # as today
fi

Thoughts?

bramp commented 8 years ago

Happy to adapt this, I had read that realpath wasn't available on OS X, but on my mac it was installed. Now that I checked, I must have installed realpath at some point. I will work out a pure OS X + Linux solution. I'm hoping there is a cross platform way that doesn't involve a conditional.

mbrukman commented 8 years ago

I would be happy to see a cross-platform solution that does not involve a conditional, of course, but if you cannot find one, I will accept the conditional as a strict improvement over the status quo, so no worries if that's the best we can do currently.

As an alternative to the OS-based conditional, we can also use a capability-based conditional, which may be much better as it will only have 2 possible branches:

if which realpath > /dev/null 2>&1 ; then
  # This system has realpath; resolve symlinks properly.
  declare -r ...  # new model
else
  # This system does not have realpath; use default mode which ignores symlinks.
  declare -r ...  # current model
fi

The other benefit of this approach is that it automatically "upgrades" itself to the mode which handles symlinks properly if the user, like yourself, installs realpath on their OS X system.

mbrukman commented 8 years ago

Hi @bramp, any updates on this PR? I'd love to merge it with the OS-independent conditional proposed above. What do you think?

mbrukman commented 8 years ago

Hi @bramp, I've implemented one solution to this issue, though it ended up being a bit more complicated than I proposed earlier. Please take a look at my patch and let me know if you have suggestions for improvements!

Thanks for submitting this issue and getting me to think about this problem.

bramp commented 8 years ago

Thanks for implementing a fix. I reviewed your change and it looks good. The only suggestion I would have made is when checking which command is available, to just call "realpath" directly, instead of calling "which realpath".