Zandronum Chat on our Discord Server Get the latest version: 3.1
Source Code
My View | View Issues | Change Log | Roadmap | Doomseeker Issue Support Ranking | Rules | My Account
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
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
Attached Files

- Relationships

-  Notes
User avatar (0019730)
WubTheCaptain (reporter)
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.
User avatar (0019737)
Blzut3 (administrator)
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 [^]'
User avatar (0019738)
WubTheCaptain (reporter)
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.)
User avatar (0019741)
Blzut3 (administrator)
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.
User avatar (0019745)
WubTheCaptain (reporter)
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.
User avatar (0019749)
Blzut3 (administrator)
2018-09-25 03:30

 I'm still confused as to what you think needs to be rewritten.
User avatar (0019959)
WubTheCaptain (reporter)
2018-10-06 18:21

 Blzut3: POSIX sh compatibility. 😛

Issue Community Support
This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2018-09-25 00:08 WubTheCaptain New Issue
2018-09-25 00:08 WubTheCaptain Reproducibility always => sometimes
2018-09-25 00:09 WubTheCaptain Note Added: 0019730
2018-09-25 00:10 WubTheCaptain Note Edited: 0019730 View Revisions
2018-09-25 00:12 WubTheCaptain Summary releasescripts/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:12 WubTheCaptain Steps to Reproduce Updated View Revisions
2018-09-25 00:19 WubTheCaptain Note Edited: 0019730 View Revisions
2018-09-25 00:21 WubTheCaptain Steps to Reproduce Updated View Revisions
2018-09-25 01:23 Blzut3 Assigned To => Blzut3
2018-09-25 01:23 Blzut3 Status new => assigned
2018-09-25 01:30 Blzut3 Note Added: 0019737
2018-09-25 01:30 Blzut3 Status assigned => needs testing
2018-09-25 01:52 WubTheCaptain Note Added: 0019738
2018-09-25 01:52 WubTheCaptain Status needs testing => needs review
2018-09-25 02:12 Blzut3 Note Added: 0019741
2018-09-25 02:51 WubTheCaptain Note Added: 0019745
2018-09-25 02:51 WubTheCaptain Status needs review => resolved
2018-09-25 02:51 WubTheCaptain Fixed in Version => 1.2
2018-09-25 02:51 WubTheCaptain Resolution open => fixed
2018-09-25 02:51 WubTheCaptain Target Version => 1.2
2018-09-25 03:30 Blzut3 Note Added: 0019749
2018-10-06 18:19 WubTheCaptain Status resolved => closed
2018-10-06 18:21 WubTheCaptain Note Added: 0019959




Questions or other issues? Contact Us.

Links

Copyright © 2000 - 2024 MantisBT Team
Powered by Mantis Bugtracker