Bug 232732 - build of expect in lang/expect does not install autoexpect script correctly
Summary: build of expect in lang/expect does not install autoexpect script correctly
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: amd64 Any
: --- Affects Only Me
Assignee: freebsd-tcltk (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-26 22:13 UTC by Dennis Clarke
Modified: 2018-10-30 11:04 UTC (History)
2 users (show)

See Also:
gahr: maintainer-feedback-


Attachments
svn-diff-expect (3.28 KB, patch)
2018-10-26 22:30 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff-expect_v2 (3.26 KB, patch)
2018-10-26 22:37 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff-expect_v3 (3.41 KB, text/plain)
2018-10-27 20:47 UTC, Walter Schwarzenfeld
gahr: maintainer-approval-
Details
svn-diff-expect_v4 (4.35 KB, patch)
2018-10-29 18:57 UTC, Walter Schwarzenfeld
no flags Details | Diff
svn-diff-expect_v5 (4.50 KB, patch)
2018-10-29 22:48 UTC, Walter Schwarzenfeld
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dennis Clarke 2018-10-26 22:13:29 UTC
The build of lang/expect looks fine and drags in tcl86-8.6.8 and builds that
correctly but at the end result the autoexpect script gets installed down 
into /usr/local/share/expect/autoexpect when it should be in /usr/local/bin 
as well as chmod 755.

# ls -lapb /usr/local/share/expect/autoexpect
-rw-r--r--  1 root  wheel  7706 Oct 26 22:02 /usr/local/share/expect/autoexpect

That is wrong.
Comment 1 Walter Schwarzenfeld freebsd_triage 2018-10-26 22:30:24 UTC
Created attachment 198682 [details]
svn-diff-expect

If I am right, this should solve it.
Comment 2 Walter Schwarzenfeld freebsd_triage 2018-10-26 22:37:16 UTC
Created attachment 198683 [details]
svn-diff-expect_v2

There was a little error in pkg-plist.
Comment 3 Dennis Clarke 2018-10-27 02:12:06 UTC
cool .. this was an issue that came up on the irc chan and I had built
expect/tk/tcl and friends over and over during the past twenty years or so
and was surprised that this was .. well .. where it ended up.
Looks like an easy fix. Thank you.
Comment 4 Walter Schwarzenfeld freebsd_triage 2018-10-27 02:22:34 UTC
Sorry, but it is wriong:
https://svnweb.freebsd.org/ports?view=revision&revision=346491

it conflicts with weather, so the patch is not ok.
Comment 5 Walter Schwarzenfeld freebsd_triage 2018-10-27 02:53:40 UTC
The only thing it is needed
chmod -R 0755 /usr/local/share/expect
Comment 6 Walter Schwarzenfeld freebsd_triage 2018-10-27 19:26:25 UTC
And:
ln -s /usr/local/bin/tclsh8.6 /usr/local/bin/tcls
and add
/usr/local/share/expect
to $PATH
Comment 7 Walter Schwarzenfeld freebsd_triage 2018-10-27 19:27:06 UTC
typo, should be
ln -s /usr/local/bin/tclsh8.6 /usr/local/bin/tclsh
Comment 8 Dennis Clarke 2018-10-27 20:19:54 UTC
(In reply to w.schwarzenfeld from comment #6)
The script is supposed to be in /usr/local/bin as per usual and there
is no need to add anything to a PATH.  It would "just work"(tm).
Comment 9 Walter Schwarzenfeld freebsd_triage 2018-10-27 20:24:50 UTC
Yes, see comment4, it was conflicting with astro/weather. And it seems it was no good change.

I postet above only as quick workaround for the moment, could never be a solution.
Comment 10 Walter Schwarzenfeld freebsd_triage 2018-10-27 20:47:52 UTC
Created attachment 198696 [details]
svn-diff-expect_v3

Found a better solution.
Comment 11 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-29 10:45:27 UTC
Comment on attachment 198696 [details]
svn-diff-expect_v3

expect should definitely *not* symlink tclsh8.6 to tclsh. lang/tcl-wrapper is what you're looking for.
Comment 12 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-29 10:51:01 UTC
The issue, in general, is that expect installs several scripts with names identical to other well-established packages. astro/weather is an example, mail/rftp would be another.

I think it's still reasonable to have them all in the dedicated directory %%DATADIR%% and let users either add that to PATH if needed, or call the scripts with tclsh8.6 /usr/local/share/expect/foo.

On the other hand, since the scripts invoke tclsh in their shebangs, I think it's reasonable to let expect depend on tcl-wrapper.

Would that work for you?
Comment 13 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-29 11:06:51 UTC
Better yet, modify the scripts to invoke the correct TCLSH. That would be as simple as:

@@ -9,11 +9,12 @@
        -for i in $(SCRIPT_LIST) ; do \
          if [ -f $$i ] ; then \
 -          $(INSTALL_PROGRAM) $$i $(DESTDIR)$(prefix)/bin/$$i ; \
++          sed -i'' -e 's|tclsh|${TCLSH_PROG}|' $$i ; \
 +          $(INSTALL_SCRIPT) $$i $(DESTDIR)$(datadir)/$$i ; \
            rm -f $$i ; \
          else true; fi ; \
        done
Comment 14 Walter Schwarzenfeld freebsd_triage 2018-10-29 13:17:08 UTC
If you install it in %%DATADIR%% you have to make at least a pkg-message.
The manpage should not in %%DATADIR%%. You cannot call it there.
----
Your solution with tclsh is better. I know mine was not good, but had no better idea.
Comment 15 Walter Schwarzenfeld freebsd_triage 2018-10-29 13:26:37 UTC
And it you decide for  %%DATADIR%% it would be nice for the user, you can add something like
a "start-script" for the scripts in it. 

something like expect-scripts
if [ -e $1 ]; then
/usr/local/share/expect/S1
fi

(I did not care in the moment of the parameters....)
Comment 16 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-29 13:52:13 UTC
(In reply to w.schwarzenfeld from comment #14)
> If you install it in %%DATADIR%% you have to make at least a pkg-message.

I think this is a reasonable proposition.

(In reply to w.schwarzenfeld from comment #14)
> The manpage should not in %%DATADIR%%. You cannot call it there.

If you put them in man1, you will likely not have solved the conflict problem.

(In reply to w.schwarzenfeld from comment #15)

> And it you decide for  %%DATADIR%% it would be nice for the user, you can add something like a "start-script" for the scripts in it. 

Not sure I get this part. Can you please elaborate / provide a patch?
Comment 17 Walter Schwarzenfeld freebsd_triage 2018-10-29 14:27:45 UTC
Put the manpages in man3, like I have done.

expect-scripts  #I use this name, name it like you want

#!/usr/local/bin/bash
string=$@
if [ "$string" == "" ]; then
echo "USAGE: expect-scripts command parameters"
echo "example:  expect-scripts autoexpect ftp ftp.myserver.org"
exit
fi
command=/usr/local/share/expect/$string
if [ -e /usr/local/share/expect/$1 ]; then
eval "$command"
fi
Comment 18 Walter Schwarzenfeld freebsd_triage 2018-10-29 14:42:10 UTC
Added error message and --help

#!/usr/local/bin/bash
string=$@
if [ "$string" == "" -o "--help" ]; then
echo "USAGE: expect-scripts command parameters"
echo "example:  expect-scripts autoexpect ftp ftp.myserver.org"
exit
fi
command=/usr/local/share/expect/$string
if [ -e /usr/local/share/expect/$1 ]; then
eval "$command"
else
echo "command not found or syntax-error"
fi
Comment 19 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-29 14:43:54 UTC
man3 is wrong - that's for system library functions.

So, how is having to use "my_wrapper weather arg1 arg2 arg3" an improvement over using "/usr/loca/share/expect/weather arg1 arg2 arg3"?
Comment 20 Walter Schwarzenfeld freebsd_triage 2018-10-29 14:51:12 UTC
There is on e man directory for misc. How can you call the man page from  /usr/local/share/expect/some_script ?  man -M /usr/local/.... does not work.

> So, how is having to use "my_wrapper weather arg1 arg2 arg3" an improvement over using 
>"/usr/loca/share/expect/weather arg1 arg2 arg3

How ever we did, I should work simple,without to type the whole path.
Comment 21 Walter Schwarzenfeld freebsd_triage 2018-10-29 14:58:33 UTC
lot of typos
How ever we do, it should work simple,without to type the whole path.
Comment 22 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-29 15:07:49 UTC
(In reply to w.schwarzenfeld from comment #20)
> There is on e man directory for misc. How can you call the man page from  /usr/local/share/expect/some_script

man /usr/local/share/expect/some_script

> How ever we do, it should work simple,without to type the whole path.

Add /usr/local/share/expect to PATH, so you don't have to introduce any additional wrapper script.

Or nullfs mount it over to /usr/local/bin.

There are multiple options to work around the nuisance of having the scripts in a separate location.

There is no option to have the scripts in /usr/local/bin and not conflict with other ports.

I think things are reasonable as they are.
Comment 23 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-29 15:17:22 UTC
We could think of putting man pages in the n section, where Tcl man pages usually go. I'll try and see if it doesn't raise any conflicts.
Comment 24 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-29 15:23:25 UTC
Or rename all scripts and manpages with an "expect-" prefix.

/usr/local/bin/expect-mkpasswd
/usr/local/man/man1/expect-mkpasswd.1.gz
Comment 25 Walter Schwarzenfeld freebsd_triage 2018-10-29 16:05:00 UTC
That's an idea. But don't forget it to mention it in pkg-message.
Comment 26 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-29 16:35:45 UTC
Patches are welcome, as always.
Comment 27 Walter Schwarzenfeld freebsd_triage 2018-10-29 18:57:01 UTC
Created attachment 198744 [details]
svn-diff-expect_v4

Here it is. I hope it is right.
Comment 28 Walter Schwarzenfeld freebsd_triage 2018-10-29 22:48:54 UTC
Created attachment 198756 [details]
svn-diff-expect_v5

Remove chmod from Makefile (did it in pkg-plist). Bump PORTEPOCH, pet portlint.
Comment 29 commit-hook freebsd_committer freebsd_triage 2018-10-30 11:03:54 UTC
A commit references this bug:

Author: gahr
Date: Tue Oct 30 11:03:31 UTC 2018
New revision: 483467
URL: https://svnweb.freebsd.org/changeset/ports/483467

Log:
  lang/expect: install example scripts and manpages in the proper location

  This commit changes the location of the example scripts and manpages. They are
  now installed in the proper PREFIX/bin and PREFIX/man/man1 - so they are in
  path and reachable by man(1) - and are renamed with an expect_ prefix to avoid
  conflicts due to their common names.

  A pkg-message has been added to notify users of this change. Portrevision has
  been bumped.

  PR:		232732 (based on)
  Submitted by:	Walter Schwarzenfeld <w.schwarzenfeld@utanet.at>
  Reported by:	Dennis Clarke <dclarke@blastwave.org>

Changes:
  head/lang/expect/Makefile
  head/lang/expect/files/patch-Makefile.in
  head/lang/expect/files/patch-tclconfig_tcl.m4
  head/lang/expect/files/pkg-message.in
  head/lang/expect/pkg-plist
Comment 30 Pietro Cerutti freebsd_committer freebsd_triage 2018-10-30 11:04:16 UTC
I opted for expect_ as a prefix, which is what debian does too:
https://packages.debian.org/stretch/amd64/expect/filelist