Skip to content

Hide loader.entrypoint manifest option#99

Merged
dimakuv merged 1 commit intomasterfrom
dimakuv/rm-loader-entrypoint
Oct 23, 2024
Merged

Hide loader.entrypoint manifest option#99
dimakuv merged 1 commit intomasterfrom
dimakuv/rm-loader-entrypoint

Conversation

@dimakuv
Copy link
Copy Markdown

@dimakuv dimakuv commented May 3, 2024

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

This commit is a counterpart to the commit "[Python,CI-Examples] Hide loader.entrypoint manifest option" in the core Gramine repo.


This change is Reviewable

Copy link
Copy Markdown
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 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @dimakuv)


-- commits line 8 at r1:

Suggestion:

[python,CI-Examples] Hide

@dimakuv dimakuv force-pushed the dimakuv/rm-loader-entrypoint branch from 1e44d2e to 25a25e2 Compare May 3, 2024 11:13
Copy link
Copy Markdown
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: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @mkow)


-- commits line 8 at r1:
Done.

Copy link
Copy Markdown
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, all discussions resolved, not enough approvals from maintainers (1 more required)

Copy link
Copy Markdown
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 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @dimakuv)

a discussion (no related file):
But it is clearly mentioned in our README -- "These examples are tested only against the most recent Gramine release (i.e., these examples are not guaranteed to work with older releases and unreleased versions of Gramine, including the latest master branch commits of Gramine).", does this imply that we need to hold this PR?


Copy link
Copy Markdown
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: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @mkow and @woju)

a discussion (no related file):

Previously, kailun-qin (Kailun Qin) wrote…

But it is clearly mentioned in our README -- "These examples are tested only against the most recent Gramine release (i.e., these examples are not guaranteed to work with older releases and unreleased versions of Gramine, including the latest master branch commits of Gramine).", does this imply that we need to hold this PR?

Hm, good point. I'm not sure what our policy should be. Technically, @kailun-qin is right, and as is currently written in the README, we should postpone this PR until right-before the next release.

Alternatively, we can change our README policy to be simpler: master Examples are guaranteed to work with master Gramine. If one wants to use a release of Gramine, they should use the same release in Examples. This is similar to how we tie GSC to Gramine. @woju @mkow @kailun-qin WDYT?


Copy link
Copy Markdown
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: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @mkow and @woju)

a discussion (no related file):

Alternatively, we can change our README policy to be simpler: master Examples are guaranteed to work with master Gramine. If one wants to use a release of Gramine, they should use the same release in Examples. This is similar to how we tie GSC to Gramine.

+1. I think we validate Examples on the corresponding Gramine before each release, so it should be fine that we state this way.


@mkow mkow requested a review from woju May 3, 2024 22:08
Copy link
Copy Markdown
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.

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @woju)

a discussion (no related file):

Previously, kailun-qin (Kailun Qin) wrote…

Alternatively, we can change our README policy to be simpler: master Examples are guaranteed to work with master Gramine. If one wants to use a release of Gramine, they should use the same release in Examples. This is similar to how we tie GSC to Gramine.

+1. I think we validate Examples on the corresponding Gramine before each release, so it should be fine that we state this way.

+1


Copy link
Copy Markdown
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: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @woju)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

+1

Created #101, please check it.


Copy link
Copy Markdown
Member

@woju woju 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: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @dimakuv and @kailun-qin)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Created #101, please check it.

This would be bad, because newbie users (those, who need examples in the first place) will install from packages or use docker image, and then will checkout this repo. So the repo should stay as is I think.


Copy link
Copy Markdown
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: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @kailun-qin and @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

This would be bad, because newbie users (those, who need examples in the first place) will install from packages or use docker image, and then will checkout this repo. So the repo should stay as is I think.

@woju What do you mean exactly? That we don't use this PR and thus don't remove loader.entrypoint from these examples?

At which point do you propose to resurrect this PR? After the next version of Gramine is released?


Copy link
Copy Markdown
Member

@woju woju 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: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @dimakuv and @kailun-qin)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@woju What do you mean exactly? That we don't use this PR and thus don't remove loader.entrypoint from these examples?

At which point do you propose to resurrect this PR? After the next version of Gramine is released?

Yes, after we release (package) something that allows to remove loader.entrypoint. Rationale is that novice users will install from packages, and then checkout the default branch (HEAD) of this repo.


Copy link
Copy Markdown
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: all files reviewed, 1 unresolved discussion (waiting on @kailun-qin and @woju)

a discussion (no related file):

Previously, woju (Wojtek Porczyk) wrote…

Yes, after we release (package) something that allows to remove loader.entrypoint. Rationale is that novice users will install from packages, and then checkout the default branch (HEAD) of this repo.

During the Gramine Tuesday meeting, it was decided to keep the current Examples policy as-is, and postpone merging this breaking change until after the next Gramine release is done.

In other words, we will follow @woju's advice.


Copy link
Copy Markdown
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: all files reviewed, 1 unresolved discussion (waiting on @kailun-qin and @woju)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

During the Gramine Tuesday meeting, it was decided to keep the current Examples policy as-is, and postpone merging this breaking change until after the next Gramine release is done.

In other words, we will follow @woju's advice.

New release of Gramine (v1.8) was done: https://github.com/gramineproject/gramine/releases/tag/v1.8

If I understand our policy correctly, now we can merge this PR, and afterwards I'll tag the Examples repo with v1.8 tag.


Copy link
Copy Markdown
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: all files reviewed, 1 unresolved discussion (waiting on @woju)

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

This commit is a counterpart to the commit "[python,CI-Examples] Hide
loader.entrypoint manifest option" in the core Gramine repo.

Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
Copy link
Copy Markdown
Member

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dimakuv dimakuv force-pushed the dimakuv/rm-loader-entrypoint branch from 25a25e2 to c9e6e4d Compare October 23, 2024 08:12
Copy link
Copy Markdown
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.

Reviewed 5 of 11 files at r1, 6 of 6 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dimakuv dimakuv merged commit c9e6e4d into master Oct 23, 2024
@dimakuv dimakuv deleted the dimakuv/rm-loader-entrypoint branch October 23, 2024 08:14
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.

4 participants