-
-
Notifications
You must be signed in to change notification settings - Fork 821
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
Feature/Steam Workshop support #4186
base: develop
Are you sure you want to change the base?
Feature/Steam Workshop support #4186
Conversation
…inuxGSM into feature/workshop-support
…inuxGSM into feature/workshop-support
if [[ "${serverresp}" =~ \"hcontent_file\":[[:space:]]*([^,]*) ]]; then | ||
remupd="${BASH_REMATCH[1]}" | ||
fi | ||
echo "${remupd}" | tr -d '"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of loading up tr(1) and using a subshell (via pipe) for this, it's far more efficient to use BASH's parameter expansion's pattern substitution, as it's already in the language and it's a very small amount of data.
echo "${remupd//\"/}"
Two initial '/' indicate a greedy match, while one is non-greedy.
This feature is available from at least BASH 3.0, including the option to omit the final '/' if no replacement string is provided.
# therefore, we have to separate the end of the filename to only lowercase it rather than the whole line | ||
# Gather parent dir, filename lowercase filename, and set lowercase destination name | ||
latestparentdir=$(dirname "${src}") | ||
latestfilelc=$(basename "${src}" | tr '[:upper:]' '[:lower:]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use BASH's parameter expansion to change the first or all letters in the variable to lower- or uppercase, without the need for calling other programs, which is more efficient, depending on the amount of data handled.
For example:
SomeVar='test string'
printf '%s\n' "${SomeVar^^}"
SomeVar='test string'
printf '%s\n' "${SomeVar,,}"
Two '^' or ',' indicate a greedy conversion to upper- or lowercase, respectively, while one of these characters changes just the first character, akin to Perl's uc()
vs. uc_first()
, if you're familiar with that.
This feature is available in >= BASH 4.0.
if [ ! -d "${workshopmodsdldir}" ]; then | ||
echo -en "creating LinuxGSM Steam Workshop data directory ${workshopmodsdldir}..." | ||
mkdir -p "${workshopmodsdldir}" | ||
exitcode=$? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (L257)) is redundant. Sometimes it's useful to specifically catch $?
, but this is mainly when you're testing for something other than just 0 or non-0, in my experience.
In this case, it's easier and more practical to directly act upon the exit status, rather than separately store the status, check the value, then act based on the value, especially if you're using [
(inefficient). The if
statement in shell programming essentially does this for us.
For example:
if mkdir -p "${workshopmodsdldir}"; then
...
else
...
fi
While this isolated case isn't a huge deal, a lot of redundant code can be a problem, both to readability and performance.
This is core BASH functionality.
echo -en "creating LinuxGSM Steam Workshop data directory ${workshopmodsdldir}..." | ||
mkdir -p "${workshopmodsdldir}" | ||
exitcode=$? | ||
if [ "${exitcode}" != 0 ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference, -eq
is the numeric comparison operator for equal to in BASH, whether in [
or [[
. The =
or ==
is a string comparison operator for equal to. Admittedly, the difference isn't often too important, but it's worth knowing and worth using the correct syntax to improve readability and avoid anything unexpected.
See CONDITIONAL EXPRESSIONS in bash(1), and note that the ARITHMETIC EVALUATION section is different, for expanding or evaluating arithmetic in, for example, ((...))
, $((...))
, ${NAME[...]}
, etc.
# Counts how many mods were installed. | ||
fn_workshop_count_installed() { | ||
if [ -f "${workshopmodsdir}" ]; then | ||
installedworkshopmodscount=$(ls -l "${workshopmodsdir}" | grep -c ^d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's almost never a good idea to include ls(1) in a script, because it's typically redundant, inefficient, and unreliable. The shell already supports glob filename pattern matching, and counting results is simple.
For example
Count=0
for File in /path/to/files/*; {
(( Count++ ))
}
printf '%d\n' $Count
Or, more concisely and more efficiently, albeit with far less flexibility:
Files=(/path/to/files/*)
printf '%d\n' ${#Files[@]}
This last one is an awesome feature that often gets overlooked.
These features are available in at least >= BASH 3.0.
I'm not sure why you're looking for '^d', so I apologise if I've missed a nuanced and obscure detail. Granted, BASH can effortlessly do what grep(1) is doing here as well.
For example:
Count=0
for File in *; {
if [[ $File == *^d* ]]; then
(( Count++ ))
fi
}
printf '%d\n' $Count
Provided you're looking for '^d' being in filenames. By using these features, that's another 2 programs and a subshell you'd take out of the equation.
# Builds list of installed Steam Workshop mods. | ||
fn_workshop_installed_list() { | ||
fn_workshop_count_installed | ||
for folder in ${workshopmodsdir}/*; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unprotected variable, although there are a few of these. I'm not one of those people who insists on everything being quote-protected, because not everything needs to be, such as integers. However, since it's a directory, one which is presumably changeable by the user or a developer, it's always best to quote-protect it. Unprotected variables like paths can lead to destructive results.
local modid="$1" | ||
local serverresp="$(curl -s -d "itemcount=1&publishedfileids[0]=${modid}" "http://api.steampowered.com/ISteamRemoteStorage/GetPublishedFileDetails/v1")" | ||
local title= | ||
if [[ "${serverresp}" =~ \"title\":[[:space:]]*([^,]*) ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that REGEX in BASH, while an awesome feature, is slower than glob pattern matching. You might be thinking that glob pattern matching is limited, so you need REGEX. I used to think the same. However, [[
uses extended globbing out of the box.
For example:
if [[ "${serverresp}" == *\"title\":*([[:space:]])(*([^,]))* ]]; then
Please test first, in-case I missed something. It's almost 5am. This took much longer than I thought it would.
Refer to extglob
in bash(1) for more information. It's a useful feature which usually needs to be manually enabled with shopt -s extglob
, with the exception of [[
. It's not particularly advanced, but it does cater to most of the common use-cases.
Extended globbing is available in at least >= BASH 3.0, but it being included in [[
is in >= BASH 4.1.
Thanks for the awesome review! I'll apply your suggestions once I have more time. :) |
@FliesWithWind Do you plan to resume your work on this soon ? I would like to add a specific contribution for DayZ Mods which are currently supported only if we rename files with uppercase characters to lowercase. |
This project is alien to me and I might've misinterpreted your goal, so please take this with a grain of salt. You can bulk-rename files to lowercase with: for File in *; do
if [[ -f $File ]]; then
mv "$File" "${File,,}"
fi
done The That would immediately rename all files in the current working directory to their lowercase counterpart. This is a non-recursive approach, by the way. However, this should be done very carefully, perhaps with If you need recursion, you can use the |
@Dr-Shadow at the moment I have my hands full, but I will try to find some time this month to apply suggestions from @terminalforlife. You are more than welcome to contribute. Arma 3 mod files and folders also need to be in lowercase, so it will likely be the same for DayZ. |
891ed8f
to
ca184e3
Compare
Description
For a while, I had a horrible but working script for downloading and updating Arma 3 mods from Steam Workshop. I figured it was about time I try adding it to LinuxGSM. The script was based on https://github.com/arkmanager/ark-server-tools
Changes in this PR are working, but are far from being pretty or complete in my opinion. I'm no bash magician, so I wanted to show it as it is, hoping for an initial review and suggestions.
I've added it as a separate module, to avoid conflicts with the current mods module.
For now, it downloads mods for Arma 3 taken from the server config file.
Once we reach a working and pretty solution, the next steps would be to add support for other games, like Arc and Starboud, and add Steam Collections support.
Fixes #[3514] #[1894] #[1623] #[960]
Type of change
Checklist
PR will not be merged until all steps are complete.
develop
branch as its base.Documentation
Documentation will definitely need an update, once this is ready for merge. :)