[merged] Cleanup atomic command minor fixes

[rhatdan]

1 Remove debugging output when I mount an ostree image.
2 Cleanup cmd_env to not modify current processes os.environ

[cgwalters]

Hmm...not related to this change but this looks a bit dangerous - we're mutating the current process' environment, which means that conceptually we can only do one action at a time. If we ever supported running multiple containers in one go, this would have to change.

It's normally better regardless to prepare the environment for the child process rather than mutating self's. Anyways, not a blocker for this patch, just noting.

[rhatdan]

Yes good point, I would love to drop most of the opt handling, since we are now passing the environment from the atomic command into the containers runtime.

[cgwalters]

Looks like the first commit was written by root@?

[rhatdan]

@cgwalters Fixed root->dwalsh
Fixed cmd_env to use a copy of os.environ()
Now container three patches.

[cgwalters]

I think you need newenv = dict(os.environ) or so.

[adelton]

The reason for tests failing is the

    def sub_env_strings(self, in_string):
        """
        Performs substitutions on an input string based on defined
        environment variables.
        :param in_string: string to perform the subs on
        :return: string
        """
        # Set environment variables
        self.cmd_env()

        # Perform variable subs
        in_string = os.path.expandvars(in_string)

        # Replace undefined variables with blank
        in_string = re.sub(r'\$\{?\S*\}?', '', in_string)

        # Solve whitespacing
        in_string = " ".join(in_string.split())

        return in_string

That os.path.expandvars uses os.environ and cannot be passed different env.

So all ${IMAGE} and similar references get effectively cleared with the subsequent re.sub, resulting in

export PYTHONPATH=${PYTHONPATH:-$(pwd)}
export ATOMIC_LIBEXEC="$(pwd)"
./atomic install --display atomic-test-6
docker run --rm=true --net=host -v /:/host -e NAME= -e IMAGE= -e HOST=/host /bin/install.sh

[rhatdan]

@cgwalters What do you think? We could change the environment expand and then change it back?

[adelton]

@cgwalters What do you think? We could change the environment expand and then change it back?

Or reimplement os.path.expandvars to use/accept self.cmd_env().

By the way, why do you want to get rid of the os.environ modifications? The changes will go away when the process exits ...

[rhatdan]

How about this far simpler fix, Also @adelton does this fix your other problem where we were substituting twice.

[cgwalters]

@adelton Regarding mutating os.environ - it's not threadsafe. https://sourceware.org/bugzilla/show_bug.cgi?id=15607

[adelton]

How many threads does a typical atomic command run?

[rhatdan]

1

[adelton]

I don't have enough Python foo to say if it should be dict(os.environ) or os.environ.copy() and whether that os.environ=oldenv does the right thing.

In any case, this patch now does not get rid of the modification of os.environ in cmd_env() which I thought was your goal and which the commit message still describes.

[rhatdan]

I think we can modify it, as long as we are about to exec. Modifying it permanently or running it multiple times can cause problems.

[cgwalters]

Maybe 1 thread right now, but as soon as e.g. we port to GDBus it'll run a thread internally. Using GLib at all really may make your process multithreaded (there's also a worker thread).

[rhatdan]

I guess it would be best if somone took os.path.expandvars and made it take an environment param

[rhatdan]

No longer needed.

[rhatdan]

Added utils.expandvars. But I am not sure if this will work properly on python2 and python3

[rhatdan]

@adelton PTAL

[rhatdan]

My testing looks good for python3 and python2 and the test suite passed.

[rh-atomic-bot]

☔ The latest upstream changes (presumably 683c0fa) made this pull request unmergeable. Please resolve the merge conflicts.

[adelton]

LGTM.

[rhatdan]

@cgwalters PTAL

[cgwalters]

@rh-atomic-bot r+ 9d5aa41

[rh-atomic-bot]

⌛ Testing commit 9d5aa41 with merge 83d87d5...

[rh-atomic-bot]

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 83d87d5 to master...