adjust ExtractResources.sh to be posix-compliant

[Thorsten42]

The ExtractResources.sh script fails when used with sh since bash specific parts are used. (I closed my previous pull request to just use bash instead of sh since an sh compatible version would be preferred #491)

In this pull request the regex for checking the "number of threads for mmap" input is adjusted. Moreover, the previous version (^[1-9+]$) was also rejecting 10 cores:

➜ bash ExtractResources.sh

Welcome to helper script to extract required dataz for MaNGOS!
Should all dataz (dbc, maps, vmaps and mmaps) be extracted? (y/n)
y
How many CPU threads should be used for extracting mmaps? (leave empty to use all available threads)
10
Only numbers are allowed!

However, the parts using ${PIPESTATUS[0]} have been removed since other pipes are also not checked and sh compatible workarounds would definitely worsen the readability.

For me the script works like this with sh but I am not a shellscript guru. So please take a close look at it.

[Dwyriel]

Just ran into this issue a few minutes ago. Even using bash it still failed (probably because I didn't remove the #!/bin/sh :d).

I replaced exit_code="${PIPESTATUS[0]}" with a simple exit_code=$? and it worked no problem. Not sure how portable this is though, but I'm pretty sure it'd work in any unix-like system.

If this works for every system this script is supposed to run on, then I think it's a better solution than forcing the use of bash.

ps: $? is supposed to be the return value of the last command, so works well in this case, but it's definitely not very readable, though a simple comment explaining it could address this issue, and I'm pretty sure it's posix compliant.

[Muehe]

@Dwyriel Thanks for the suggestion, that's also the solution we came up with when discussing it on Discord recently, somebody™ just needs to commit it. We found a major memory leak in the mmap generator during that though, so got a bit sidetracked. 😅

And yes, $? is POSIX-compliant, as per: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_05_02

For reference, here is the patch I came up with, combining Thorsten's changes and your suggestion for the exit code:

diff --git a/contrib/extractor_scripts/ExtractResources.sh b/contrib/extractor_scripts/ExtractResources.sh
index dcc17997a1..a72f110d7b 100755
--- a/contrib/extractor_scripts/ExtractResources.sh
+++ b/contrib/extractor_scripts/ExtractResources.sh
@@ -144,8 +144,8 @@ then
   echo "How many CPU threads should be used for extracting mmaps? (leave empty to use all available threads)"
   read line
   echo
-  if [[ ! -z $line ]]; then
-    if [[ $line =~ ^[1-9+]$ ]]; then
+  if [ ! -z $line ]; then
+    if [ "$(expr "$line" : "^[1-9][0-9]*$")" -gt 0 ]; then
       NUM_THREAD=$line
     else
       echo "Only numbers are allowed!"
@@ -252,8 +252,8 @@ if [ "$USE_VMAPS" = "1" ]
 then
   echo "$(date): Start extraction of vmaps..." | tee -a $LOG_FILE
   $PREFIX/vmap_extractor $VMAP_RES $VMAP_OPT_RES | tee -a $DETAIL_LOG_FILE
-  exit_code="${PIPESTATUS[0]}"
-  if [[ "$exit_code" -ne "0" ]]; then
+  exit_code="$?"
+  if [ "$exit_code" -ne "0" ]; then
     echo "$(date): Extraction of vmaps failed with errors. Aborting extraction. See the log file for more details."
     exit "$exit_code"
   fi
@@ -264,8 +264,8 @@ then
   fi
   echo "$(date): Start assembling of vmaps..." | tee -a $LOG_FILE
   $PREFIX/vmap_assembler ${OUTPUT_PATH:-.}/Buildings ${OUTPUT_PATH:-.}/vmaps | tee -a $DETAIL_LOG_FILE
-  exit_code="${PIPESTATUS[0]}"
-  if [[ "$exit_code" -ne "0" ]]; then
+  exit_code="$?"
+  if [ "$exit_code" -ne "0" ]; then
     echo "$(date): Assembling of vmaps failed with errors. Aborting extraction. See the log file for more details."
     exit "$exit_code"
   fi

[Muehe]

Oh by the way, you can check if a script is compliant with the shellcheck program, either installed locally or via pasting the script on https://www.shellcheck.net/

[Dwyriel]

@Muehe hey neat, hope somebody™ commits it soon them :D

Also, thanks for pointing out about shellcheck, didn't know about it. Ran the script through it and it had a few warnings mentioning the PIPESTATUS and a few other things as well, pretty useful.

Cheers~ ^^

[Muehe]

Merged as ca1526a

Since this included some other changes I made a new commit, but put you as the author @Thorsten42

So this PR won't show as merged here on Github, but it is. Thanks for your input and sorry for the long wait.

[evil-at-wow]

I replaced exit_code="${PIPESTATUS[0]}" with a simple exit_code=$? and it worked no problem. Not sure how portable this is though, but I'm pretty sure it'd work in any unix-like system.

It's going to work, but it does the wrong thing. That $? gives the exit status of the last command, which in the case of a pipe is the exit status of the last command. So that would check the exit status of tee here, not the actual vmap_extractor/vmap_assembler command as originally intended.

[Dwyriel]

@evil-at-wow that is true yes... did a bit more digging trying to find a posix way to do so, and it seems there's a way that'd make it work.

Simply setting pipefail (set -o pipefail) would do the trick, it's also part of posix and most shells support it. so there's no reason not to use it and keep the exit_code="$?" as is.

That said, I'm not sure how exactly pipefail would interact with stdout and if tee would still execute or not, even if the piped program failed, so might not have the desired functionality. (I'm not very knowledgeable about posix or script stuff, sowwy >.<)

[Muehe]

Oh damn, thanks for pointing @evil-at-wow . However this same problem should occur with ${PIPESTATUS[0]} if I'm not mistaken?

Seems this can be solved with moving the commands a bit and adding an eval though.

[Muehe]

Hmm, no that doesn't work quite right:

$ echo $status
0
$ eval ` { ls /dontexists || echo status="$?"; } | tee test.txt`
ls: cannot access '/dontexists': No such file or directory
$ cat test.txt 
status=2

Maybe ls prints to STDERR instead of STDOUT?

[Dwyriel]

@Muehe wrote a small cpp program that prints to stdout and returns 3 and did some testing with both eval and pipefail and those are the result:

Eval prints "status=3" and writes both the stdout from the program plus the echo ("status=3") to the output file.

Pipefail has the correct output both in the terminal and in the file, as it does not stop the piping, just doesn't overwrite $? if the return type of any piped program was not 0.

script I used for testing: (eval one is the same as the linked but, changing ls to be my own program)

set -o pipefail
/home/lotie/Downloads/a.out | tee ./stuff.txt
echo "$?" #output 3 

revolutionary cpp program:

std::cout << "some stuff" << std::endl;
return 3;

Edit: if I print to std::cerr (stderr) tee does not pick it up, so yes ls is most likely writing to stderr, redirecting stderr to stdout would be a solution if you also want that output.

[Muehe]

@Dwyriel Since the stackexchange link you posted mentions that it might take up to a decade for this change to proliferate to all shells I installed dash to check (v0.5.12-1), and the option is indeed still missing there:

$ set -o pipefail
dash: 2: set: Illegal option -o pipefail

One alternative solution would be an output redirection:

command >> logfile 2>?1; exit_code="$?"

The downside being that it would prevent the command outputs from being shown during the script execution.

Another would be saving the exit code to a temporary file:

{ command; echo $? > exit_code.tmp; } | tee -a logfile; status="$(cat exit_code.tmp)"; rm exit_code.tmp

Or we could just completely remove the check as Thorsten initially suggested.

[Dwyriel]

@Muehe Well, found another solution that works well, but requires some cleanup after use, namely: named pipes

mkfifo named_pipe
tee -a stuff.txt < named_pipe &
./a1.out > named_pipe 2>&1 #redirecting stderr is optional
echo $?

tee -a stuff.txt < named_pipe &
./a2.out > named_pipe 2>&1
echo $?
rm named_pipe #remove named pipe after use

output:

$ ./t.sh
some stuff
some error
3
some stuff2
some error2
4

file

some stuff
some error
some stuff2
some error2

Ran in every shell I tried (sh, bash, dash).
No idea if it's really all that portable or not, it looks cleaner and it's more readable, but could be a point of failure if there's some obscure shell out there that does not support it.

I don't think removing the check is the best idea as there would be no feedback in case something went wrong, so the user might think the vmaps were extracted and assembled correctly when they weren't.

Both saving the exit code to a temp file like you said and named pipes had the same result for me, so imo those are the best solutions so far.

[Muehe]

Interesting. I also found mktemp command in the meantime, which can be used to create temporary files with a randomised name. Also have to be manually deleted though, unless you want to rely on system settings.

I'll play around with both approaches a bit, but intuitively the file approach seems to be a bit more readable IMHO, since it uses less output redirection.

[Dwyriel]

Ok so... while messing around with the actual script to see what works and what doesn't (and how well it works), I found another problem kind of related kind of not..

When I was getting to the vmap_extractor part, it seemed like it was just hanging, even when I used a non-modified script, and it indeed was. I ran it without piping it to tee and vmap_extractor would print a few things to the console then wait for user input to close itself. this is the output I was getting:

Fri Sep 22 02:19:12 AM -03 2023: Start extraction of vmaps...
Your output directory seems to be polluted, please use an empty directory!
<press return to exit>

When piping to tee it'd just stay stuck at the Fri Sep 22 02:19:12 AM -03 2023: Start extraction of vmaps... part until I'd either interrupt the execution or "randomly" pressed enter, as it was waiting for user input.

I'm not very familiar with tee, but I don't think using tee with interactive programs is a good idea. I tried looking for a way to make it work and the only way I found was to run tee then pipe it into the program you want then pipe that into tee again, but it didn't work at all.

So, putting that vmap_extractor (and possibly ad and vmap_assembler as well) may be interactive, either those programs need some reworking, or this script. Unless there's an actual way to do this properly and I'm just not aware.

A possible workaround might be the use of script(1):

script -c "command goes here" log_file.txt

How it'd be incorporated in the script I'm not sure, might even require a second .sh that has the commands you wanna run, and it'd probably need some cleanup of the logfile as well as script writes some extra stuff at the beginning and end of the file.

[insunaa]

the output directory pollution should generally be noticed before vmaps extraction starts, so this is indeed a strange situation

[Muehe]

@Dwyriel This is because the program isn't running in it's own shell, so printf isn't automatically flushing to stdout. Can be fixed like this:

diff --git a/contrib/vmap_extractor/vmapextract/vmapexport.cpp b/contrib/vmap_extractor/vmapextract/vmapexport.cpp
index c265a59923..0d8272c627 100644
--- a/contrib/vmap_extractor/vmapextract/vmapexport.cpp
+++ b/contrib/vmap_extractor/vmapextract/vmapexport.cpp
@@ -478,6 +478,7 @@ int main(int argc, char** argv)
         {
             printf("Your output directory seems to be polluted, please use an empty directory!\n");
             printf("<press return to exit>");
+            fflush(stdout);
             char garbage[2];
             scanf("%c", garbage);
             return 1;

Curiously this provides a test case for the conditions checking the exit code, the script indeed thinks it finished right now although it didn't. Also not sure where that mkdir line after originates from...

Fr 22. Sep 16:08:38 CEST 2023: Start extraction of vmaps...
Your output directory seems to be polluted, please use an empty directory!
<press return to exit>
Fr 22. Sep 16:08:50 CEST 2023: Extracting of vmaps finished
mkdir: cannot create directory ‘./maps2/vmaps’: File exists

[Muehe]

Oh hell, the line comes from here:

  if [ "$exit_code" -ne "0" ]; then
    echo "$(date): Extraction of vmaps failed with errors. Aborting extraction. See the log file for more details."
    exit "$exit_code"
  fi
  echo "$(date): Extracting of vmaps finished" | tee -a $LOG_FILE
  if [ ! -d "$(pwd)/vmaps" ]
  then
    mkdir ${OUTPUT_PATH:-.}/vmaps
  fi

The problem is I started the script with parameters for input and output path, but the condition only checks if the directory exists in pwd instead of the output directory I specified. 🙈

[Dwyriel]

@Muehe unless this breaks some intended functionality in other platforms, wouldn't it be best if a program like vmap_extractor ran without any sort of interactions? Printing what went wrong and returning with a failure code would allow the script to take care of the rest. And also because the interaction output ("<press return to exit>") would be saved to the log file as well, which might be a bit unorthodox.

ps: Now that I'm re-reading my last message I can see it might have sounded a bit rude or condescending. Sorry about that, it was really late and my brain was barely functional.

[Muehe]

unless this breaks some intended functionality in other platforms, wouldn't it be best if a program like vmap_extractor ran without any sort of interactions?

Yes, very much so, and thus I'll probably just remove the scanf from there. And add a way around the interactive parts of the script itself when started with the a option. Otherwise this script can't be used automatically at all.

Now that I'm re-reading my last message I can see it might have sounded a bit rude or condescending.

Don't worry, it didn't. Well to me at least. 😅

Small progress report, I tried both the mktemp and mkfifo approaches, but neither worked... until I noticed the set -e in line 2 and removed it. 🙈 For reference:

When this option is on, when any command fails (for any of the reasons listed in Section 2.8.1, Consequences of Shell Errors or by returning an exit status greater than zero), the shell immediately shall exit, as if by executing the exit special built-in utility with no arguments, with the following exceptions:

1. The failure of any individual command in a multi-command pipeline shall not cause the shell to exit. Only the failure of the pipeline itself shall be considered.

I think we were hitting that exception in 1. because we are using a command | tee pipeline, so the script didn't exit, but it still messed with expected behaviour further down-script. Had me quite confused for the last hour. :D

Another thing I noticed is that log files are not overwritten/appended if they already exist, so will have to look into that as well.

[Dwyriel]

Found another inconsistency/problem with the most recent change:

mangos-classic/contrib/extractor_scripts/ExtractResources.sh

Line 148 in ca1526a

if [ ! -z $line ]; then

if the user typed something containing whitespaces, the script will lay them out as multiple params rather than only one, meaning that if I typed 2 2, this line would be interpreted as if [ ! -z 2 2 ]; then rather than if [ ! -z "2 2" ]; then which throws an error and proceeds as if I hadn't typed anything rather than going to the next line to check if what I typed is valid or not.

$line should be surrounded by quotes to make it check the whole thing if [ ! -z "$line" ]; then

[Muehe]

Yeah I actually went through all occurrences of SC2086 and changed them. It's pertinent because many of the variables are paths, which might contain spaces. Just need to do it for the other script as well before I push it.

Ignored the occurrences of SC2162 (read without -r can mangle backslashes) though, since it seemed irrelevant for our usage.

[Muehe]

@Dwyriel Pushed all the changes. Let me know if you find anything else. 👍

[Dwyriel]

@Muehe well what do you know.. :d

So, something quite weird is happening, two in fact, and I was not able to pinpoint what is causing it. This is the error I'm getting:

Fri Sep 22 09:44:04 PM -03 2023: Start extraction of vmaps...
Extract for VMAPs05.
./vmap_extractor [-?][-s][-l][-d <path>]
   -s : (default) small size (data size optimization), ~500MB less vmap data.
   -l : large size, ~500MB more vmap data. (might contain more details)
   -d <path>: Path to the vector data source folder.
   -o <path>: Path to the output folder.
   -? : This message.
Fri Sep 22 09:44:04 PM -03 2023: Extraction of vmaps failed with errors. Aborting extraction. See the log file for more details.

Was running the script from within the proper wow folder, I ran it both dry (no args) and with ./ ./ext as the args.

That said.. whatever the problem is might not be related to vmap_extractor at all..
in both cases the executed command was:

./vmap_extractor -l                         #no args
./vmap_extractor -l -d .//Data -o ./ext     #args: ./ ./ext
./vmap_extractor -l -d .//Data -o ./        #args: ./ ./

so the command itself seems fine I think (maybe not the first one?), what isn't fine is that when I was passing an output dir, the previous parts of the script were still outputting to ./ and not to ./ext (clustering my wow folder). And of course, the second weird thing is how even when running it completely dry, or even by manually passing ./ ./ as args, it still failed with the same error.

I did not have much time today to really look into it deeply, and tomorrow will be an even busier day for me, but I did try to check the paths it was passing to the prev commands as well and they seemed to be fine, so I really have no idea what is going wrong.

Script is fresh from a recompilation I did earlier after pulling the latest changes, no modifications.

Edit: If I tell the script I do not want high res maps or if I just pass the a arg then ad also shows the same behavior, printing out the help message and returning.

[Dwyriel]

Figured it out, the problem is the overuse of quotes for some variables.

Some of the variables prepare arguments for other commands, but because they're being used inside quotes they are passed as a singular argument to those programs rather than being laid out as they should. For example, this is how vmap_extractor was been run:

./vmap_extractor "-l" "-d .//Data -o ./"

But the intended command would be:

./vmap_extractor "-l" "-d" ".//Data" "-o" "./" #the double / doesn't matter afaik

Meaning the whitespaces are required to be there otherwise the program won't know how to interpret some of those arguments.

This is a commit I made with the required changes for it to "run" eaa38a9, but there's still another problem that needs solving: If the client or output folder contain spaces, the commands won't execute as we'd encounter the same problem, so variables like AD_OPT_RES and VMAP_OPT_RES will need to be reworked to properly work with folders with any name.

Another issue I encountered is that the script does not check if ad failed or not, same goes for MoveMapGen, so it keeps executing when it most likely shouldn't. (pretty sure this was a problem even before removing set -e as all those commands are piped)

Edit: A possible easy "solution" to the first problem would be making it less dynamic, requiring it to be run inside the client folder and outputting to a specific, hard coded, folder. I don't really consider this a solution, but it's a lot less work. Docs would also need some updating, so there's also that.

[Muehe]

Merged eaa38a9 thank you! ❤️

In my defence I did try running the script before pushing my fixes and it #worked-on-my-machine. 🙈

[Dwyriel]

@Muehe Created a new PR #519 containing the fixes to the 2 other problems I mentioned, and a few extra things, though I did not add any checks to MoveMapGen, as I'm not sure it's needed or not.

Do you think the script should check the exit code of MoveMapGen? And if possible, could you test those changes to make sure it's working as intended?

[Muehe]

@Dwyriel I'll try to get it merged tomorrow or over the weekend, but at a glance the changes look good. Not sure about MoveMapGen either, will have to look into it.

[Dwyriel]

@Muehe alrighty, I didn't want to touch MoveMapGen (yet) because it was actually giving me some trouble, but not really related to the script, but the system. Don't know why but after running for a while the system would freeze up and, after a few secs, kill the process. My guess is a memory leak somewhere.

Either way I wanted to investigate the root cause to make sure what was going wrong before making any changes I wouldn't be able to test.

[Muehe]

Yeah, that's the memory leak I mentioned further up thread. Bit over my head though, I believe @cyberium and @insunaa have looked into this, but not sure if they made any progress yet. Extraction should still work with 32GB+swap/pagefile though. I only got 16GB+swap so I run into the same problem at the moment.

[Muehe]

@Dwyriel We found a fix for the memory leak, see here. It's unclear if it will be applied like this, because there is ongoing work on the mmap code and this is probably not the root cause, but can be used as a stop-gap solution for testing the scripts.

[Dwyriel]

@Muehe What a difference it made, total system mem usage never went above 2gb :D

Made the changes and tested them, still haven't merged with the PR branch, have a look a it and let me know what you think db0f54e. If everything looks good I'll merge this commit into the PR branch.

ps: speaking of PR branches, I realized the source branch of my previous PR (#518) was the master (where I made additional changes not related to this) and not the correct branch, so I closed it and opened a new one (#519). My bad there >.<