Merge pull request #52 from manics/fix-arg-env-handling

Fix arg env handling

Author: manics

repo2shellscript/shellscript.py

@@ -25,8 +25,7 @@
 
 
 def _expand_env(s, *args):
-    # repo2docker uses PATH when expanding PATH
-    env = {"PATH": "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}
+    env = {}
     for e in reversed(args):
         env.update(e)
     return Template(s).substitute(env)
@@ -78,7 +77,7 @@ def _docker_copy(copy, chown):
     return statement
 
 
-def dockerfile_to_bash(dockerfile, buildargs):
+def dockerfile_to_bash(dockerfile, buildargs, parentenv):
     """
     Convert a Dockerfile to a bash script
 
@@ -94,16 +93,24 @@ def dockerfile_to_bash(dockerfile, buildargs):
     bash = []
     cmd = ""
     entrypoint = ""
+
     # Needed so we know which directory to run the start command from
     currentdir = ""
-    # Runtime environment
-    runtimeenv = {}
-    # Build and runtime environment
-    currentenv = {}
-    parser = DockerfileParser(dockerfile)
+
+    # The final runtime environment (expanded ENV only)
+    runtimeenv = parentenv.copy()
+
+    # Combined build and runtime environments (since during the build we need
+    # to take ARG and ENV into account)
+    currentenv = parentenv.copy()
+
+    parser = DockerfileParser(
+        dockerfile, env_replace=True, build_args=buildargs, parent_env=parentenv
+    )
     user = "root"
 
-    for d in parser.structure:
+    assert len(parser.structure) == len(parser.context_structure)
+    for (d, ctx) in zip(parser.structure, parser.context_structure):
         statement = ""
         instruction = d["instruction"]
         for line in d["content"].splitlines():
@@ -120,18 +127,12 @@ def dockerfile_to_bash(dockerfile, buildargs):
                 raise NotImplementedError(f"Base image {d['value']} not supported")
             statement += base_setup
         elif instruction == "ARG":
-            argname = d["value"].split("=", 1)[0]
-            try:
-                argvalue = shlex.quote(buildargs[d["value"]])
-            except KeyError:
-                if "=" in d["value"]:
-                    argvalue = d["value"].split("=", 1)[1]
-                else:
-                    raise
-            # Expand because this may eventually end up as a runtime env
-            argvalue = _expand_env(argvalue, currentenv)
-            currentenv[argname] = argvalue
-            statement += f"export {argname}={argvalue}\n"
+            for argname, argvalue in ctx.line_args.items():
+                # Expand because this may eventually end up as a runtime env
+                argvalue = _expand_env(argvalue, currentenv)
+                currentenv[argname] = argvalue
+                escaped_argvalue = shlex.quote(argvalue)
+                statement += f"export {argname}={escaped_argvalue}\n"
         elif instruction == "CMD":
             cmd = " ".join(shlex.quote(p) for p in json.loads(d["value"]))
         elif instruction == "COPY":
@@ -143,20 +144,17 @@ def dockerfile_to_bash(dockerfile, buildargs):
         elif instruction == "ENTRYPOINT":
             entrypoint = " ".join(shlex.quote(p) for p in json.loads(d["value"]))
         elif instruction == "ENV":
-            # repodocker is inconsistent in how it uses ENV
-            try:
-                k, v = d["value"].split("=", 1)
-            except ValueError:
-                k, v = d["value"].split(" ", 1)
-            argvalue = _expand_env(v, currentenv)
-            currentenv[k] = argvalue
-            statement += f"export {k}={argvalue}\n"
-            runtimeenv[k] = argvalue
+            for envname, envvalue in ctx.line_envs.items():
+                envvalue = _expand_env(envvalue, currentenv)
+                currentenv[envname] = envvalue
+                runtimeenv[envname] = envvalue
+                escaped_envvalue = shlex.quote(envvalue)
+                statement += f"export {envname}={escaped_envvalue}\n"
         elif instruction == "RUN":
             run = _sudo_user(user, d["value"], currentenv)
             statement += f"{run}\n"
         elif instruction == "USER":
-            user = d["value"]
+            user = _expand_env(d["value"], currentenv)
         elif instruction == "WORKDIR":
             statement += f"cd {d['value']}\n"
             currentdir = _expand_env(d["value"], currentenv)
@@ -175,8 +173,7 @@ def dockerfile_to_bash(dockerfile, buildargs):
         "dir": currentdir,
         "env": runtimeenv,
         "start": f"{entrypoint} {cmd}",
-        # Expand ${NB_USER}
-        "user": _expand_env(user, currentenv),
+        "user": user,
     }
     return r
 
@@ -233,6 +230,10 @@ def build(
     ):
 
         buildargs = buildargs or {}
+        # repo2docker uses PATH when expanding PATH
+        parentenv = {
+            "PATH": "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
+        }
 
         if not tag:
             tag = str(uuid4())
@@ -260,7 +261,7 @@ def build(
         else:
             dockerfile = os.path.join(builddir)
 
-        r = dockerfile_to_bash(dockerfile, buildargs)
+        r = dockerfile_to_bash(dockerfile, buildargs, parentenv)
         build_file = os.path.join(builddir, "repo2shellscript-build.bash")
         start_file = os.path.join(builddir, "repo2shellscript-start.bash")
         systemd_file = os.path.join(builddir, "repo2shellscript.service")

setup.py

@@ -4,7 +4,7 @@
     name="repo2shellscript",
     # https://github.com/jupyter/repo2docker/pull/848 was merged!
     install_requires=[
-        "dockerfile-parse",
+        "dockerfile-parse>=2,<3",
         "jupyter-repo2docker>=2022.02.0",
         "importlib_resources;python_version<'3.7'",
     ],

tests/reference-outputs/test/repo2shellscript-build.bash

@@ -307,7 +307,7 @@ fi
 # time ${MAMBA_EXE} clean --all -f -y && \
 # ${MAMBA_EXE} list -p ${NB_PYTHON_PREFIX} \
 # '
-sudo -u ${NB_USER} --preserve-env=DEBIAN_FRONTEND,LC_ALL,LANG,LANGUAGE,SHELL,NB_USER,NB_UID,USER,HOME,APP_BASE,CONDA_DIR,NB_PYTHON_PREFIX,NPM_DIR,NPM_CONFIG_GLOBALCONFIG,NB_ENVIRONMENT_FILE,MAMBA_ROOT_PREFIX,MAMBA_EXE,KERNEL_PYTHON_PREFIX,PATH,REPO_DIR,CONDA_DEFAULT_ENV bash -c 'TIMEFORMAT='"'"'time: %3R'"'"' bash -c '"'"'time ${MAMBA_EXE} env update -p ${NB_PYTHON_PREFIX} --file "environment.yml" && time ${MAMBA_EXE} clean --all -f -y && ${MAMBA_EXE} list -p ${NB_PYTHON_PREFIX} '"'"''
+sudo -u test --preserve-env=PATH,DEBIAN_FRONTEND,LC_ALL,LANG,LANGUAGE,SHELL,NB_USER,NB_UID,USER,HOME,APP_BASE,CONDA_DIR,NB_PYTHON_PREFIX,NPM_DIR,NPM_CONFIG_GLOBALCONFIG,NB_ENVIRONMENT_FILE,MAMBA_ROOT_PREFIX,MAMBA_EXE,KERNEL_PYTHON_PREFIX,REPO_DIR,CONDA_DEFAULT_ENV bash -c 'TIMEFORMAT='"'"'time: %3R'"'"' bash -c '"'"'time ${MAMBA_EXE} env update -p ${NB_PYTHON_PREFIX} --file "environment.yml" && time ${MAMBA_EXE} clean --all -f -y && ${MAMBA_EXE} list -p ${NB_PYTHON_PREFIX} '"'"''
 
 # ensure root user after preassemble scripts
 

tests/reference-outputs/test/repo2shellscript-start.bash

@@ -4,6 +4,7 @@ if [ $(id -un) != test ]; then
     echo ERROR: Must be run as user test
     exit 1
 fi
+export PATH=/home/test/.local/bin:/home/test/.local/bin:/srv/conda/envs/notebook/bin:/srv/conda/bin:/srv/npm/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
 export DEBIAN_FRONTEND=noninteractive
 export LC_ALL=en_US.UTF-8
 export LANG=en_US.UTF-8
@@ -20,7 +21,6 @@ export NB_ENVIRONMENT_FILE=/tmp/env/environment.lock
 export MAMBA_ROOT_PREFIX=/srv/conda
 export MAMBA_EXE=/srv/conda/bin/mamba
 export KERNEL_PYTHON_PREFIX=/srv/conda/envs/notebook
-export PATH=/home/test/.local/bin:/home/test/.local/bin:/srv/conda/envs/notebook/bin:/srv/conda/bin:/srv/npm/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
 export REPO_DIR=/home/test
 export CONDA_DEFAULT_ENV=/srv/conda/envs/notebook
 export PYTHONUNBUFFERED=1

tests/reference-outputs/test/repo2shellscript.service

@@ -5,6 +5,7 @@ Description=repo2shellscript
 User=test
 Restart=always
 RestartSec=10
+Environment='PATH=/home/test/.local/bin:/home/test/.local/bin:/srv/conda/envs/notebook/bin:/srv/conda/bin:/srv/npm/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'
 Environment='DEBIAN_FRONTEND=noninteractive'
 Environment='LC_ALL=en_US.UTF-8'
 Environment='LANG=en_US.UTF-8'
@@ -21,7 +22,6 @@ Environment='NB_ENVIRONMENT_FILE=/tmp/env/environment.lock'
 Environment='MAMBA_ROOT_PREFIX=/srv/conda'
 Environment='MAMBA_EXE=/srv/conda/bin/mamba'
 Environment='KERNEL_PYTHON_PREFIX=/srv/conda/envs/notebook'
-Environment='PATH=/home/test/.local/bin:/home/test/.local/bin:/srv/conda/envs/notebook/bin:/srv/conda/bin:/srv/npm/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'
 Environment='REPO_DIR=/home/test'
 Environment='CONDA_DEFAULT_ENV=/srv/conda/envs/notebook'
 Environment='PYTHONUNBUFFERED=1'

tests/test_repo2shellscript.py

@@ -23,7 +23,7 @@ def _recursive_filelist(d):
 def _normalise_build_script(lines):
     for line in lines:
         line = re.sub(
-            r"build_script_files/\S+-2fsite-2dpackages-2f(\S+)-\w+(\s+|/)",
+            r"build_script_files/\S+-2f(repo2docker-2fbuildpacks-2f\S+)-\w+(\s+|/)",
             r"<normalised>\1 ",
             line,
         )
@@ -51,7 +51,7 @@ def test_compare(tmp_path):
     # Normalise filenames under build_script_files
     for build_script in (tmp_path / "test" / "build_script_files").iterdir():
         normalised_name = re.match(
-            r"^.+-2fsite-2dpackages-2f(.+)-\w+$", build_script.name
+            r"^.+-2f(repo2docker-2fbuildpacks-2f.+)-\w+$", build_script.name
         ).group(1)
         build_script.rename(build_script.parent / normalised_name)