Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Python,CI-Examples] Hide loader.entrypoint manifest option #1716

Merged
merged 1 commit into from
May 3, 2024

Conversation

dimakuv
Copy link
Contributor

@dimakuv dimakuv commented Jan 11, 2024

Description of the changes

For end users, the loader.entrypoint manifest option is always the LibOS binary. Similarly, sgx.trusted_files must always contain the LibOS binary.

To preserve special cases like PAL tests (where loader.entrypoint points to the PAL test binary), we allow special loader.entrypoint. In this case, the LibOS binary is not appended to sgx.trusted_files.

Rationale

I was asked too many times by casual users what the hell this is. I don't see any point in not allowing this info to go implicit.

So, it's a small user-friendliness improvement.

TODOs

If folks agree that it's a good change, then I'll do the same for:

  • Examples repo,
  • GSC repo -- actually no need, since GSC templates are not user-facing.

How to test this PR?

Changes CI-Examples.


This change is Reviewable

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 22 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

a discussion (no related file):
As it's user facing, maybe we can discuss this and #1717 in our next meeting (for quicker and wider feedback)?


Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 22 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)

a discussion (no related file):

Previously, kailun-qin (Kailun Qin) wrote…

As it's user facing, maybe we can discuss this and #1717 in our next meeting (for quicker and wider feedback)?

Done, see the updated meeting agenda: #1713

Blocking here until we discuss in the meeting.


Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 22 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) (waiting on @kailun-qin)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done, see the updated meeting agenda: #1713

Blocking here until we discuss in the meeting.

Done, we discussed and it was accepted.


Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 22 files at r1, 20 of 20 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @kailun-qin)


Documentation/manifest-syntax.rst line 73 at r2 (raw file):

   loader.entrypoint = "[URI]"
    (Default: "<path to libsysdb.so>")

Suggestion:

   loader.entrypoint = "[URI]"
   (Default: "<path to libsysdb.so>")

Documentation/manpages/gramine-manifest.rst line 107 at r2 (raw file):

.. code-block:: jinja

   loader.entrypoint = "file:{{ gramine.libos }}"

Shouldn't we drop it from here?


python/graminelibos/manifest.py line 350 at r2 (raw file):

        loader = manifest.setdefault('loader', {})
        if 'entrypoint' not in loader:
            loader['entrypoint'] = f"file:{_env.globals['gramine']['libos']}"

Suggestion:

loader['entrypoint'] = f'file:{_env.globals["gramine"]["libos"]}'

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 19 of 22 files reviewed, 5 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 @kailun-qin and @mkow)

a discussion (no related file):
Putting a blocking comment to not forget to do the same for the Examples repo (see also the checkbox in the PR desc)



Documentation/manifest-syntax.rst line 73 at r2 (raw file):

   loader.entrypoint = "[URI]"
    (Default: "<path to libsysdb.so>")

Done.


Documentation/manpages/gramine-manifest.rst line 107 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Shouldn't we drop it from here?

Yeah, I think so. It's an example manifest file, so it should be aligned with the best practices and other example files.


python/graminelibos/manifest.py line 350 at r2 (raw file):

        loader = manifest.setdefault('loader', {})
        if 'entrypoint' not in loader:
            loader['entrypoint'] = f"file:{_env.globals['gramine']['libos']}"

Done.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 22 files at r1, 17 of 20 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners

For end users, the `loader.entrypoint` manifest option is always the
LibOS binary. Similarly, `sgx.trusted_files` must always contain the
LibOS binary.

To preserve special cases like PAL tests (where `loader.entrypoint`
points to the PAL test binary), we allow special `loader.entrypoint`. In
this case, the LibOS binary is not appended to `sgx.trusted_files`.

Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
@mkow mkow force-pushed the dimakuv/rm-loader-entrypoint branch from ecfdda1 to 4fa74d8 Compare May 2, 2024 23:50
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Putting a blocking comment to not forget to do the same for the Examples repo (see also the checkbox in the PR desc)

gramineproject/examples#99 -- done


@mkow mkow merged commit 4fa74d8 into master May 3, 2024
18 checks passed
@mkow mkow deleted the dimakuv/rm-loader-entrypoint branch May 3, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants