Support any platform w/readlink & remove dupe code

[pjeby]

This PR is a proof-of-concept to address the issues discussed in #49. Specifically, it:

• Removes the unnecessary (and undesirable) physical path resolution in basher-link
• Removes code duplication (of resolve_link) across multiple files
• Implements symlink resolution in a way that works on any platform with readlink, including busybox, docker alpine bash images, and OS X without homebrew
• Vendors the symlink resolution code in such a way that it does not require copy-and-paste to update, nor does it require extra steps for end users to install

It does this by adding a new file, libexec/basher.realpath. This file can be sourced by anything that wants to use realpath.* functions (such as basher-link and its tests), and it also works as a bootstrap loader for basher. bin/basher links to libexec/basher.realpath, which when run computes the location of the libexec dir, puts it on the path, and then exec's libexec/basher. This allows libexec/basher to forego any symlink calculations of its own.

The libexec/basher.realpath file is tracked under revision control, so a checkout of basher is immediately usable without any other steps, just as before.

However, if a new upstream version of bashup/realpaths is needed, a developer can:

$ git -C vendor/bashup/realpaths pull  # pull updated version
$ make vendor test  # rebuild and test
$ git commit -a     # check in changes

In this way, the distributed version can be kept up to date with the upstream code, without any manual copy-and-paste.

[kadaan]

@pjeby This looks really good. I tested it locally and it works! Hopefully this will make it in.

[kadaan]

@juanibiapina Any chance you can give feedback on this?

[juanibiapina]

Later today or over the weekend. Sorry I've been unresponsive.

…

On Thu, Feb 7, 2019, 7:18 PM Joel Baranick ***@***.*** wrote: @juanibiapina <https://github.com/juanibiapina> Any chance you can give feedback on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAitvPeksoH5MSwWCuXPmWrAESX7tSpCks5vLG4HgaJpZM4agdHI> .

[kadaan]

@juanibiapina No worries. Real life is intruding on fun...

[juanibiapina]

Thanks for the contributions. I can't express how exited I am to see so many people participating on the discussions!

The bottom line issue here seems to be support for OSX without homebrew. Is this is hard requirement? Making basher depend on GNU coreutils is not a problem at all if we document this dependency and add an early abort (even travis has it by default). This commit has been valid for 6 years now: rbenv/rbenv@81bb14e

If this solves all problems, I'm ok with simplifying the link command so it doesn't need the -f flag for now, and then we can talk how to improve link (perhaps even alternative solutions altogether, like an external tool for this, or different usage etc).

Other than that, tests now take one minute more to run on my computer, which makes me think the symlink resolution has become slower. Also the entry point has become complicated to follow for a person without much experience with shell scripts.

[kadaan]

@juanibiapina Personally, I'm fine with greadlink support ala rbenv/rbenv@81bb14e. That said, basher doesn't support that right now as it doesn't check for greadlink

[juanibiapina]

@kadaan It checks in the main command, but not in the link command, which I'm proposing we reduce the functionality, change usage or extract it.

[kadaan]

@juanibiapina Whatever makes it work on OS X is fine. I'm fine adding coreutils to my system. That said, I don't want to have to replace system libraries with coreutils copies.

[pjeby]

I've fixed the test performance issue, so the performance is now about the same as it was without the PR, at least on my machine. (The issue was in the tests themselves, not the changes to the main code; the main code should actually be faster for link targets that aren't themselves symlinks.)

As for the link command, it actually doesn't need readlink at all, let alone -f. It shouldn't resolve symlinks, but keep them intact. What it needs is absolute paths, which can be had without using readlink in any capacity.

The main purpose of this PR is to stop reinventing wheels in every basher subcommand that needs to do path manipulation: if they need an absolute path, they can use realpath.absolute; if they need a relative path they can use realpath.relative, if they need to follow a symlink to a real file or directory, there's realpath.follow, and so on. By using a documented, extensively tested library for these things, that has no third-party installation requirements (e.g. coreutils or homebrew), each basher module can avoid reinventing wheels every time their path-related requirements change.

The main basher script already had an adhoc implementation of realpath.location (to work around the missing realpath/readlink -f in the absence of coreutils), relying on its own version of follow_link, while the link command and test scripts had their own, distinct implementations of follow_link.

(Ironically, the entire issue of readlink -f would never have arisen if the link subcommand also copied the abs_dirname code from the basher main script, as it is already a bash reimplementation of dirname $(readlink -f "$1"). This patch is just substituting documented and independently tested routines for these existing, untested and ad-hoc reimplementations.)

[juanibiapina]

Running basher on this PR takes 35ms vs 45ms in master branch.

[juanibiapina]

I want to find a way to use realpaths, but 3 things are bothering me:

• stuff inside libexec which are not really sub commands
• files with names that refer to realpath
• the generation of the main binary mixes things in a convoluted way (sorry I can't express this better right now)

What if we rewrite this as a generic wrapper script that creates the main binary inside the bin folder without a symlink? The wrapper can be commited and its generation logic (and all parts) can be extracted into a specific set of files (or subfolder). That way we have a clear separation for everything used to generate the main binary. Other hard dependencies can also be added this way in the future.

[pjeby]

I don't have any objections in principle, but I'm a bit curious how it would work.

On the pro side, if the tests actually run this new wrapper it'd be great, because right now they run libexec/basher and bypass testing the wrapper. On the con side, the wrapper would have to be regenerated every time you made a change to the main basher script.

Any dependencies, though, need to either be on PATH or some other environment variable in order to be sourced by the things that use them. So libexec seems like the right place, at least for the library to be sourced. Anything else gets trickier. (I do agree that the loader .stub can be moved someplace else, perhaps lib/?)

Hm. It just occurred to me that you might be saying that you want libexec/basher.realpath to become bin/basher, which would work fine for booting basher, as it could put libexec on the path and then run libexec/basher. Except that then if, say, basher-link wants to source it, how would it know where to source it from? I suppose we could have a vendor-lib directory that gets put into an environment variable?

Anyway, I'm definitely willing to make changes, I'm just having trouble picturing what you want and how it would work. This was very much an ad-hoc thing I threw together to show it could be done, and didn't give a lot of thought to the stub placement, because at first I didn't know I even needed a stub, I thought it could all be done in the makefile (but that got too messy; the sed is bad enough as it is.)

Also, if there's a generic way to make realpaths into a boot loader for scripts like this, I'd be willing to take changes upstream. The .stub stuff was something I was thinking about adding to it anyway, because it would allow projects like this (not to mention projects installed by basher!) to find their "stuff", including their own recursively-cloned dependencies.

[juanibiapina]

I don't have a clear vision for the solution for this. The current state bothers me because I feel things are out of place and it is confusing to find what you're looking for.

I've tried a couple of experiments and I'm still unable to get to a state I'm comfortable with. I still want to try one more time tough.

[pjeby]

What things are you looking for, that are confusing and out of place? Maybe I can re-arrange them in some fashion.