Skip to content

Commit

Permalink
Change path searching (Review carefully, breaking change!)
Browse files Browse the repository at this point in the history
This completely changes the way the path options are handled to
determine installed binaries.
From each given path possible binaries are searched
- in locations matching binary releases and custom builds
- with the given path seen as
  - an installation dir or
  - a parent dir for multiple installations
  - the directory containing the 'lilypond' executable
  • Loading branch information
uliska committed Feb 22, 2017
1 parent d0a1bd4 commit 41a6920
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 12 deletions.
2 changes: 1 addition & 1 deletion lydiff.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def validate_lilypond_options(options):
'lilypondoptions': [''],
'path': os.pathsep.join(['/usr/bin',
'~/opt/*/bin',
'~/lilypond/usr/bin']),

This comment has been minimized.

Copy link
@uliska

uliska Feb 22, 2017

Author Collaborator

@joram-berger Actually I'm not sure how to deal with the first two default paths.

'~/lilypond']),
'diff': None,
'resolution': 200,
}
Expand Down
53 changes: 42 additions & 11 deletions lydiff/lyversions.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,61 @@

class Versions:
def __init__(self, paths, search=True):
self._list = []
if os.pathsep in paths:
self._paths = paths.split(os.pathsep)
else:
self._paths = [paths]
if search:
self._executables()
self._list = self._executables()
else:
self._list = []

def _executables(self):
"""search for executables and fill internal list"""
pattern = ['%s/lilypond' % p for p in self._paths]

def release_binary(base):
"""Generate a LilyPond executable name in a binary release directory"""
return os.path.join(base, 'usr', 'bin', 'lilypond')

def build_binary(base):
"""Generate a LilyPond executable name in a custom build directory"""
return os.path.join(base, 'out', 'bin', 'lilypond')

def package_binary(base):
"""Generate a LilyPond executable name directly in the path"""
return os.path.join(base, 'lilypond')

result = []
available_binaries = []
for p in self._paths:
# walk over all given paths and their immediate subdirectories,
# testing each for 'lilypond' executables, both in the place for
# binary releases and for local builds.

p = os.path.expanduser(p)
if not os.path.exists(p) and (not os.path.isdir(p)):
print("Warning: Path {} doesn't point to a directory. Skipping.".format(p))
continue

for p in pattern:
path = find_executable(p)
if path:
available_binaries.append(path)
else:
available_binaries += glob.glob(os.path.expanduser(p))
# Create list with path and all its subdirectories (assuming path is
# either the LilyPond directory itself or a parent directory
# with multiple installations)
dirs = [p] + [os.path.join(p, d) for d in os.listdir(p) if os.path.isdir(os.path.join(p, d))]

for d in dirs:
binaries = [b for b in [release_binary(d), build_binary(d), package_binary(d)] if os.access(b, os.X_OK)]
available_binaries.extend(binaries)

# Retrieve version for each LilyPond binary
# TODO: Should this be error-checked
# ('lilypond' might not be a LilyPond executable)?
for binary in available_binaries:
version = subprocess.check_output([binary, "--version"])
version = [int(i) for i in version.split()[2].decode().split('.')]
self._list.append((*version, binary))
self._list.sort()
result.append((*version, binary))

result.sort()
return result

def _tuple(self, string):
return tuple(int(i) for i in string.split("."))
Expand Down

7 comments on commit 41a6920

@joram-berger
Copy link
Owner

@joram-berger joram-berger commented on 41a6920 Feb 27, 2017

Choose a reason for hiding this comment

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

I don't understand this patch and how the path handling was changed. Problems I see now:

  • I seem to be unable to use wildcards like ~/my/path/*/bin before.
  • The searching algorithm finds /usr/bin/X11/lilypond which can not be run (it is executable but does not find the necessary internals of lilypond). It is able to state its version, though. So this might even be used and then fail.
  • Probably more because I can't test it in the current state (I can't use my custom lilypond versions, because they are not found).

@joram-berger
Copy link
Owner

Choose a reason for hiding this comment

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

I think the space separator should be re-enabled because we can use shell globbing then -p my/path/* just works. Or we have to use the glob module.

@uliska
Copy link
Collaborator Author

@uliska uliska commented on 41a6920 Feb 27, 2017

Choose a reason for hiding this comment

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

OK, there's two different things here: You don't understand what I intended with the patch, and I'm don't sufficiently aware where LIlyPond installations can reside beyond the two ways I have them.

The basic assumption is that the given path is the top level directory of a LilyPond installation. (e.g. ~/software/lilypond/releases/2.18.2). Relative from there it looks for a lilypond executable in either usr/bin (binary release, installed for the user) or out/bin (local build). The second assumption is that the given path is a parent directory containing multiple installations (in the above example this would be ~/software/lilypond/releases which contains 2.18.2, 2.19.54 and others). So all the subdirectories of the path are searched automatically and the * isn't actually necessary anymore. But OTOH this is why /usr/bin/X11/lilypond is found. The third assumption is that the given path directly contains a lilypond executable.

So that is what the current behaviour does, now to what it does (or know) not. I don't really know what's happening when running the binary release installer with sudo, i.e. install lilyPond globally, and I don't know how installations from the Linux distribution packages work. So these are the cases that may not be properly handled ATM.

Could you show where your LilyPond files reside (i.e. what led you to your implementation fo the searching)?

@joram-berger
Copy link
Owner

@joram-berger joram-berger commented on 41a6920 Feb 28, 2017

Choose a reason for hiding this comment

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

Top level of LilyPond installation sounds good. I have it in ~/path/sw/ly/lilypond-2.19.50-1/ for instance. The next sentences might be the problem: The executable is in ~/path/sw/ly/lilypond-2.19.50-1/bin, not ~/path/sw/ly/lilypond-2.19.50-1/out/bin or ~/path/sw/ly/lilypond-2.19.50-1/usr/bin. I wonder why it is not found in ~/path/sw/ly/lilypond-2.19.50-1/lilypond/usr/bin which is also present. But here I get a PermissionError from the check_output in lyversions.py:55. Apparently, it tries to execute lilypond directly in the base directory and it does not even exist.

I am fine with searching through all subdirectories. It will probably result in too many occurrences because every installer already contains at least two lilyponds. I have never encountered the right executable not being directly in a bin folder. So this could be an easy way to prevent the /usr/bin/X11/lilypond. IMHO it's better to look at all default places instead. I found these so far:

  • binary from website:
    • $PATH/bin/lilypond
    • $PATH/lilypond/usr/bin/lilypond (we could drop this ...)
  • Linux system:
    • /usr/bin
  • Custom build:
    • $PATH/out/bin/lilypond
  • Windows:
    • ?

@joram-berger
Copy link
Owner

Choose a reason for hiding this comment

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

One misconception on my side: if we look for bin/lilypond or in out/bin and usr/binall occurrences above should be found.
The permission denied is perhaps just a matter of the os.access and we need something like isfile to skip directories. I can't look at it right now. I will in the next days.

@uliska
Copy link
Collaborator Author

@uliska uliska commented on 41a6920 Feb 28, 2017

Choose a reason for hiding this comment

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

Some clarifications:

1
When you say "binary from website" you mean installed through the downloaded shell script, not using sudo? And what is $PATH referring to in this case (it looks you mean the home directory)?

2
I assume you have a file /usr/bin/lilypond present? (I obviously can't install lilypond through the package manager as there is "no installation candidate" - presumable because of the Guile2 issue). If you could you please try cat /usr/bin/lilypond, as I think this is the wrapper script?

@uliska
Copy link
Collaborator Author

@uliska uliska commented on 41a6920 Feb 28, 2017

Choose a reason for hiding this comment

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

The permission denied is perhaps just a matter of the os.access and we need something like isfile to skip directories.

Maybe this has to do with the difference between the lilypond executable and the wrapper script with the same name. I'd have to know which file causes the error - maybe check with a print() statement before that line?

Please sign in to comment.