MantisBT - Doomseeker
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0003520Doomseeker[All Projects] Bugpublic2018-09-25 00:082018-10-06 18:21
ReporterWubTheCaptain 
Assigned ToBlzut3 
PrioritynormalSeverityminorReproducibilitysometimes
StatusclosedResolutionfixed 
PlatformOSDebian GNU/LinuxOS Versionbuster/sid
Product Version1.2 
Target Version1.2Fixed in Version1.2 
Summary0003520: makesourcepackages.sh: Fragile script doesn't use mktemp(1) or cleanup incomplete build directory, fails and exits
DescriptionFaulty code in create_vcs_info() function of this script doesn't `trap` failure/exit signal or use mktemp(1) to create a temporary directory.

If mktemp(1) was used instead of `mkdir build`, this issue could almost plausibly never happen (but it would leave temporary directories/files anyway if not cleaned).
Steps To ReproduceIn $SRC/releasescripts/ directory:

$ ./makesourcepackages.sh --no-sign
Doomseeker version: 1.2
wadseeker version: 1.2
-- The C compiler identification is GNU 8.2.0
-- The CXX compiler identification is GNU 8.2.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
^C
$ [ -d build ] && echo "build directory exists"
build directory exists
$ ./makesourcepackages.sh --no-sign
Doomseeker version: 1.2
wadseeker version: 1.2
mkdir: cannot create directory ‘build’: File exists
Failed to create temporary build directory.
$ echo $?
1
Additional Information
TagsNo tags attached.
Relationships
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2018-09-25 00:08WubTheCaptainNew Issue
2018-09-25 00:08WubTheCaptainReproducibilityalways => sometimes
2018-09-25 00:09WubTheCaptainNote Added: 0019730
2018-09-25 00:10WubTheCaptainNote Edited: 0019730bug_revision_view_page.php?bugnote_id=19730#r11981
2018-09-25 00:12WubTheCaptainSummaryreleasescripts/makesourcepackage.sh: Fragile script doesn't use mktemp(1) or cleanup incomplete build directory, fails and exits => makesourcepackages.sh: Fragile script doesn't use mktemp(1) or cleanup incomplete build directory, fails and exits
2018-09-25 00:12WubTheCaptainSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=11983#r11983
2018-09-25 00:19WubTheCaptainNote Edited: 0019730bug_revision_view_page.php?bugnote_id=19730#r11984
2018-09-25 00:21WubTheCaptainSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=11985#r11985
2018-09-25 01:23Blzut3Assigned To => Blzut3
2018-09-25 01:23Blzut3Statusnew => assigned
2018-09-25 01:30Blzut3Note Added: 0019737
2018-09-25 01:30Blzut3Statusassigned => needs testing
2018-09-25 01:52WubTheCaptainNote Added: 0019738
2018-09-25 01:52WubTheCaptainStatusneeds testing => needs review
2018-09-25 02:12Blzut3Note Added: 0019741
2018-09-25 02:51WubTheCaptainNote Added: 0019745
2018-09-25 02:51WubTheCaptainStatusneeds review => resolved
2018-09-25 02:51WubTheCaptainFixed in Version => 1.2
2018-09-25 02:51WubTheCaptainResolutionopen => fixed
2018-09-25 02:51WubTheCaptainTarget Version => 1.2
2018-09-25 03:30Blzut3Note Added: 0019749
2018-10-06 18:19WubTheCaptainStatusresolved => closed
2018-10-06 18:21WubTheCaptainNote Added: 0019959

Notes
(0019730)
WubTheCaptain   
2018-09-25 00:09   
(edited on: 2018-09-25 00:19)
PS: fuck bash, this script is terrible anyway. 🙃 (Or kindly, it's not robust enough.)

Give me enough time and motivation and I'll rewrite the whole script (probably in POSIX sh). If not, anyone (myself included) may just patch this issue first.
(0019737)
Blzut3   
2018-09-25 01:30   
A little dramatic there. :P BASH is way better than POSIX shell. My day job actually involves maintaining several thousand line bash scripts which are actually designed for end users. Most notably I actually wrote a command interpreter in bash which was a lot of fun managing IPC between background tasks and doing TUI visualizations. Actually found out that the version of BASH CentOS ships is buggy enough that I had to upgrade it.

Anyway over POSIX shell bash gives you "[[ ]]" which not only is a lot faster than "[ ]/test" but actually tokenizes in the way that a programmer would expect. So that feature alone will make scripts do the right thing more often. POSIX shell also lacks arrays and associative arrays which are a crazy powerful tool for build robust shell scripts.

So yes you might find my modern style for shell scripts a little weird, but it works. These release scripts are only designed to capture what I need to do to perform a release so I never really thought about someone wanting it to cleanup if interrupted. I only focused on the error cases.

In any case it's a one line trap to make it behave better and I suppose there's no reason I can't use mktemp so...

'https://bitbucket.org/Doomseeker/doomseeker/commits/e3f5bac45caadfed624320e2d92b8fae735a3763 [^]'
(0019738)
WubTheCaptain   
2018-09-25 01:52   
This doesn't catch all cases. How about trapping and calling a cleanup() function when $? != 0? The cleanup function can then rm -rf $BuildDir.

Also an obvious ShellCheck warning:

Quote
In makesourcepackages.sh line 29:
trap "kill -INT -$BASHPID" INT
                 ^-- SC2064: Use single quotes, otherwise this expands now rather than when signalled.


(Other ShellCheck warnings ignored.)
(0019741)
Blzut3   
2018-09-25 02:12   
I assume you mean "set -e" or something like that?
set -e

f1() {
    false
}

f2() {
    f1
    echo 'set -e is so useful!'
}

if f2; then
    echo 'it totally catches errors!'
fi


The shell check warning isn't obvious since I really did mean to bake the PID of the topmost shell into there. "kill -INT -PID" means to kill the foreground process of a process group.
(0019745)
WubTheCaptain   
2018-09-25 02:51   
Nevermind. I've talked some crap in my previous note.

Until actual rewrite, I won't bother fixing trivial issues in this ticket.
(0019749)
Blzut3   
2018-09-25 03:30   
I'm still confused as to what you think needs to be rewritten.
(0019959)
WubTheCaptain   
2018-10-06 18:21   
Blzut3: POSIX sh compatibility. 😛