Arvados

A branch with a bugfix for the build is up for review. I built the package, installed it, confirmed the permissions were right, and confirmed that su -c 'pip freeze' nobody produced correct output after installation.

LGTM.

Closest thing to a complaint is that we aren't fixing what looks to me like an existing bug here:

pyfpm_workdir=$(mktemp ...) && (
  set -e
  ...
)
if [ -n "$pyfpm_workdir" ]; then
  rm -rf "$pyfpm_workdir" 
fi

Shouldn't this be:

set -e
pyfpm_workdir=$(mktemp ...)
...
rm -rf "$pyfpm_workdir" 

If we fail to make a tmpdir, we should probably admit defeat, rather than silently building a package with broken permissions...

Tom Clegg wrote:

Closest thing to a complaint is that we aren't fixing what looks to me like an existing bug here:

I know this code is subtle, but I don't think that's right. I think the current code does what you want; and I think it's better than your suggestion. We want to clean the tmpdir, whether the build process succeeds or fails, at any point. The cleanest way to do that from a code perspective is to do that cleanup after the subshell doing the main build is done. If we do that, we can't do the mktemp inside the subshell, because we'll forget where the tmpdir is after the subshell is done.

Chaining the mktemp and the subshell with && means that the build is prevented if making a temporary directory fails. In that case, the subshell doesn't run at all, and $? will have mktemp's exit code when we check that later, so there's an error about it.

Did I understand you right, or did I miss something in your comment?

No, you're right. Apparently I ignored the if [ 0 != "$?" ] part. Sorry for the noise.