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

Handle system detection like shell detection. #159

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

lucc
Copy link
Collaborator

@lucc lucc commented Apr 19, 2016

I think we can even add more commits to this PR but I wanted to discuss it first.

My idea was to get rid of all these if [ -n "$some_system" ] ... checks in the installed script in the end. Therefore we would need a file for each system that needs special code to emulate the things like seq or tail -n and so on. The git version would source the correct file and the makefile could then include the correct function into the "compiled" versions (configured and standalone scripts) at "compile" time.

The idea comes from pass again. On line 189 they do this:

source "$(dirname "$0")/platform/$(uname | cut -d _ -f 1 | tr '[:upper:]' '[:lower:]').sh" 2>/dev/null # PLATFORM_FUNCTION_FILE

and in the makefile they change the path of the platform file and install the correct platform file for the system into /lib:

PLATFORMFILE := src/platform/$(shell uname | cut -d _ -f 1 | tr '[:upper:]' '[:lower:]').sh
 # ...
ifneq ($(strip $(wildcard $(PLATFORMFILE))),)
install: install-common
    @install -v -d "$(DESTDIR)$(LIBDIR)/password-store" && install -m 0644 -v "$(PLATFORMFILE)" "$(DESTDIR)$(LIBDIR)/password-store/platform.sh"
    @install -v -d "$(DESTDIR)$(BINDIR)/"
    sed 's:.*PLATFORM_FUNCTION_FILE.*:source "$(DESTDIR)$(LIBDIR)/password-store/platform.sh":' src/password-store.sh > "$(DESTDIR)$(BINDIR)/pass"
    @chmod 0755 "$(DESTDIR)$(BINDIR)/pass"
else
install: install-common
    @install -v -d "$(DESTDIR)$(BINDIR)/"
    sed '/PLATFORM_FUNCTION_FILE/d' src/password-store.sh > "$(DESTDIR)$(BINDIR)/pass"
    @chmod 0755 "$(DESTDIR)$(BINDIR)/pass"
endif

@rkitover
Copy link
Owner

@lucc that's awesome re: the sed command in the Makefile, I didn't know you could do that.

As for generating platform specific code, it's up to you, but I don't think it's worth the effort. The platform checks are cached and don't effect runtime speed very much, and there are other areas we could spend time on that would be of more benefit, like adding proper support python and ruby docs etc.

Let me know when you want me to merge this.

rkitover added a commit that referenced this pull request Apr 19, 2016
Fix bug from #156 and include commit from #159
@rkitover
Copy link
Owner

@lucc I think I may need this for vimcat, at least the system detection include.

@lucc lucc force-pushed the system-detection branch from c432e0e to 257a799 Compare April 23, 2016 10:11
@lucc
Copy link
Collaborator Author

lucc commented Apr 23, 2016

Sorry I don't understand you last comment. Can you elaborate?

As far as I can see (with grep) the variables set in inc/system.sh are never used in vimcat. vimcat has a small bit of independent code to check the system it is running on.

@lucc lucc force-pushed the system-detection branch from 257a799 to 50915c6 Compare April 23, 2016 12:45
@rkitover
Copy link
Owner

@lucc see 58d5648

Can you put all the vars on one line in 049ff27, like:

linux=$linux;bsd=$bsd;solaris=$solaris;...

50915c6 is probably unnecessary.

@lucc
Copy link
Collaborator Author

lucc commented Apr 24, 2016

About 58d5648: I don't where vimcat uses the code from the system detection in vimpager (that I moved to inc/system.sh. As far as I can tell vimcat will call and parse uname itself and is therefore unaffected by the move of system detection code from the main vimpager script into inc/system.sh.

About 049ff27 and putting variable definitions on one line: In which file do you want to have them on one line? In inc/system.sh? But they where never redefined there before (they are only defined individually during the case stuff), why would that be necessary to redefine them on one line? They are also not (re)defined in script/find_system, only echoed to paste them into the configured script from the makefile. So I am sorry but I don't understand where you want me to put that line of code and also why.

About 50915c6: This is actually a preparational commit for something that I had planned (and that was the original intention of this PR): I think we could handle commands like sed, awk, grep -q and so on in a similar fashion like we handle $POSIX_SHELL. Currently we have wrappers for these commands that do some checks of the system and call the correct executable. I think these checks could be moved to a script and the makefile. The git version could then include the script, the bundled version would bundle the script and the configured version would have the path to these executables hard coded during make (similar to what we do with $POSIX_SHELL).

@rkitover
Copy link
Owner

@lucc:

About 58d5648: I don't where vimcat uses the code from the system detection in vimpager (that I moved to inc/system.sh. As far as I can tell vimcat will call and parse uname itself and is therefore unaffected by the move of system detection code from the main vimpager script into inc/system.sh.

Yes, the point is that it does not use it but I want to use it there.

About 049ff27 and putting variable definitions on one line: In which file do you want to have them on one line? In inc/system.sh? But they where never redefined there before (they are only defined individually during the case stuff), why would that be necessary to redefine them on one line? They are also not (re)defined in script/find_system, only echoed to paste them into the configured script from the makefile. So I am sorry but I don't understand where you want me to put that line of code and also why.

The echo should be one line, so that the included block of code is not huge.

About 50915c6: This is actually a preparational commit for something that I had planned (and that was the original intention of this PR): I think we could handle commands like sed, awk, grep -q and so on in a similar fashion like we handle $POSIX_SHELL. Currently we have wrappers for these commands that do some checks of the system and call the correct executable. I think these checks could be moved to a script and the makefile. The git version could then include the script, the bundled version would bundle the script and the configured version would have the path to these executables hard coded during make (similar to what we do with $POSIX_SHELL).

This will not work. There are too many differences between configurations of OSes and too many OSes. E.g. a Solaris user may have gsed and gawk installed or not etc. Further e.g. openbsd/netbsd/freebsd are essentially the same for my purposes and so don't need separate platform files, and the same caveats apply there about user-installed utilities. And most importantly this is completely unnecessary, adds a layer of complexity for no gain in either performance or maintainability.

@lucc lucc force-pushed the system-detection branch from 50915c6 to f3eacbb Compare April 24, 2016 17:23
@lucc
Copy link
Collaborator Author

lucc commented Apr 24, 2016

Ok, I pushed some new commits (and squashed the old ones). Maybe I can explain my idea better with this.

for 1: you could try to source inc/platform.sh from vimcatand use its variables now.

for 2: the echo in scripts/find_platform uses backslashes and only prints one line. You can check: make vimpager.configured; head -n 13 vimpager.configured. Should the echo command itself also be on one line?

for 3: Maybe 29b81d7 can make it more clear what I think. The idea is started there and I am looking for more places where I can do this.

Related to 29b81d7: I noticed (with the new -x option) that the old wrapper functions for sed and awk where trying to find the correct executable every time they where run so the variable was not reused.

f3eacbb is just another idea for more variables and code paths that could be incorporated in this approach. In the end extract_bundled_scripts() could be only defined in the standalone version.

@rkitover
Copy link
Owner

Related to 29b81d7: I noticed (with the new -x option) that the old wrapper functions for sed and awk where trying to find the correct executable every time they where run so the variable was not reused.

Good catch there, that happens because they are used in subprocesses, so they have to be primed at init time.

@lucc
Copy link
Collaborator Author

lucc commented Apr 24, 2016

I added some more commits. The head and tail wrapper functions have the same problem as the sed and awk wrappers.

The commits about $runtime, $PREFIX and $vimcat show more of my plan in this PR.

@lucc lucc force-pushed the system-detection branch from c98c122 to a6a8688 Compare April 25, 2016 08:58
@rkitover
Copy link
Owner

rkitover commented May 8, 2016

@lucc can you update this one and I'll take a look

@lucc lucc force-pushed the system-detection branch from a6a8688 to abec22c Compare May 9, 2016 21:10
@lucc
Copy link
Collaborator Author

lucc commented May 9, 2016

Dropped some commits due to the recent config PR.

@rkitover
Copy link
Owner

@lucc This is good, but I was thinking, why not put the functions into platform.sh as well, that way the whole thing is encapsulated in that file.

Also please rebase and squash into fewer commits.

@lucc
Copy link
Collaborator Author

lucc commented May 12, 2016

Do you mean awk () ..., sed () ... head_n () ... and tail_n () ... or which functions? If we put these into platform.sh we have to perform two different actions with platform.sh when building *.configured: sourcing platform.sh in order to get the correct value for $AWK and so on, and then including parts of the file into the configured file (the function definitions) in order to really use the variables when calling awk and friends.

And I don't feel like these functions really belong in platform.sh as they are just the contrary, they are platform independent due to the variables set in the platform file.

We could of course add marker comments to platform.sh like this

# setting variables
AWK=...
: ...

# START OF FUNCTION DEFINITIONS
awk () {
....

and in the makefile we could do something like sed -n '/^# START OF FUNCTION DEFINITIONS$/,$p' inc/plaftorm.sh >> $@.work

Or do you have a better idea?

@rkitover
Copy link
Owner

We could also rearrange the runtime into:

/usr/share/vimpager/vim
/usr/share/vimpager/inc

then the platform file can be included from the share dir.

Also I can rewrite head_n and tail_n to use normal head/tail syntax.

@lucc
Copy link
Collaborator Author

lucc commented May 12, 2016

Which files would go into /usr/share/vimpager/inc? bundled_scripts.sh and do_uudecode.sh are not needed for the installed version at all. And prologue.sh is sourced during install and the result pasted into the installed script.

I like the last point and would like to keep it. It feels strange to me that installed software should investigate the system it is running on and try to bootstrap stuff like the shell and head/tail syntax it can use. In my opinion such bootstraping is ugly and should stay in ./configure && make && make install.

rkitover added a commit that referenced this pull request May 12, 2016
Instead of using head_n/tail_n functions use head and tail functions
that take -<N> as the first parameter, like the actual utilities.

Also for the man page detection, detect NAME with overstrikes as well.
@rkitover
Copy link
Owner

@luc ^^ I rewrote head and tail to use normal syntax.

Anyway, head and tail are not that important, except in the case of tail not understanding -- file syntax anymore, but as I just discovered, I don't use tail anywhere at the moment. But I don't know if I will in the future, or someone else will.

Anyway, we can solve 99% of these problems by just checking that /usr/xpg6/bin:/usr/xpg4/bin is in the PATH and prepending it if necessary. Of course I don't know the configuration of every possible platform, so this more cautious approach may be better for edge cases.

The reason the checks have to run at runtime is because the configuration of the system can change after the software is installed. E.g. on Solaris, someone may first install vimpager and then only later install OpenCSW and the GNU utilities.

@rkitover
Copy link
Owner

@lucc yeah maybe /usr/share/vimpager/sh instead of inc and we'd only put factored out library code there like platform.sh (shared between vimpager and vimcat) and the like.

@lucc
Copy link
Collaborator Author

lucc commented May 12, 2016

I like d501c9a !!

I feel like I have to give up on understanding Solaris and it maybe conformity to POSIX.

@lucc lucc force-pushed the system-detection branch from abec22c to ac84bdd Compare May 12, 2016 10:58
@rkitover
Copy link
Owner

I will explain Solaris some, although I'm by no means an expert.

First, I suggest you get a Solaris 10 virtual machine image to play with here.

it's in VirtualBox format, VMWare and Parallels can import this, and there are converters. Or you can just use VirtualBox.

Install the appropriate UTF-8 locale and OpenCSW.

Use these instructions to install the locale. You'll need to download the iso and mount it as a virtual disc in your VM for this.

Ok, now on to the good stuff.

Solaris keeps original UNIX utilities in /bin, many of these are very old and extremely limited. For example, /bin/awk is original awk and /bin/sed is original sed. Original awk is missing a lot of things like functions, while original sed is missing a lot of things like regular expression alternation \| support. Why Sun did this I don't know, but they provided newer utilities in two places:

  1. '/usr/xpg6/bin`
rkitover@sol10  ➤  ls /usr/xpg6/bin
bc  crontab  dc  ed  edit  ex  expr  getconf  ls  stty  tr  vedit  vi  view  xargs
  1. /usr/xpg4/bin
rkitover@sol10  ➤  ls /usr/xpg4/bin
alias     batch  command  delta  egrep  fg     getconf  ipcs  ls    nice   pr    sh    tr       vedit
ar        bg     cp       df     env    fgrep  getopts  jobs  m4    nl     read  sort  type     vi
at        cd     crontab  du     ex     file   grep     kill  make  nm     rm    stty  ulimit   view
awk       chgrp  ctags    ed     expr   find   hash     link  more  nohup  sccs  tail  umask    wait
basename  chown  date     edit   fc     get    id       ln    mv    od     sed   test  unalias  who

The xpg6 utils are newer, but most are in the xpg4 directory.

These are significantly better and generally POSIX compliant. For example, /usr/xpg4/bin/awk is "new awk", the language most people think of as awk and it runs most regular awk code.

Also /bin/sh is what I told you about earlier is close to heirloom-sh, original Bourne. While /usr/xpg4/bin/sh is a much nicer ksh and is mostly POSIX compliant.

@lucc
Copy link
Collaborator Author

lucc commented May 12, 2016

I managed to download and run the Solaris VM. So now I can see the oddities myself.

I want to share with you some of my thoughts and explain why I am so annoyingly persistent about all these bootstrapping and executable finding stuff. I hope that the other systems do not undermine my idea by being equally strange in a different way. (Please tell me if there is one.)

  1. I think that our bootstrapping code is very complex so I aim to reduce and simplify it.
  2. I am often wondering why we have to look for a "better" awk/sed/head/tail... . What does "better" in this case mean? I think we only need an executable that can handle the scripts we pass to it correctly (for me that counts as "good enough" and if it doesn't handle them correctly its "bad", to me there is no "better").
  3. I assume that it is the job of the admin/distro/user to set $PATH not the job of a program. a) they know the system much better or might have changed it in some unthinkable way, b) it reduces duplication because every program would have to figure out how to set $PATH on every system. Not modifying $PATH gives control back to the user who might want to test vimpager with some special version of program X by modifying $PATH before calling vimpager. We would stop them from doing this and maybe also introduce bugs like mentioned here in connection with dein.
  4. I did now see that Solaris is not helping us much with point 3, but for 2 I think we can use /usr/bin/{awk,sed} as all the things we use seem to be present (I just looked at the man page I did not run code to test this but keep in mind that filename encoding was recently reimplemented in a simpler fashion). If it is really necessary we can still add /usr/xpg4/bin to $PATH but I think we don't even use any program from /usr/xpg6/bin so we need not add that.
  5. This also means that we should not need to care if someone installs gnu coreutils on Solaris after installing vimpager. We will still find the version of awk/... that works for us. If somebody is placing an older (incompatible) version earlier in $PATH I don't see any way to save guard us (we don't have one right now), they just seem to want to break stuff so I would not spend my time stopping them.
  6. B/c of 2 I also vote not to look for special versions like nakw, gsed, ... if possible. As long as the thing we find can do the job we are just complicating maintenance and debugging.

@lucc lucc force-pushed the system-detection branch from ac84bdd to c01344c Compare May 12, 2016 21:37
@lucc lucc force-pushed the system-detection branch 2 times, most recently from 57241d4 to d566a1f Compare May 24, 2016 23:04
@lucc lucc force-pushed the system-detection branch 2 times, most recently from 0219fce to 193783a Compare June 21, 2016 16:54
This simplifies some search patterns that are used in the makefile to copy
sourced scripts into the "compiled" versions.  The simplified patterns should
be more robust to minor comment or quoting changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants