Remove Gramine dependencies that are not needed at runtime #144
Conversation
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @amathew3)
gsc.py line 347 at r1 (raw file):
# using the user-provided config file with info on OS distro, Gramine version and SGX driver env = jinja2.Environment() env.globals.update(yaml.safe_load(args.config_file))
After this line, you should add env.globals.update(vars(args))
gsc.py line 370 at r1 (raw file):
build_docker_image(docker_socket.api, tmp_build_path, signed_image_name, 'Dockerfile.sign', forcerm=True, buildargs={"passphrase": args.passphrase, "remove_gramine_deps":str(args.remove_gramine_deps)})
This is wrong, please revert this. You should use Jinja-style variables, not ARG Dockerfile variables.
gsc.py line 515 at r1 (raw file):
sub_sign.add_argument('-p', '--passphrase', help='Passphrase for the signing key.') sub_sign.add_argument('--remove-gramine-deps', action='store_true', help='Remove Gramine dependencies that are not needed at runtime')
Please add a dot at the end of the sentence
Documentation/index.rst line 176 at r1 (raw file):
Remove Gramine dependencies that are not needed at runtime. This may have a negative side effect of removing even those dependencies that are actually needed by the original application.
Please also add a sentence at the end Use with care!
templates/Dockerfile.common.sign.template line 21 at r1 (raw file):
COPY --from=unsigned_image /gramine/app_files/*.sgx /gramine/app_files/ {% block uninstall %}{% endblock %}
This will not work on non-root images, because uninstalling packages requires root privileges. Just try some non-root image for yourself, and you'll see how GSC fails here.
You'll need to surround this block with something like this:
gsc/templates/Dockerfile.common.build.template
Lines 11 to 12 in f656037
gsc/templates/Dockerfile.common.build.template
Lines 26 to 27 in f656037
And please test with root and non-root images, to make sure it works in all cases.
templates/centos/Dockerfile.sign.template line 4 at r1 (raw file):
{% block uninstall %} ARG remove_gramine_deps
That's wrong. You don't want Dockerfile arguments, you want to use Jinja-style variables. See my other comment.
templates/centos/Dockerfile.sign.template line 5 at r1 (raw file):
{% block uninstall %} ARG remove_gramine_deps RUN if [ "$remove_gramine_deps" = "True" ];then \
The RUN keyword should be under the if body, like this:
{% if remove_gramine_deps %}
RUN \
dnf update -y \
...
{% endif %}
templates/centos/Dockerfile.sign.template line 6 at r1 (raw file):
ARG remove_gramine_deps RUN if [ "$remove_gramine_deps" = "True" ];then \ dnf update -y \
Why do you need this update?
templates/centos/Dockerfile.sign.template line 7 at r1 (raw file):
RUN if [ "$remove_gramine_deps" = "True" ];then \ dnf update -y \ && dnf install -y python3-pip \
You must install pip to uninstall the previously-installed Python packages (like click), do I understand correctly?
templates/debian/Dockerfile.sign.template line 4 at r1 (raw file):
{% block uninstall %} ARG remove_gramine_deps
ditto
templates/debian/Dockerfile.sign.template line 5 at r1 (raw file):
{% block uninstall %} ARG remove_gramine_deps RUN if [ "$remove_gramine_deps" = "True" ];then \
ditto
templates/debian/Dockerfile.sign.template line 6 at r1 (raw file):
ARG remove_gramine_deps RUN if [ "$remove_gramine_deps" = "True" ];then \ apt update -y \
ditto
templates/debian/Dockerfile.sign.template line 18 at r1 (raw file):
python3-pyelftools \ && apt autoremove -y \ && rm -rf /var/lib/apt/lists/*; \
Why this additional rm?
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
gsc.py line 347 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
After this line, you should add
env.globals.update(vars(args))
Done
gsc.py line 370 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is wrong, please revert this. You should use Jinja-style variables, not
ARGDockerfile variables.
Done
gsc.py line 515 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a dot at the end of the sentence
Done
Documentation/index.rst line 176 at r1 (raw file):
Use with care!
Done
templates/Dockerfile.common.sign.template line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This will not work on non-root images, because uninstalling packages requires root privileges. Just try some non-root image for yourself, and you'll see how GSC fails here.
You'll need to surround this block with something like this:
gsc/templates/Dockerfile.common.build.template
Lines 11 to 12 in f656037
gsc/templates/Dockerfile.common.build.template
Lines 26 to 27 in f656037
And please test with root and non-root images, to make sure it works in all cases.
Done
templates/centos/Dockerfile.sign.template line 4 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
That's wrong. You don't want Dockerfile arguments, you want to use Jinja-style variables. See my other comment.
Modified to Jinja style variable...
templates/centos/Dockerfile.sign.template line 5 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The
RUNkeyword should be under the if body, like this:{% if remove_gramine_deps %} RUN \ dnf update -y \ ... {% endif %}
Done
templates/centos/Dockerfile.sign.template line 6 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need this update?
dnf update is not needed as the repo is already updated. Removed.
templates/centos/Dockerfile.sign.template line 7 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You must install
pipto uninstall the previously-installed Python packages (likeclick), do I understand correctly?
Yes, the understanding is correct. Since the install and removal is happening at the same layer no much space needed.
templates/debian/Dockerfile.sign.template line 4 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done
templates/debian/Dockerfile.sign.template line 5 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done
templates/debian/Dockerfile.sign.template line 6 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done
templates/debian/Dockerfile.sign.template line 18 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why this additional
rm?
Removing list of packages downloaded as part of apt update. rm -rf /var/lib/apt/lists/* is also recommended in https://docs.docker.com/develop/develop-images/dockerfile_best-practices/
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
templates/centos/Dockerfile.sign.template line 6 at r1 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
dnf updateis not needed as the repo is already updated. Removed.
Sorry, Considering PR#141, we should be doing dnf update before installing some package.
templates/debian/Dockerfile.sign.template line 5 at r1 (raw file):
Switch back to original app_image user
templates/debian/Dockerfile.sign.template line 6 at r1 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
Done
Sorry, Considering PR#141, we should be doing apt update before installing some package.
There was a problem hiding this comment.
Reviewed 3 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
gsc.py line 334 at r3 (raw file):
tmp_build_path = gsc_tmp_build_path(args.image) # pathlib obj with build artifacts
Accidental change? Please remove this line.
templates/centos/Dockerfile.sign.template line 4 at r1 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
Modified to Jinja style variable...
Thanks, looks much better now!
templates/debian/Dockerfile.sign.template line 5 at r3 (raw file):
{% block uninstall %} {% if remove_gramine_deps %} RUN \
There is no need for two spaces before RUN. Or am I missing something? Why this indentation?
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
gsc.py line 334 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Accidental change? Please remove this line.
Sorry, It was an accidental change.. Removed.
templates/debian/Dockerfile.sign.template line 5 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
There is no need for two spaces before
RUN. Or am I missing something? Why this indentation?
Indentation not needed, removed.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
templates/Dockerfile.common.sign.template line 23 at r4 (raw file):
# Temporarily switch to the root user to uninstall packages USER root
Please move {% if remove_gramine_deps %} here, before block uninstall
templates/centos/Dockerfile.sign.template line 4 at r4 (raw file):
{% block uninstall %} {% if remove_gramine_deps %}
Sorry, just noticed that you have this line duplicated in each OS-specific template. There seems to be no reason for this duplication, so please move it into the common template.
templates/debian/Dockerfile.sign.template line 4 at r4 (raw file):
{% block uninstall %} {% if remove_gramine_deps %}
ditto (move from here into common template)
There was a problem hiding this comment.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
templates/centos/Dockerfile.sign.template line 4 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Sorry, just noticed that you have this line duplicated in each OS-specific template. There seems to be no reason for this duplication, so please move it into the
commontemplate.
Done, Now it is in the common place
templates/debian/Dockerfile.sign.template line 4 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (move from here into
commontemplate)
Done, Now it is in the common sign file
There was a problem hiding this comment.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
templates/Dockerfile.common.sign.template line 23 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please move
{% if remove_gramine_deps %}here, beforeblock uninstall
Done
templates/Dockerfile.common.sign.template line 30 at r5 (raw file):
USER {{app_user}} {% endif %}
Please remove empty line here.
There was a problem hiding this comment.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
templates/Dockerfile.common.sign.template line 30 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove empty line here.
Removed, thanks
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 1 of 5 files at r2, 1 of 2 files at r4, 2 of 3 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
templates/centos/Dockerfile.sign.template line 7 at r6 (raw file):
dnf update -y \ && dnf install -y python3-pip \ && pip3 uninstall -y click jinja2 \
double space
Code quote:
pip3 uninstall
There was a problem hiding this comment.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
templates/centos/Dockerfile.sign.template line 7 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
double space
Corrected
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
a discussion (no related file):
Let's wait for @woju's review, he was the one involved in discussing this whole thing. I pinged him on priv already.
There was a problem hiding this comment.
Reviewed 1 of 5 files at r2, 1 of 2 files at r4, 1 of 3 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
gsc.py line 348 at r7 (raw file):
env = jinja2.Environment() env.globals.update(yaml.safe_load(args.config_file)) env.globals.update(vars(args))
I don't like this solution, because you all options into the env, which might have conflicting names. Right now it doesn't, but it's not clear to anyone adding new CLI options that you need to make sure those don't collide with unrelated subsystem.
I'd prefer (in order of preference):
- add explicit
'--define', '-D'option similar to what we have ingramine-manifestthat would acceptkey=valuepairs which are added to globals (and only those); then if you prefer, add--remove-gramine-depsas an alias for-Dremove_gramine_deps=true; - add whole namespace as a single object (
env.globals['args'] = args) and use those as{% if args.flag %}. Because that's namespace, there will be a less risk of a collision.
gsc.py line 516 at r7 (raw file):
sub_sign.add_argument('-p', '--passphrase', help='Passphrase for the signing key.') sub_sign.add_argument('--remove-gramine-deps', action='store_true', help='Remove Gramine dependencies that are not needed at runtime.')
Plz also add --no-remove-gramine-deps (action='store_false' and proper dest=). In case someone is scripting this, you need to be able to revert this.
templates/centos/Dockerfile.sign.template line 6 at r1 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
Sorry, Considering PR#141, we should be doing
dnf updatebefore installing some package.
This is wrong: dnf update is an alias for dnf upgrade and will update all packages in the system like apt-get upgrade. If you intended to force cache refresh, IIUC this is dnf makecache, but you probably don't need it at all, because dnf downloads repos whenever it likes as part of other commands, (here it will download in next line, dnf install).
templates/centos/Dockerfile.sign.template line 9 at r7 (raw file):
&& pip3 uninstall -y click jinja2 \ 'tomli>=1.1.0' 'tomli-w>=0.4.0' \ 'toml>=0.10' \
Why do you need explicit versions for pip uninstall?
templates/centos/Dockerfile.sign.template line 10 at r7 (raw file):
'tomli>=1.1.0' 'tomli-w>=0.4.0' \ 'toml>=0.10' \ && rpm -e --nodeps binutils \
No rpm -e please, see #141. Please do dnf remove, or comment why we rpm -e, inter alia, openssl or tcl.
templates/debian/Dockerfile.sign.template line 9 at r7 (raw file):
&& pip3 uninstall -y click jinja2 \ 'tomli>=1.1.0' 'tomli-w>=0.4.0' \ 'toml>=0.10' \
ditto (why explicit versions)
templates/debian/Dockerfile.sign.template line 19 at r7 (raw file):
python3-pyelftools \ && apt autoremove -y \ && rm -rf /var/lib/apt/lists/*;
apt-get whole stanza please.
There was a problem hiding this comment.
Reviewable status: 1 of 5 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @woju)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Let's wait for @woju's review, he was the one involved in discussing this whole thing. I pinged him on priv already.
ok
gsc.py line 348 at r7 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I don't like this solution, because you all options into the env, which might have conflicting names. Right now it doesn't, but it's not clear to anyone adding new CLI options that you need to make sure those don't collide with unrelated subsystem.
I'd prefer (in order of preference):
- add explicit
'--define', '-D'option similar to what we have ingramine-manifestthat would acceptkey=valuepairs which are added to globals (and only those); then if you prefer, add--remove-gramine-depsas an alias for-Dremove_gramine_deps=true;- add whole namespace as a single object (
env.globals['args'] = args) and use those as{% if args.flag %}. Because that's namespace, there will be a less risk of a collision.
added --define option, also created alias --remove-gramine-deps which will be exposed to user.
gsc.py line 516 at r7 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Plz also add
--no-remove-gramine-deps(action='store_false'and properdest=). In case someone is scripting this, you need to be able to revert this.
Added.
templates/centos/Dockerfile.sign.template line 6 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
This is wrong:
dnf updateis an alias fordnf upgradeand will update all packages in the system likeapt-get upgrade. If you intended to force cache refresh, IIUC this isdnf makecache, but you probably don't need it at all, because dnf downloads repos whenever it likes as part of other commands, (here it will download in next line,dnf install).
Yes, we don't need to do cache refresh in CentOS. removed dnf update.
templates/centos/Dockerfile.sign.template line 9 at r7 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Why do you need explicit versions for pip uninstall?
As we install 2 version of toml, thought of removing those explicit versions.
templates/centos/Dockerfile.sign.template line 10 at r7 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
No
rpm -eplease, see #141. Please dodnf remove, or comment why werpm -e, inter alia, openssl or tcl.
moved to dnf remove
templates/debian/Dockerfile.sign.template line 9 at r7 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
ditto (why explicit versions)
pip3 uninstall toml is not removing both versions of toml which we install as part of GSC build. So removing the with explicit versions.
templates/debian/Dockerfile.sign.template line 19 at r7 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
apt-getwhole stanza please.
Done
There was a problem hiding this comment.
Reviewed 4 of 4 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @mkow, and @woju)
gsc.py line 348 at r7 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
added --define option, also created alias --remove-gramine-deps which will be exposed to user.
@woju We have this problem in other places in GSC (for gsc build and gsc build-gramine sub-commands). Do you want to fix everywhere?
@amathew3 You applied both suggestions from Woju, though he wanted only one of them. I actually would prefer suggestion number 2 (export whole namespace)... Let's first decide how we want to deal with it in the whole GSC, not only gsc sign-image sub-command.
gsc.py line 202 at r9 (raw file):
env.filters['shlex_quote'] = shlex.quote env.globals.update(config) env.globals.update(vars(args))
Here we're doing the same problematic thing (the one @woju doesn't like)
gsc.py line 306 at r9 (raw file):
env = jinja2.Environment() env.globals.update(config) env.globals.update(vars(args))
Here we're doing the same problematic thing (the one @woju doesn't like)
Documentation/index.rst line 175 at r9 (raw file):
Set image sign-time variables during :command:`gsc sign` (same as `docker build --build-arg`).
This docker build --build-arg is meaningless here. I would remove it.
templates/centos/Dockerfile.sign.template line 5 at r9 (raw file):
{% block uninstall %} RUN \ pip3 uninstall -y click jinja2 \
But you need this dnf install -y python3-pip, no? Otherwise you probably don't have pip3 installed at all, and this command will fail.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @dimakuv, @mkow, and @woju)
gsc.py line 348 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@woju We have this problem in other places in GSC (for
gsc buildandgsc build-graminesub-commands). Do you want to fix everywhere?@amathew3 You applied both suggestions from Woju, though he wanted only one of them. I actually would prefer suggestion number 2 (export whole namespace)... Let's first decide how we want to deal with it in the whole GSC, not only
gsc sign-imagesub-command.
@dimakuv - The whole namespace as a single object implementation avoids collisions (and yes we need to decide if GSC in general should do this way), and the first suggestion from Woju is just about adding explicit --define key value pair with aliases. How are these two achieving the same purpose?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @mkow, and @woju)
gsc.py line 348 at r7 (raw file):
How are these two achieving the same purpose?
@aneessahib Sure, we could implement both, but this looks like an overkill for me. The main idea of @woju was to have a clear separation between variables-from-command-line-arguments and variables-from-other-places (like internal variables from a script or internal Jinja engine variables).
And we could achieve a clear separation just be prepending all variables-from-command-line-arguments with signargs. as is currently done in the PR (though only for the gsc sign-image sub-command).
I wouldn't mind using both options, though it will require non-trivial amount of changes to GSC, and it will be a completely orthogonal task to what this PR wanted to achieve initially.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @dimakuv, @mkow, and @woju)
gsc.py line 348 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
How are these two achieving the same purpose?
@aneessahib Sure, we could implement both, but this looks like an overkill for me. The main idea of @woju was to have a clear separation between variables-from-command-line-arguments and variables-from-other-places (like internal variables from a script or internal Jinja engine variables).
And we could achieve a clear separation just be prepending all variables-from-command-line-arguments with
signargs.as is currently done in the PR (though only for thegsc sign-imagesub-command).I wouldn't mind using both options, though it will require non-trivial amount of changes to GSC, and it will be a completely orthogonal task to what this PR wanted to achieve initially.
Ok i get it. Agree - I would prefer keeping the namespace separation for another PR to cleanly implement across GSC.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @mkow, and @woju)
gsc.py line 348 at r7 (raw file):
Previously, aneessahib (Anees Sahib) wrote…
Ok i get it. Agree - I would prefer keeping the namespace separation for another PR to cleanly implement across GSC.
I would also prefer two PRs, each doing exactly one thing:
- This PR that does only the removal of Gramine dependencies (as originally intended).
- One more PR that creates the namespace separation (for all GSC sub-commands).
If Woju insists, we could first merge the second PR and then the first one. But I wouldn't care much about the order of PRs. @woju Does this sound like a plan to you?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @mkow, and @woju)
gsc.py line 132 at r9 (raw file):
print(f'Could not set build arg `{item}` from environment.') sys.exit(1) return buildargs_dict
Please add an empty line after this line
gsc.py line 133 at r9 (raw file):
sys.exit(1) return buildargs_dict def extract_sign_args(args):
Can you rename this to extract_define_args(args)? It makes more sense
gsc.py line 134 at r9 (raw file):
return buildargs_dict def extract_sign_args(args): signargs_dict = {}
Please rename to defineargs_dict
gsc.py line 140 at r9 (raw file):
signargs_dict[key] = value else: # user specified --build-arg with key and without value, let's retrieve value from env
--build-arg- > --define
gsc.py line 144 at r9 (raw file):
signargs_dict[item] = os.environ[item] else: print(f'Could not set build arg `{item}` from environment.')
build arg -> define arg
gsc.py line 202 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Here we're doing the same problematic thing (the one @woju doesn't like)
@amathew3 Please create a new PR or GitHub issue that fixes this, and I'll resolve my comment here.
gsc.py line 306 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Here we're doing the same problematic thing (the one @woju doesn't like)
@amathew3 Please create a new PR or GitHub issue that fixes this, and I'll resolve my comment here.
gsc.py line 532 at r9 (raw file):
help='Set image sign-time variables (same as "docker build --build-arg").') sub_sign.add_argument('--remove-gramine-deps', action='append_const', dest='define', const='remove_gramine_deps=true',help='Remove Gramine dependencies that are not needed at runtime.')
Please wrap to 100 chars per line
gsc.py line 534 at r9 (raw file):
const='remove_gramine_deps=true',help='Remove Gramine dependencies that are not needed at runtime.') sub_sign.add_argument('--no-remove-gramine-deps', action='append_const', dest='define', const='remove_gramine_deps=false',help='Retain Gramine dependencies that are not needed at runtime.')
Please wrap to 100 chars per line
templates/centos/Dockerfile.sign.template line 9 at r7 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
As we install 2 version of toml, thought of removing those explicit versions.
I also don't understand why you need to specify explicit versions here?
templates/debian/Dockerfile.sign.template line 9 at r7 (raw file):
pip3 uninstall toml is not removing both versions of toml which we install as part of GSC build.
What do you mean by "both versions"?
There was a problem hiding this comment.
Reviewed 4 of 4 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @dimakuv, and @mkow)
gsc.py line 348 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would also prefer two PRs, each doing exactly one thing:
- This PR that does only the removal of Gramine dependencies (as originally intended).
- One more PR that creates the namespace separation (for all GSC sub-commands).
If Woju insists, we could first merge the second PR and then the first one. But I wouldn't care much about the order of PRs. @woju Does this sound like a plan to you?
I don't care either way, there's no need to merge the fix before this PR. As you prefer.
gsc.py line 140 at r9 (raw file):
signargs_dict[key] = value else: # user specified --build-arg with key and without value, let's retrieve value from env
No, this is wrong. If user specified arg without value, then the value should either be True, or be a hard failure. Double-using this as sourcing value from env is unexpected. If you want to read value from env, then please add another option.
gsc.py line 363 at r9 (raw file):
env.globals.update(yaml.safe_load(args.config_file)) extract_user_from_image_config(unsigned_image.attrs['Config'], env) env.globals['signargs']=extract_sign_args(args)
Do you expect some other args that you needed to qualify those args as "sign" args?
gsc.py line 534 at r9 (raw file):
const='remove_gramine_deps=true',help='Remove Gramine dependencies that are not needed at runtime.') sub_sign.add_argument('--no-remove-gramine-deps', action='append_const', dest='define', const='remove_gramine_deps=false',help='Retain Gramine dependencies that are not needed at runtime.')
Please set this to empty string, so it's always false-ish.
templates/Dockerfile.common.sign.template line 21 at r9 (raw file):
COPY --from=unsigned_image /gramine/app_files/*.sgx /gramine/app_files/ {% if signargs.remove_gramine_deps == 'true' %}
== 'true' should be unnecessary: nonempty string is true.
templates/centos/Dockerfile.sign.template line 9 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I also don't understand why you need to specify explicit versions here?
You can't have two versions installed together in one store, so if you did pip install twice, you have 1 version, probably newer one (or last one if you installed with ==).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @mkow, and @woju)
gsc.py line 140 at r9 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
No, this is wrong. If user specified arg without value, then the value should either be
True, or be a hard failure. Double-using this as sourcing value from env is unexpected. If you want to read value from env, then please add another option.
+1 to @woju. I kinda liked this trick, but I agree that this is very unexpected to unprepared user.
gsc.py line 363 at r9 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Do you expect some other args that you needed to qualify those args as "sign" args?
+1, I agree that just ['args'] is a good name. Please fix the corresponding template too!
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @woju)
gsc.py line 348 at r7 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I don't care either way, there's no need to merge the fix before this PR. As you prefer.
Will address in separate PR
gsc.py line 132 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add an empty line after this line
Done
gsc.py line 134 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please rename to
defineargs_dict
Done
gsc.py line 140 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1 to @woju. I kinda liked this trick, but I agree that this is very unexpected to unprepared user.
User specified arg without value performing a force quit.
gsc.py line 140 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
--build-arg- >--define
Done
gsc.py line 144 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
build arg->define arg
Done
gsc.py line 363 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1, I agree that just
['args']is a good name. Please fix the corresponding template too!
Done
gsc.py line 532 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please wrap to 100 chars per line
Done
gsc.py line 534 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please wrap to 100 chars per line
Done
gsc.py line 534 at r9 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Please set this to empty string, so it's always false-ish.
Done
Documentation/index.rst line 175 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This
docker build --build-argis meaningless here. I would remove it.
Done
templates/Dockerfile.common.sign.template line 21 at r9 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
== 'true'should be unnecessary: nonempty string is true.
What if user passes --define remove_gramine_deps=false?. In this case we will have a nonempty string with value as false.
templates/centos/Dockerfile.sign.template line 9 at r7 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
You can't have two versions installed together in one store, so if you did
pip installtwice, you have 1 version, probably newer one (or last one if you installed with==).
Removed version numbers.
templates/centos/Dockerfile.sign.template line 5 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But you need this
dnf install -y python3-pip, no? Otherwise you probably don't havepip3installed at all, and this command will fail.
In CentOS, we are not removing python3-pip after the build stage.
templates/debian/Dockerfile.sign.template line 9 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
pip3 uninstall toml is not removing both versions of toml which we install as part of GSC build.
What do you mean by "both versions"?
I was taking about the specific versions of toml,tomli and toml-w. The only intention here is to remove the version which we install as part of build. Its clear to that in remove it is not going to make any sense. Cleaned up. Thanks.
There was a problem hiding this comment.
Reviewed 4 of 5 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @dimakuv, and @mkow)
gsc.py line 348 at r7 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
Will address in separate PR
Sure.
gsc.py line 142 at r11 (raw file):
else: #user specified --define with key and without value, exiting here. print(f'Could not find define arg `{item}` value.')
I would really prefer if this said message that you need to use = or that the syntax is NAME=VALUE or something else explaining how to correct the (to us obvious) user mistake. Otherwise this error is not actionable by user, who would rightly ask "yeah so why the d*mn thing >Could not find define arg? I've put it right there!").
templates/Dockerfile.common.sign.template line 21 at r9 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
What if user passes
--define remove_gramine_deps=false?. In this case we will have a nonempty string with value asfalse.
What if user passes --define remove_gramine_deps=True? or =1?
It's easier to explain to people that every non-empty string is true-ish than doing this "properly". By "properly" I mean introducing some more complicated function that folds the case and checks some common other variants like "on" and "yes". AFAIK there's no such standard function, everyone makes their own. For example, jinja itself has this internal helper in one of the extensions, but you can't use it directly, because it's not exported into the environment:
The problem is, you can't do it when parsing --define (because there may be other things to define that are not boolean, for example path to the signing key will be one, cf. #118). You'd have to do parsing here in template (and then probably error out if the option value is invalid). You can do this by implementing jinja test (https://jinja.palletsprojects.com/en/3.1.x/api/#writing-tests), so eventually you'd be able to write something like:
{% if args.remove_gramine_deps is trueish %}
{# ... #}
{% endif %}Quick draft if you prefer, totally untested:
def test_trueish(value):
value = value.casefold()
if not value or value in ('false', 'off', 'no'):
return False
if value in ('true', 'on', 'yes'):
return True
if value.isdigit():
return bool(int(value))
raise ValueError(f'invalid value for trueish: {value!r}')
env.tests['trueish'] = test_trueish
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 19 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @woju)
gsc.py line 133 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you rename this to
extract_define_args(args)? It makes more sense
Done.
gsc.py line 534 at r9 (raw file):
Previously, amathew3 (Abin Mathew) wrote…
Done
Sorry for marking it as 'Done'. The parse logic for --define fails if we set it to empty string. Either we need to modify the parse logic to accommodate empty key, or specify remove_gramine_deps=false as const for --no-remove-gramine-deps. For me setting remove_gramine_deps=false as const looks better.
gsc.py line 142 at r11 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I would really prefer if this said message that you need to use
=or that the syntax isNAME=VALUEor something else explaining how to correct the (to us obvious) user mistake. Otherwise this error is not actionable by user, who would rightly ask "yeah so why the d*mn thing >Could not find define arg? I've put it right there!").
Modified the message.
templates/Dockerfile.common.sign.template line 21 at r9 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
What if user passes
--define remove_gramine_deps=True? or=1?It's easier to explain to people that every non-empty string is true-ish than doing this "properly". By "properly" I mean introducing some more complicated function that folds the case and checks some common other variants like "on" and "yes". AFAIK there's no such standard function, everyone makes their own. For example, jinja itself has this internal helper in one of the extensions, but you can't use it directly, because it's not exported into the environment:
The problem is, you can't do it when parsing
--define(because there may be other things to define that are not boolean, for example path to the signing key will be one, cf. #118). You'd have to do parsing here in template (and then probably error out if the option value is invalid). You can do this by implementing jinja test (https://jinja.palletsprojects.com/en/3.1.x/api/#writing-tests), so eventually you'd be able to write something like:{% if args.remove_gramine_deps is trueish %} {# ... #} {% endif %}Quick draft if you prefer, totally untested:
def test_trueish(value): value = value.casefold() if not value or value in ('false', 'off', 'no'): return False if value in ('true', 'on', 'yes'): return True if value.isdigit(): return bool(int(value)) raise ValueError(f'invalid value for trueish: {value!r}') env.tests['trueish'] = test_trueish
Thanks for the inputs.. The logic is modified and tested.
There was a problem hiding this comment.
Reviewed 3 of 5 files at r10, 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @mkow, and @woju)
gsc.py line 141 at r12 (raw file):
defineargs_dict[key] = value else: #user specified --define with key and without value, exiting here.
Just remove this comment, it's obvious from the error message below.
gsc.py line 142 at r12 (raw file):
else: #user specified --define with key and without value, exiting here. print(f'Invalid value for argument `{item}`, Expected `--define {item}=True/False`')
Why True/False? It can be any value (this is a generic function you have here). So just ... {item}=<value>
gsc.py line 142 at r12 (raw file):
else: #user specified --define with key and without value, exiting here. print(f'Invalid value for argument `{item}`, Expected `--define {item}=True/False`')
Expected -> expected (small letter)
gsc.py line 532 at r12 (raw file):
sub_sign.add_argument('--remove-gramine-deps', action='append_const', dest='define', const='remove_gramine_deps=true', help='Remove Gramine dependencies that are not needed' 'at runtime.')
Please add a space before at:
' at runtime.')
Otherwise it will render as ...not neededat runtime.
gsc.py line 534 at r12 (raw file):
'at runtime.') sub_sign.add_argument('--no-remove-gramine-deps', action='append_const', dest='define', const='remove_gramine_deps=false', help='Retain Gramine dependencies that are not needed'
Remove trailing space
gsc.py line 535 at r12 (raw file):
sub_sign.add_argument('--no-remove-gramine-deps', action='append_const', dest='define', const='remove_gramine_deps=false', help='Retain Gramine dependencies that are not needed' 'at runtime.')
ditto (add space)
gsc.py line 541 at r12 (raw file):
sub_info.add_argument('image', help='Name of the graminized Docker image.') def test_trueish(value):
Please move this function to the very beginning of this file (before other functions). It will be easier for future readers this way (people typically read from top to bottom, so it makes more sense to have this helper function as the very first one).
templates/Dockerfile.common.sign.template line 30 at r12 (raw file):
USER {{app_user}} {% endif %}
Please remove empty line
There was a problem hiding this comment.
Reviewable status: 3 of 5 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @woju)
gsc.py line 141 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Just remove this comment, it's obvious from the error message below.
Done
gsc.py line 142 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why
True/False? It can be any value (this is a generic function you have here). So just... {item}=<value>
Done
gsc.py line 532 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a space before
at:' at runtime.')Otherwise it will render as
...not neededat runtime.
Done
gsc.py line 534 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Remove trailing space
Done
gsc.py line 535 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (add space)
Done
gsc.py line 541 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please move this function to the very beginning of this file (before other functions). It will be easier for future readers this way (people typically read from top to bottom, so it makes more sense to have this helper function as the very first one).
Done
templates/Dockerfile.common.sign.template line 30 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove empty line
Done
There was a problem hiding this comment.
Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3, @mkow, and @woju)
a discussion (no related file):
The PR looks good to me now. I'm blocking only on those comments where I requested a new PR that adds similar --define args to other GSC sub-commands. @amathew3 Either create such a PR, or create a GitHub issue that explains what will need to be done -- and then I resolve the rest of my blocking comments here.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @woju)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The PR looks good to me now. I'm blocking only on those comments where I requested a new PR that adds similar
--defineargs to other GSC sub-commands. @amathew3 Either create such a PR, or create a GitHub issue that explains what will need to be done -- and then I resolve the rest of my blocking comments here.
https://github.com/gramineproject/gsc/issues/150, created for tracking the similar --define args changes needed in gsc.py
gsc.py line 306 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@amathew3 Please create a new PR or GitHub issue that fixes this, and I'll resolve my comment here.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @woju)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @mkow)
gsc.py line 534 at r9 (raw file):
Sorry for marking it as 'Done'
No, why, this looks like it might be working. LGTM.
There was a problem hiding this comment.
Reviewed 3 of 5 files at r10, 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
a discussion (no related file):
Quick check after final rebase:
- Without
--remove-gramine-deps(default):
# cd /
# du -h -s -c
1015M total- With
--remove-gramine-deps(this PR):
# cd /
# du -h -s -c
797M totalI also tried --no-remove-gramine-deps and -Dremove_gramine_deps=1 options. Everything works.
gsc.py line 25 at r13 (raw file):
def test_trueish(value): value = value.casefold()
Had to fix this function, otherwise I had an error (when I don't specify --remove-gramine-deps) about dict object is NULL (smth like this).
There was a problem hiding this comment.
Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
gsc.py line 25 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Had to fix this function, otherwise I had an error (when I don't specify
--remove-gramine-deps) aboutdict object is NULL(smth like this).
Yes, obviously. My mistake.
There was a problem hiding this comment.
Reviewable status: 4 of 5 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
gsc.py line 25 at r13 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Yes, obviously. My mistake.
Fixed.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
gsc.py line 25 at r15 (raw file):
def test_trueish(value): if bool(value) is True:
So, this function can now return None?
There was a problem hiding this comment.
@amathew3: please never use push --force, but push --force-with-lease, it prevents such cases.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
There was a problem hiding this comment.
Reverted the new changes.
@mkow, thanks for pointing it out... Will start using --force-with-lease
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @amathew3 and @mkow)
a discussion (no related file):
@amathew3 Please don't do anything new on this PR, I took it over (again). We'll merge it soon.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)
gsc.py line 25 at r15 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
So, this function can now return
None?
Done (again)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r16.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @amathew3)
-- commits line 9 at r14:
The commit message got removed in the way, was this intended?
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
Previously, mkow (Michał Kowalczyk) wrote…
The commit message got removed in the way, was this intended?
Ok, seems to be a Reviewable bug, GitHub and git show the right commit message.
To remove these Gramine dependencies, a new command-line argument `--remove-gramine-deps` (alias to `-Dremove-gramine-deps=true`) is introduced. By default it is not set, since removing Gramine dependencies could also remove packages required by original app. This option is useful to minimize the final Docker image, in terms of the amount of installed software. Signed-off-by: abin <[email protected]>
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status:
a discussion (no related file):
Rebased again. Looks like Reviewable bug is gone now (I waited for a couple days, and in the meantime we heard nothing from Reviewable folks, and also the master branch was updated in this repo).
I hope Reviewable folks can still analyze what was the bug, by checking the previous revision which had problems (r16). For us, looks like we can proceed with final merge.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status:
Remove Gramine packages that are not needed at runtime. These packages are needed in build time and not needed in run time.
This change is