check presence of CWD at the end of run_shell_cmd and try to return to original working directory if non-existent
#4390
Conversation
boegel
changed the title
run_cmd (via complete_cmd)
|
@lexming I've enhanced an existing test to catch this problem, see lexming#2 The only way to trigger the problem was to specify a working directory to If |
|
@lexming I tried rebuilding That also makes me a bit reluctant to merge this, since we're actually changing behavior here: the post That doesn't mean there no bug here, you could argue that |
|
@boegel ok I'll investigate this further 👍 |
|
@lexming Any updates? |
|
@lexming ping? |
|
I finally troubleshooted this issue. The underlying cause is a not-so-reliable NFS mount in our cluster where the installation directory is located. The combination of this flaky NFS mount with a heavy build process like the one in OpenFOAM that is done directly on the installation directory ( I opened #4525 to improve the error reporting in this case. Nonetheless, I still believe that we should change directory to a well defined location (like I agree that this is a change of behavior, so I'm changing targets to |
… is robust against command that removes the directory it's running in
…riginal workdir or jump into a new working subdirectory and remove it
|
Improved the tests to consider 2 different cases:
|
lexming
changed the title
run_cmd (via complete_cmd)run_shell_cmd
… without work_dir set
|
We discussed this in the EB5 meeting. I will summarize my thoughts. I think the premise is wrong here, neither the import os
import subprocess
print(os.getcwd())
p = subprocess.Popen(['pwd'], shell=True, cwd='/tmp', stdout=subprocess.PIPE)
a, b = p.communicate()
print('subprocess cwd:', a.decode('utf8'))
print(os.getcwd())
p = subprocess.Popen(['cd Downloads; pwd'], shell=True, stdout=subprocess.PIPE)
a, b = p.communicate()
print('subprocess cwd:', a.decode('utf8'))
print(os.getcwd())
p = subprocess.Popen(['pwd'], cwd='Downloads', shell=True, stdout=subprocess.PIPE)
a, b = p.communicate()
print('subprocess cwd:', a.decode('utf8'))
print(os.getcwd())it's still just the expected so In some easyblocks we still do patterns like but those should be replaced with just knowing that the cwd of the main process will not under any circumstance change, regardless of what some_path is or cmd does. So this would drastically change this, ensuring that the dir is changed to I'm struggling a bit to understand what the tests are trying to do here. I attempted extracting the core parts so i can run it manually and this work just fine with EB5.0 without this PR. The output from the command is and my |
|
@Micket Indeed, you are right and the working directory passed to For instance, the following is a scenario that will cause trouble:
The original issue in this PR is effectively triggering this problem. The command executed in So we still need to decide what to do in such a case, as any forward execution of EB in that situation is dangerous. I agree with you that always changing the working directory at the end of |
lexming
changed the title
run_shell_cmdrun_shell_cmd and try to return to original working directory if non-existent
Yes, or literally any other directory. Maybe it deletes the builddir and there is of course no way out of that, except fixing the broken build/command that was executed.
I don't understand the issue with that. This should crash/be a problem, and it is? Especially if entire
I think the most important thing is that subprocess/run_shell_cmd never changes anything for the main process, be it working directory or environment variables. If anything, i think all the sprinkled |
OpenFOAM installation fails after a successful build because the hooks after the run command fail to determine the working directory. The reason is that the last working directory during the build gets deleted just before that step.
I suggest to change a bit the order of execution here, to make sure that we sit in an existing working directory before we call the hooks.