search

morrownr / 8821cu-20210916

Linux Driver for USB WiFi Adapters that are based on the RTL8811CU, RTL8821CU, RTL8821CUH and RTL8731AU Chipsets - v5.12.0.4
Other
606 stars 134 forks source link

Should not assume use of nano as editor #54

Closed colincdean closed 1 year ago

colincdean commented 1 year ago

Both install-driver.sh and edit-options.sh hard-wire use of nano as editor for /etc/modprobe.d/8821cu.conf.

On Debian-based systems at least, it would be preferable to honor the user's choice of editor, by invoking sensible-editor if found, or editor if not, instead of nano. If not familiar with this idea, see sensible-editor(1) and update-alternatives(1) man pages.

I'm not a nano user (preferring emacs or vi), and other Linux tools (such as crontab) will let me use what I want.

To avoid code duplication, maybe install-driver.sh should employ edit-options.sh?

M0les commented 1 year ago

https://github.com/morrownr/8821cu-20210916/pull/53 ?

morrownr commented 1 year ago

Gents,

Give me a chance to ponder this issue. Miles recently posted a PR to the old version of this driver and I am holding off merging it as I wanted to work the issue here.

My first inclination is simply to change to using edlin as the default. Okay, that is a joke. If you don't get it, you are too young.

On Debian-based systems at least, it would be preferable to honor the user's choice of editor, by invoking sensible-editor if found

What if we borrow that script and include it in the driver so that we can be sure it is available? Remember, one of the primary goals in supporting the drivers here is make them as easy as possible to install on as many Linux distros as possible. I have to carefully consider solutions that are distro specific.

To avoid code duplication, maybe install-driver.sh should employ edit-options.sh?

The reason for the separate script is to give the ability to edit the proper file separately from installation. We have a lot of newbies and no matter what you put in the docs, which they won't read, right @Jibun-no-Kage ?, they will edit the 8821cu.conf in the driver directory...which will have no affect at all. Alternative solutions are welcome.

Nick

colincdean commented 1 year ago

@morrownr

I fear the sensible-editor script may itself be distro-specific, and certainly has some hard-wired assumptions about return codes.

So, on further thought, I'd be happy if the driver scripts kept it simple and just checked for the EDITOR and VISUAL environment variables to override use of nano, as long as that was mentioned in README.md.

morrownr commented 1 year ago

I'd be happy if the driver scripts kept it simple and just checked for the EDITOR and VISUAL environment variables to override use of nano, as long as that was mentioned in README.md.

With that thought in mind, I will compare your thoughts with what Miles submitting and see where we go from here. I checked and EDITOR/VISUAL are not set on my system and I am using a Debian based system (Mint).

I think it is important to throw the options file up during installation so that users are aware of it. We'll see.

Thanks,

Nick

M0les commented 1 year ago

I don't think I have any authority to make a decision, but some opinions I offer:

Jibun-no-Kage commented 1 year ago

Use vi as preference... oh, the horror... the horror! Sorry, I remember when all I could use vi. I have been spoiled since nano. :) Geez, edlin? Please how about 'edt' and writing custom edi.ini files? Any PDP-11 users out here? And I did say PDP-11.... Not VAX. [Cough]

Reading... ah... is that noun or a verb? What is it again?

morrownr commented 1 year ago

Miles,

but nano is more forgiving on new users.

That is the reason I used it. I wrote a text editor several years ago... in REXX. Now Jibun is really going to get fired up.

Both PRs I submitted basically try ${EDITOR} ➧ nano ➧ fail. It might be better as ${VISUAL} ➧ ${EDITOR} ➧ nano ➧ fail.

I'll look at your PR again and see if I can start working it in sometime this week.

Hey, Alexa, what text editor do I like?

Nick

alkisg commented 1 year ago

First solution there has a nice summary: https://stackoverflow.com/questions/21045537/c-unix-open-a-text-file-with-the-default-editor

M0les commented 1 year ago

First solution there has a nice summary: https://stackoverflow.com/questions/21045537/c-unix-open-a-text-file-with-the-default-editor

I could easily implement that (The first solution by "pts"), but it now makes "launch a text editor" into a 9-clause cascading if statement. Certainly makes a case for defining a separate script or function to be called from both places we want to edit the config file from.

I think there are a few errors in that solution, though (or is it just my preference bias?):

Funny aside: My text-mode Fedora install does have xdg-open installed (Admittedly a case that's specifically excluded in the StackExchange solution). The way it handles opening a text file is to print the MIME type (applicatiion/x-sh) and prompt to "D) download or C) cancel". If I select "D" (download), it fires-up Lynx (a text-mode web browser) on a file-download form using the full file:// URI for the file in question. I didn't actually "download" the file as it seemed a bit... pointless?

M0les commented 1 year ago

I believe the consensus solution is:

${VISUAL} ➧ ${EDITOR} ➧ nano ➧ fail

Jibun-no-Kage commented 1 year ago

That is the reason I used it. I wrote a text editor several years ago... in REXX. Now Jibun is really going to get fired up.

What? Did someone say something? [Snoring in the background]

morrownr commented 1 year ago

I believe the consensus solution is:

${VISUAL} ➧ ${EDITOR} ➧ nano ➧ fail

Unless there is an objection, we should head in this direction. We can always adjust course if need be. Let me suggest this plan:

Is that going to work?

Separate subject: Sometimes you need to step back and look at the big picture. The last driver code changes were made before Christmas if memory serves me correctly. At this point, I can't find any driver errors. Nothing. I have been beating up monitor mode and AP mode and I simply can't find any problems. MU-MIMO is working well. DFS channels are working well in AP mode. Nobody else is reporting any issues with the driver either and there are about 16 of us here. So... here we are and the only things we have going right now is this editor issue and the low mem issue. I think it is about time to post an issue to all that proposes a release date so folks can make a plan to finish the testing they are doing. If I proposed a 2023-02-02 release date, is that going to be enough time to work and test this issue?

Nick

On Mon, Jan 16, 2023 at 4:29 AM Miles Goodhew @.***> wrote:

I believe the consensus solution is:

${VISUAL} ➧ ${EDITOR} ➧ nano ➧ fail

— Reply to this email directly, view it on GitHub https://github.com/morrownr/8821cu-20210916/issues/54#issuecomment-1383828910, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQO2VQWBBHZYNXNZN4DF53DWSUPITANCNFSM6AAAAAAT34HWMM . You are receiving this because you were mentioned.Message ID: @.***>

M0les commented 1 year ago

I've updated the PR to use the logic above. The code's still duplicated between the install-driver.sh and edit-options.sh scripts, but that's another issue.

morrownr commented 1 year ago

I merged a patch from @M0les this morning.

I guess I need to set EDITOR="edlin" now. Seriously, if you see any improvements that can be made, please bring it on.

Jibun-no-Kage commented 1 year ago

I will sit down and do a serious review in the next couple of days. Nice try baiting with 'edlin', I AM NOT FALLING FOR THAT. LOL

morrownr commented 1 year ago

I've updated the PR to use the logic above. The code's still duplicated between the install-driver.sh and edit-options.sh scripts, but that's another issue.

Miles,

I currently have some duplicated code right now as well but getting things working is top priority. We can clean and optimize later. I'm not big on saving a byte just to save a byte, especially if the code is easier to follow for newbies... there is an educational aspect of this project as well.

morrownr commented 1 year ago

I will sit down and do a serious review in the next couple of days. Nice try baiting with 'edlin', I AM NOT FALLING FOR THAT. LOL

Jibun,

I would appreciate your eyes on this patch. I would like to add a Linux version of edlin to the repo and change the script to detect if it is you and if so, run edlin.

Nick

Jibun-no-Kage commented 1 year ago

I will definitely review the latest. As for edlin in the repo.... [CENSORED] And for that matter.... {CENSORED]. And... The horse you.... [CENSORED]... Your grandmother wears.... [CENSORED].... Monty CENSORED].... Python.... [CENSORED]..

morrownr commented 1 year ago

Miles,

I was doing some testing on something else and decided to do the following:

$ export EDITOR=geany $ echo $EDITOR geany

Yet, when I enter export EDITOR=geany before running ./install-driver.sh I get nano, not geany. I haven't looked at the code again since noting this but should this work? Seems strange that it would not based on the discussion.

Nick

M0les commented 1 year ago

I was doing some testing on something else and decided to do the following:

$ export EDITOR=geany $ echo $EDITOR geany

Yet, when I enter export EDITOR=geany before running ./install-driver.sh I get nano, not geany. I haven't looked at the code again since noting this but should this work? Seems strange that it would not based on the discussion.

Hm, is geany actually installed and on your ${PATH}? Other possibilities: was VISUAL set to nano? (That'll take precedence over EDITOR); Possibly geany is somehow aliased or linked to nano (Pretty unlikely/weird thing to do, though). Certainly the only way to get to nano is if one of the VISUAL/EDITOR variables are set to it or more likely the VISUAL or EDITOR variables don't refer to executables on ${PATH} (or absolute paths or shell functions or internal commands)

A test would be seeing what happens when you try to run ${EDITOR} from the shell. Also try seeing what the predicate command -v "${EDITOR}" gives you when run from the shell (Should print-out something like /usr/bin/geany and return $? = 0, but I presume you'll get something else).

Apart from all that, geany being a GUI editor and the fact these scripts need to be run as root, you may have fun getting the DISPLAY working if you're logged-in as a non-root user. That's pretty out of scope for this change, though.

Jibun-no-Kage commented 1 year ago

HA HA! The compile on 256 MB Pi only took 8,940 seconds! So there! Now testing. Updates to follow.

Memory just before install-driver script run... # free total used free shared buff/cache available Mem: 180512 43916 59152 844 77444 87020 Swap: 102396 256 102140

# lsusb Bus 001 Device 009: ID 09ae:0002 Tripp Lite Any Device (see discussion) Bus 001 Device 011: ID 0bda:a811 Realtek Semiconductor Corp. RTL8811AU 802.11a/b/g/n/ac WLAN Adapter Bus 001 Device 003: ID 0424:ec00 Microchip Technology, Inc. (formerly SMSC) SMSC9512/9514 Fast Ethernet Adapter Bus 001 Device 002: ID 0424:9512 Microchip Technology, Inc. (formerly SMSC) SMC9512/9514 USB Hub Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

# lsusb Bus 001 Device 009: ID 09ae:0002 Tripp Lite Any Device (see discussion) Bus 001 Device 012: ID 0bda:c811 Realtek Semiconductor Corp. 802.11ac NIC Bus 001 Device 003: ID 0424:ec00 Microchip Technology, Inc. (formerly SMSC) SMSC9512/9514 Fast Ethernet Adapter Bus 001 Device 002: ID 0424:9512 Microchip Technology, Inc. (formerly SMSC) SMC9512/9514 USB Hub Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

# ls -l total 192 -rw-r--r-- 1 root root 5883 Jan 17 15:51 8821cu.conf drwxr-xr-x 7 root root 4096 Jan 17 15:51 core -rw-r--r-- 1 root root 183 Jan 17 15:51 dkms.conf -rwxr-xr-x 1 root root 508 Jan 17 15:51 dkms-make.sh drwxr-xr-x 3 root root 4096 Jan 17 15:51 docs -rwxr-xr-x 1 root root 1225 Jan 17 15:51 edit-options.sh drwxr-xr-x 9 root root 4096 Jan 17 15:51 hal -rw-r--r-- 1 root root 2060 Jan 17 15:51 halmac.mk drwxr-xr-x 5 root root 12288 Jan 17 15:51 include -rwxr-xr-x 1 root root 11098 Jan 17 15:51 install-driver.sh -rw-r--r-- 1 root root 104 Jan 17 15:51 Kconfig -rw-r--r-- 1 root root 656 Jan 17 15:51 LICENSE -rw-r--r-- 1 root root 75897 Jan 17 15:51 Makefile drwxr-xr-x 3 root root 4096 Jan 17 15:51 os_dep drwxr-xr-x 2 root root 4096 Jan 17 15:51 platform -rw-r--r-- 1 root root 23989 Jan 17 15:51 README.md -rwxr-xr-x 1 root root 4051 Jan 17 15:51 remove-driver.sh -rw-r--r-- 1 root root 1309 Jan 17 15:51 rtl8821c.mk -rwxr-xr-x 1 root root 638 Jan 17 15:51 save-log.sh -rw-r--r-- 1 root root 1085 Jan 17 15:51 supported-device-IDs

# apt install dkms bc git build-essential raspberrypi-kernel-headers network-manager iw [...] Unpacking raspberrypi-kernel-headers (1:1.20220830-1) ... [...]

# uname -a Linux pink 5.15.61+ #1579 Fri Aug 26 11:08:59 BST 2022 armv6l GNU/Linux

[ Start Time... 1/18/2023, 13:30]

# ./install-driver.sh : --------------------------- : install-driver.sh v20230116 : armv6l (ARCH) : 180512 (SMEM) : 1/1 (sproc/nproc) : 5.15.61+ (KVER) : gcc (Raspbian 10.2.1-6+rpi1) 10.2.1 20210110 : dkms:2.8 : --------------------------- Installing 8821cu.conf to /etc/modprobe.d The dkms installation routines are in use. Copying source files to /usr/src/rtl8821cu-5.12.0.4

Creating symlink /var/lib/dkms/rtl8821cu/5.12.0.4/source -> /usr/src/rtl8821cu-5.12.0.4

DKMS: add completed. The driver was added to dkms successfully.

Kernel preparation unnecessary for this kernel. Skipping...

Building module: cleaning build area... ./dkms-make.sh....

[ Left To Make Lunch.... ] [ ...Checked On DKMS Build Progress... ] [ Let the Dachshunds Out... ] [ ...Checked On DKMS Build Progress... ] [ Watch A Couple Of Adtrain's Basement Videos On Youtube... ] [ Ate Said Lunch...] [ ...Checked On DKMS Build Progress... ] [ Watched the Dachshund Chase Amazon Delivery Person, Fast For A Human.... ] [ ...Checked On DKMS Build Progress... ] [ Took A Nap... ] [ ...Checked On DKMS Build Progress... ] [ Watched the Dust Settle On My Desk... ] [ ...Checked On DKMS Build Progress... ] [ Search HBO, NetFlix, Etc. For Something To Watch... ] [ ...Checked On DKMS Build Progress... ] [ Watched An Adam Savage Tested Youtube Video.... ] [ ...Checked On DKMS Build Progress... ] [ Stood On Weight Scale... Maybe SHould Lose The 10 Pounds I Gained Over The Holidays? ] [ ...Checked On DKMS Build Progress... ] [ Worked On The Outline To My Book... ] [ ...Checked On DKMS Build Progress... ] [ Thought About Some Day Purchasing A Lottery Ticket... ] [ ...Checked On DKMS Build Progress... ] [ Practiced My Hiragana Writing... ] [ ...Checked On DKMS Build Progress... ] [ Worked On My Gimbal Control Platform... ] [ ...Checked On DKMS Build Progress... ] [ Pondered The Nature Of The Universe...] [ ...Checked On DKMS Build Progress... ] [ Played Fetch With Indie... ] [ ...Checked On DKMS Build Progress... ] [ Scratched Nikki Ears...] [ ...Checked On DKMS Build Progress... ] [ Step Into The Tardius...]

cleaning build area...

DKMS: build completed. The driver was built by dkms successfully.

[ 15:49]

8821cu.ko.xz: Running module version sanity check.

depmod.................

DKMS: install completed. The driver was installed by dkms successfully. Do you want to edit the driver options file now? [y/N] y Do you want to reboot now? (recommended) [y/N] y

# cat /proc/meminfo MemTotal: 180512 kB MemFree: 39248 kB MemAvailable: 77352 kB Buffers: 21888 kB Cached: 55312 kB SwapCached: 176 kB Active: 31896 kB Inactive: 61556 kB Active(anon): 828 kB Inactive(anon): 16188 kB Active(file): 31068 kB Inactive(file): 45368 kB Unevictable: 12 kB Mlocked: 12 kB SwapTotal: 102396 kB SwapFree: 101884 kB Dirty: 128 kB Writeback: 0 kB AnonPages: 16104 kB Mapped: 36012 kB Shmem: 764 kB KReclaimable: 13448 kB Slab: 23688 kB SReclaimable: 13448 kB SUnreclaim: 10240 kB KernelStack: 736 kB PageTables: 964 kB NFS_Unstable: 0 kB Bounce: 0 kB WritebackTmp: 0 kB CommitLimit: 192652 kB Committed_AS: 205496 kB VmallocTotal: 835584 kB VmallocUsed: 6284 kB VmallocChunk: 0 kB Percpu: 64 kB CmaTotal: 65536 kB CmaFree: 10028 kB

Compile Time Results... = 0 days 2 hours 29 minutes 0 seconds = 0.103472 days = 2.48333 hours = 149 minutes = 8,940 seconds

# cat /sys/firmware/devicetree/base/model Raspberry Pi Model B Rev 1

Jibun-no-Kage commented 1 year ago

Using network manager, can scan networks, associate to 2.4 and 5.0. So basic testing done. I will do a bit more tonight and tomorrow. I will do a speed/throughput test, but since I have older equipment, I am not going to get much compare to what others can. But for me, sufficient, I have IoT, microcontrollers and such, so I really don't need volume over wireless. I don't stream over wireless.

morrownr commented 1 year ago

Miles,

Hm, is geany actually installed and on your ${PATH}?

It is. What I am going to do is walk through the code and see what is going on. That result was not what I expected so let me play around with it. Thanks for the suggestions.

Nick

morrownr commented 1 year ago

Left To Make Lunch....

For you or the dogs? You did let the dogs go hungry?

Well, this is the editor thread, not the low mem thread so I'll keep it short: I knew you would drag out the old Pi. I'm impressed that it only took 2.5 hours. Would you mind doing a removal followed by a manual dkms install? That could be a bedtime project. You have convined me that the mem issue is good unless we hit extremes like the OP was so now if you can make sure manual dkms install and kernel upgrade triggered installs are what we need to make sure are working.

Nick

Jibun-no-Kage commented 1 year ago

Oh.... my bad, I did post to the wrong thread. Sorry about that. But just to reply to your comment above. I triggered an OS update, and the the DKMS seemed to work fine since the OS update included a kernel update, I happen to have 3 drivers integrated to DKMS, our current target, 8821AU and 8188EU. I need to review the logs to confirm, but it looked like during the update, DKMS built all three.

I will do the remove and re-install as well.

Jibun-no-Kage commented 1 year ago

Reposted to the correct thread. Oh, and the dogs eat in the evening... they don't get lunch. LOL

M0les commented 1 year ago

Hm, is geany actually installed and on your ${PATH}?

It is. What I am going to do is walk through the code and see what is going on. That result was not what I expected so let me play around with it. Thanks for the suggestions.

Interestingly, when I got my XFCE gui box rebuilt, I tried EDITOR=geaney (Note misspelling) and got nano when I ran the edit-options.sh script (which made my toupee flip). Then I noticed and fixed the typo, tried again and it worked as expected.

morrownr commented 1 year ago

Miles,

It was the root thing. A good way to test:

$ sudo env VISUAL=geany ./edit-options.sh $ sudo env EDITOR=vi ./install-driver.sh

Nick

Jibun-no-Kage commented 1 year ago

New song? "I was.... A.... Roooot.... Thang?"

colincdean commented 1 year ago

I'm happy to cast my vote (if I have one) for ${VISUAL} -> ${EDITOR} -> nano -> fail. A variation that would probably work slightly more generally is ${VISUAL} -> ${EDITOR} -> nano -> vi -> fail, as I've never come across a Linux system (or any other Unix-like platform over the last 35 years) without vi(m) installed.

morrownr commented 1 year ago

A variation that would probably work slightly more generally is ${VISUAL} -> ${EDITOR} -> nano -> vi -> fail

I have no problem with this variant either if @M0les wants to take a look at it.

I merged some patches yesterday and one of them makes some slight changes to how the editor patch is handled. The DEFAULT_EDITOR line has a change. I pondered whether the setting should be in the installation script and decided to try something different... 

DEFAULT_EDITOR=`cat default-editor`

I created a text file called default-editor and put nano inside.

The rest of the editor support:

# Try to find the user's default text editor through ${VISUAL}, ${EDITOR} or nano
if command -v "${VISUAL}" >/dev/null 2>&1
then
        TEXT_EDITOR="${VISUAL}"
elif command -v "${EDITOR}" >/dev/null 2>&1
then
        TEXT_EDITOR="${EDITOR}"
elif command -v "${DEFAULT_EDITOR}" >/dev/null 2>&1
then
        TEXT_EDITOR="${DEFAULT_EDITOR}"
else
        echo "No text editor found (default: ${DEFAULT_EDITOR})."
        echo "Please install ${DEFAULT_EDITOR} or edit the file 'default-editor' to specify your editor."
        echo "Once complete, please run \"sudo ./${SCRIPT_NAME}\""
        exit 1
fi

So, everything how Miles submitted it with the exception of where the default editor name is stored. Tell me what you think. I can revert if this is a bad idea.

Nick

M0les commented 1 year ago

Right, so basically the change is from

DEFAULT_EDITOR="nano"

to:

DEFAULT_EDITOR=`cat default-editor`

I suppose there are two things to consider:

  1. Whether to make this change: I suggest not to, because the current way it's in keeping with the other constants defined at the top of the scripts (e.g. SCRIPT_NAME, DRV_VERSION, etc.) I don't hold a very strong opinion about this, though. If you think having the default editor in a single-setting config file is better, go for it - I just think it actually complicates things (i.e. "keep your config constants together, preferably at the top of the code that uses them").
  2. How to make the change (style/syntax): I'm a bit more opinionated about renaming the default-editor file default-editor.txt (Because it's just a plain text file with no special formatting or executability). I'd also recommend using the inbuilt Bash file inlining: DEFAULT_EDITOR=$(<default-editor.txt) - as the shebang line makes this a Bash-specific script. The cat program is supposed to be for concatenating multiple files together and script pedants will harass you if you're just piping a single file somewhere (Also: backticks).
morrownr commented 1 year ago

Miles,

I have read and understand your post. I am using the experience I have gained over the last few years while working with many users that are newbies. While I certainly do not get everything right, my guiding light is to think about how any change will impact the numbers of issues coming from newbies and I have to consider that because of the time required to help them. Thoughts:

I suggest not to, because the current way it's in keeping with the other constants defined at the top of the scripts (e.g. SCRIPT_NAME, DRV_VERSION, etc.)

The other constants are there for those providing tech support but DEFAULT_EDITOR is there for the user and it seems to me that allowing a simply way for newbies or anyone for that matter to change the default would be beneficial. Having them edit the line in install-driver.sh is inviting them to mess around and mess things up or so my thinking goes. Maybe I am way off base here and please come back and tell me I am if I am. I also would like to hear the thoughts of others.

    echo "Please install one and set the VISUAL or EDITOR variables to point to it."

That line is what sent me down this path. I see that as not being newbie friendly. Let's work together and find a solution here that is newbie friendly.

I'm a bit more opinionated about renaming the default-editor file default-editor.txt (Because it's just a plain text file with no special formatting or executability). I'd also recommend using the inbuilt Bash file inlining: DEFAULT_EDITOR=$(<default-editor.txt)

I have no problems with and thank you for both of those ideas.

Additional related items:

Colin mentioned possibly going to a ${VISUAL} -> ${EDITOR} -> nano -> vi -> fail solution. I have no problems with that if there is a reasonable way to implement it.

I had a user the other day try to start install-driver.sh like:

$ sudo sh install-driver.sh

Since I use bash specific stuff, it blew up. I thought about getting Jibun to tell the guy to "RTFM" but that is not a kind thing to do so... do you and anyone else here have a cleaver bit of code to check for users trying to run sh so that the script can exit and scold them (and tell them what they should do?)... so that I do not lose time replying to issues like this?

Thanks,

Nick

Jibun-no-Kage commented 1 year ago

Is not $0 the entire command line executed? You could just parse that for anything not 'bash' after sudo and before install-driver.sh, or something akin to that? And use BASH_SOURCE to validate?

https://stackoverflow.com/questions/35006457/choosing-between-0-and-bash-source

If BASH_SOURCE is null they did not use bash... You might want to check the default shell setting as well? 'echo $SHELL' type of thing...

#!/bin/bash # #

echo "${0}" echo "$BASH_SOURCE" echo $SHELL



# bash ./IExecuteMe.sh
./IExecuteMe.sh
./IExecuteMe.sh
/bin/bash

# sh ./IExecuteMe.sh
./IExecuteMe.sh

/bin/bash

# ./IExecuteMe.sh
./IExecuteMe.sh
./IExecuteMe.sh
/bin/bash
Jibun-no-Kage commented 1 year ago

Oh.... I forgot the flashing red letters in my example, dang it!!!

Jibun-no-Kage commented 1 year ago

Last, had to look this up.... if you REALLY wanted to go nuts, you could find the process PID, then query '/proc/$$/cmdline' where $$ is the PID, then you could REALLY see what they did. LOL I once used something like this a long time ago to keep people from assuming stuff.

People don't read, even proof readers don't read... Years ago I put in a technical document "And Mickey Mouse At Step X Clicked The Mouse Here." Get this, that Mickey Mouse comment made it through 3 different people that SWORE they read my documentation and it was good to be published. So I waited until the last second and then called it out... All 3 of the proof readers/editors had some explaining to do!

morrownr commented 1 year ago

@M0les

My bad for merging something into your work before saying something. I apologize. If you could give it another look based on Colin's input and my thoughts, I would appreciate it. I can revert my changes if you wish.

@Jibun-no-Kage

Thanks for the ideas. I will see what I can come up with and then you can fix it.

Nick

Jibun-no-Kage commented 1 year ago

Fix or break it?

morrownr commented 1 year ago

Fix or break it?

I'll break it, you fix it!

Come to think of it, I probably ought to put up a new Status message. I think we are still on track for a 2023-02-02 release unless somebody sees something I am not seeing. Here we are working on what editor should be used and how take care of those who don't read the README. No real bug reports with the driver code seen for a while.

Jibun-no-Kage commented 1 year ago

Are you kidding me... I just got this notice via electronic mail...

Due to project overruns, unexpected sourcing delays from China, and various other supply side issues, the availability of 'Flashing Red Letters' is not believed to be available within the expected timeline for release. Please accept our sincere regrets regarding this difficult time. We are making every effort to proactively address this short fall in components, that we once believe we had in sufficient quality for your project. We hope this difficult situation will not impact our future efforts and opportunities to continue to supply you with our elegant and exclusive product line of stylized lighted typeface glyph objects.

Thank you for your understanding and continued future business.

Sincerely, The Management

M0les commented 1 year ago

... Maybe I am way off base here and please come back and tell me I am if I am. I also would like to hear the thoughts of others.

Well that's why I'm not very strongly advocating for changing it. It's true that cotton-balling user (mis-)behaviour can lead to higher code complexity. But given that the longest script's only about 400 lines, it's not really a big concern.

    echo "Please install one and set the VISUAL or EDITOR variables to point to it."

That line is what sent me down this path. I see that as not being newbie friendly. Let's work together and find a solution here that is newbie friendly.

It is a bit of a mouthful.

... Colin mentioned possibly going to a ${VISUAL} -> ${EDITOR} -> nano -> vi -> fail solution. I have no problems with that if there is a reasonable way to implement it.

I think I proposed something like that early on too. I can revisit.

$ sudo sh install-driver.sh

Since I use bash specific stuff, it blew up. I thought about getting Jibun to tell the guy to "RTFM" but that is not a kind thing to do so... do you and anyone else here have a cleaver bit of code to check for users trying to run sh so that the script can exit and scold them (and tell them what they should do?)... so that I do not lose time replying to issues like this?

Yeesh! For one of those rare systems that have bash, but don't just symlink /bin/sh to it. I have some thoughts for this, but I'll address outline them in a later quote comment.

M0les commented 1 year ago

Is not $0 the entire command line executed?...

No, that's the first parameter to the shell executable, i.e. the name of the script you're running. The rest of the parameters are in the $@ array.

Why not just test what ${SHELL} is?

Simple "fail-out" method:

if [[ bash != "$(basename "${SHELL}")" ]]; then
        echo "Sorry, this script needs to be run in the \"bash\" shell environment (not \"${SHELL}\")"
        exit 1
fi
# Fall-through to script body

NOTE: The double-double-quoting in "$(basename "${SHELL}")" to protect if ${SHELL} is not set (Usually in /etc/bashrc or equivalents for other shells)

More complex "recall in bash" method:

if [[ bash != "$(basename "${SHELL}")" ]]; then
        SHELL=$(which bash 2>/dev/null)
        ${SHELL} $0 $@
        exit 0
fi
# Fall-through to script body

NOTE: The command builtin is bash-specific, so using which as we know this code is not running inside bash at this time.

Most complex "recall in bash if it's installed otherwise fail"

if [[ bash != "$(basename "${SHELL}")" ]]; then
        SHELL=$(which bash 2>/dev/null)
        if [[ 0 -ne $? ]]; then
                echo "This script needs the \"bash\" shell environment to run"
                exit 1
        fi
        ${SHELL} $0 $@
        exit 0
fi
# Fall-through to script body
Jibun-no-Kage commented 1 year ago

The key question that users always ask.... when they get an error message is, "So Now What!!!??" So error messages must at least in some way provide direction. In our case this should be the read-me of course. But saying this, we don't have to be exhaustive just directional if you will. In the case of an editor, we can simply say in the read me that this solution requires editing of the driver option file. And we recommend using editor 'nano' or whatever. and we suggest they install nano or whatever. This achieves 3 things, 1) we address correct action on error result, 2) we encourage reading of the documentation, and 3) we should only get questions from users that have attempted 1 and 2 to a reasonable extent. In general this works, or so has been my experience.

Documentation for applications, etc. usually only explains WHY you need to do X OR.... only explain HOW you DO X THEN Y THEN Z. This is an issue even in Fortunate 50 companies that train or promote extensive knowledge base systems that qualify ONLY HOW-TO, and never WHY-DO, or the reverse. I spent years making this point to management 100s of times.... basically every time I had new management above me. Fortunately, 99% of the time, they actually listened. And as a result the documentation I signed off on, always included WHY YOU DO X and then HOW YOU DO X. Over time I achieved a smarter user population over time.

I truly believe if we do this, both WHY-DO and HOW-TO-DO, in brief at a minimum, we WILL avoid some of the hassle of users tripping over simplistic issues, to the greatest degree possible. That said, you can't fix stupid, so there is always a small minority, that is difficult, but such is life of a developer/publisher.

M0les commented 1 year ago

I'm happy to cast my vote (if I have one) for ${VISUAL} -> ${EDITOR} -> nano -> fail. A variation that would probably work slightly more generally is ${VISUAL} -> ${EDITOR} -> nano -> vi -> fail, as I've never come across a Linux system (or any other Unix-like platform over the last 35 years) without vi(m) installed.

That's my experience too. Possible excursions to Emacs along the way.

M0les commented 1 year ago

@morrownr

My bad for merging something into your work before saying something. I apologize. If you could give it another look based on Colin's input and my thoughts, I would appreciate it. I can revert my changes if you wish.

I'm used to a feature-branch development model (i.e. Submit PRs for review/correction before merge). I'm not particularly worried about what you do with the main branch, though - I think that's up to those with more experience in the code base to worry about.

M0les commented 1 year ago

New PR covers lots of stuff talked about above.

morrownr commented 1 year ago

@M0les

I write this a big smile on face as I know I have done roughly the same thing many times in my life...

Why not just test what ${SHELL} is?

Simple "fail-out" method:
if [[ bash != "$(basename "${SHELL}")" ]]; then
        echo "Sorry, this script needs to be run in the \"bash\" shell environment (not \"${SHELL}\")"
        exit 1
fi
# Fall-through to script body

So, I decided to give the above a workout to see if it accomplishes the mission. Result:

$ sudo sh install-driver.sh install-driver.sh: 46: [[: not found

Line 46 is:

if [[ bash != "$(basename "${SHELL}")" ]]; then

[[ is bash specific.

Jibun, before you jump all over this... my bet is that you have done similar things before so go back to testing the driver on your IBM PC with 64k of ram. <g>

Nick

Jibun-no-Kage commented 1 year ago

Use "-z" or "-n" maybe?

https://stackoverflow.com/questions/10849297/compare-a-string-using-sh-shell

Jibun-no-Kage commented 1 year ago

Oh, and my first IBM PC XT had 256K. Got to love ISA expansion cards.

morrownr commented 1 year ago

Use "-z" or "-n" maybe?

https://stackoverflow.com/questions/10849297/compare-a-string-using-sh-shell

I think I am explaining the problem wrong. Here is the deal:

User starts installation script as follows:

$ sudo sh install-driver.sh

Since install-driver.sh contains some bash specific code, sh complains and the installation goes south. bash is a superset of sh. "[[" is part of bash but not part of sh, thus the error. The test routine needs to work with sh, it can't have anything bash specific.

The best logic as far as I can tell is: if user used sudo sh or sh to run the script, then exit with message telling him/her what to do.

Nick