Skip to content
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

rc1: add shebang to sub-scripts #1597

Merged
merged 1 commit into from Jul 24, 2018

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Jul 24, 2018

Problem: the lack of a #!/bin/bash confuses spindle when it
runs flux.

Add the missing shebang.

Fixes #1596

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 24, 2018

Might also mention in the problem statement that lack of shebang causes an extra call to exec. I think what might be happening is that bash tries to exec a text file, and if exec fails, it falls back to running a copy of bash with the file as input. (i.e., this isn't just fixing a spindle issue)

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 24, 2018

Coverage Status

Coverage decreased (-0.006%) to 79.44% when pulling a608eab on garlick:spindle_fixes into 4830a90 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 24, 2018

Codecov Report

Merging #1597 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1597      +/-   ##
==========================================
- Coverage   79.27%   79.26%   -0.01%     
==========================================
  Files         171      171              
  Lines       31341    31341              
==========================================
- Hits        24846    24844       -2     
- Misses       6495     6497       +2
Impacted Files Coverage Δ
src/common/libflux/request.c 88.46% <0%> (-1.29%) ⬇️
src/broker/modservice.c 80.58% <0%> (-0.98%) ⬇️
src/common/libutil/base64.c 95.07% <0%> (-0.71%) ⬇️
src/common/libflux/handle.c 83.66% <0%> (-0.25%) ⬇️
src/common/libflux/message.c 81.02% <0%> (+0.11%) ⬆️
src/common/libflux/response.c 83.55% <0%> (+0.65%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 24, 2018

rc1: add shebang to sub-scripts
Problem: rc1 sub-scripts lack #!/bin/bash, thus the
shell must fail an execve(2) with ENOEXEC, before
falling back to interpreting the file as a shell script.
This behavior also confuses spindle, a tool for speeding
up executable loading on clusters, when it runs flux,
as discussed in #1514.

Add #!/bin/bash to rc1 sub-scripts.

Fixes #1596

@garlick garlick force-pushed the garlick:spindle_fixes branch from 371636a to a608eab Jul 24, 2018

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 24, 2018

Ok, thanks! I was going off an strace run. With the current version of bash anyway, it will try to exec() the script, which gets an error (ENOEXEC). Bash then falls back to forking a new shell and feeding the shell the script:

$ cat t.sh
echo hi
$ strace -e process  bash -c './t.sh'
execve("/usr/bin/bash", ["bash", "-c", "./t.sh"], [/* 70 vars */]) = 0
arch_prctl(ARCH_SET_FS, 0x2aaaaab126c0) = 0
clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x2aaaaab12990) = 227703
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=227703, si_uid=6885, si_status=0, si_utime=0, si_stime=0} ---
wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], WNOHANG, NULL) = 227703
wait4(-1, 0x7fffffffcb50, WNOHANG, NULL) = -1 ECHILD (No child processes)
execve("./t.sh", ["./t.sh"], [/* 69 vars */]) = -1 ENOEXEC (Exec format error)
clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x2aaaaab12990) = 227706
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=227706, si_uid=6885, si_status=0, si_utime=0, si_stime=0} ---
wait4(-1, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], WNOHANG, NULL) = 227706
wait4(-1, 0x7fffffffcb50, WNOHANG, NULL) = -1 ECHILD (No child processes)
hi
exit_group(0)                           = ?
+++ exited with 0 +++

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 24, 2018

That's pretty definitive then! Does my revised commit message capture it?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 24, 2018

Yes, totally! Sorry I didn't mean to be nit-picky!

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 24, 2018

Not at all!

@grondo grondo merged commit 55d47d5 into flux-framework:master Jul 24, 2018

3 of 4 checks passed

codecov/project 79.26% (-0.01%) compared to 4830a90
Details
codecov/patch Coverage not affected when comparing 4830a90...a608eab
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.006%) to 79.44%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.