[python] Move from `toml` library to `tomli` + `tomli-w` #995

dimakuv · 2022-10-20T11:16:04Z

Description of the changes

toml Python library (for parsing and dumping TOML files) is buggy and doesn't support the full spec of TOML 1.0.0. This PR replaces it with more robust tomli (for parsing) and tomli-w (for dumping).

Useful links:

Closes #990. Fixes #968.

How to test this PR?

CI. One LibOS test was modified a bit to test more TOML cases.

This change is

dimakuv

Reviewable status: 0 of 10 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

.ci/ubuntu18.04.dockerfile line 90 at r1 (raw file):

    'sphinx_rtd_theme<1' \
    'tomli>=1.1.0' \
    'tomli-w>=1.0.0' \

FYI: These are not existing on Ubuntu 18.04 and 20.04. But it exists on Ubuntu 22.04. I guess @woju can take the packages from 22.04 when creating packages for Gramine v1.4?

Also, Python 3.11+ has tomli in its standard lib (under a new name tomllib). But there is no tomli-w, so I don't know if adding a switch like this would be benefial.

libos/test/regression/file_check_policy_strict.manifest.template line 33 at r1 (raw file):


  # test correct parsing of `\\x2d` sequence (previously used `toml` Python parser had a bug)
  { uri = "file:nonexisting\\x2dfile", sha256 = "0123456789012345678901234567890123456789012345678901234567890123" },

FYI: this is to test uiri/toml#404 and gramineproject/gsc#101.

python/graminelibos/manifest.py line 107 at r1 (raw file):

                  "https://gramine.readthedocs.io/en/latest/manifest-syntax.html#trusted-files")

        # Current toml versions (< 1.0) do not support non-homogeneous arrays

FYI: This comment doesn't seem to correspond to the Python logic below (the logic is needed, but this comment is irrelevant for tomli).

mkow

Reviewed 7 of 10 files at r1, 3 of 3 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 @woju)

a discussion (no related file):
Please remember to drop the hack from GSC after merging this PR.

-- commits line 4 at r2:
It isn't dumping the files, but the structs to files.

Suggestion:

generating TOML files

.ci/ubuntu18.04.dockerfile line 90 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

FYI: These are not existing on Ubuntu 18.04 and 20.04. But it exists on Ubuntu 22.04. I guess @woju can take the packages from 22.04 when creating packages for Gramine v1.4?

Also, Python 3.11+ has tomli in its standard lib (under a new name tomllib). But there is no tomli-w, so I don't know if adding a switch like this would be benefial.

Blocking until @woju replies.

libos/test/regression/file_check_policy_strict.manifest.template line 33 at r2 (raw file):


  # test correct parsing of `\\x2d` sequence (previously used `toml` Python parser had a bug)
  { uri = "file:nonexisting\\x2dfile", sha256 = "0123456789012345678901234567890123456789012345678901234567890123" },

I think this deserves to be a separate test. I don't like this packing of multiple unrelated tests into a single one, we can accidentally delete them because of this (e.g. when we decide to remove/change sgx.file_check_policy switch).

dimakuv

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 @mkow and @woju)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Please remember to drop the hack from GSC after merging this PR.

gramineproject/gsc#106 -- we can't drop the hack, at least not until the next stable release of Gramine. (GSC is supposed to be compatible with the last stable release, which is currently v1.3.1). But in the PR I linked, I added the FIXME at that hack.

-- commits line 4 at r2:

Previously, mkow (Michał Kowalczyk) wrote…

It isn't dumping the files, but the structs to files.

Will fix during final rebase.

libos/test/regression/file_check_policy_strict.manifest.template line 33 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think this deserves to be a separate test. I don't like this packing of multiple unrelated tests into a single one, we can accidentally delete them because of this (e.g. when we decide to remove/change sgx.file_check_policy switch).

A separate test that does what? I mean, what would the corresponding C source do?

mkow

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) (waiting on @dimakuv and @woju)

libos/test/regression/file_check_policy_strict.manifest.template line 33 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

A separate test that does what? I mean, what would the corresponding C source do?

It can do nothing, just an empty binary with a tricky manifest (we could move all weird/legacy manifest syntaxes testing there, right now we AFAIR have them spread throughout random tests).

dimakuv

Reviewable status: 9 of 18 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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @woju)

-- commits line 7 at r2:
Add a new para during final rebase:

New LibOS regression test is added that combines all tricky/legacy TOML syntax, including
the checks for TOML-syntax bugs.

.ci/ubuntu18.04.dockerfile line 90 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Blocking until @woju replies.

@woju ping

libos/test/regression/file_check_policy_strict.manifest.template line 33 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

It can do nothing, just an empty binary with a tricky manifest (we could move all weird/legacy manifest syntaxes testing there, right now we AFAIR have them spread throughout random tests).

Done, good idea.

libos/test/regression/file_check_policy_strict.manifest.template line 30 at r3 (raw file):

  # test TOML inline table syntax with `sha256` (trusted_testfile has hard-coded contents, so we can
  # use pre-calculated SHA256 hash)
  { uri = "file:trusted_testfile", sha256 = "41dacdf1e6d0481d3b1ab1a91f93139db02b96f29cfdd3fb0b819ba1e33cafc4" },

FYI: Note that I kept these syntaxes for TOML inline table syntax here, because it's impossible to combine the two syntaxes in one file (in one toml_parsing.manifest.template)

mkow

Reviewed 9 of 9 files at r3, all commit messages.
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 @dimakuv and @woju)

woju

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 @dimakuv and @mkow)

.ci/ubuntu18.04.dockerfile line 90 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@woju ping

@dimakuv I've added python3-tomli and python3-tomli-w to unstable-bionic and unstable-focal repositories. If you use Ubuntu 22.04, you don't need to do anything. If you use Debian 11, you need to install those from stable-backports distro repo. When we'll release, I'll move the packages to bionic and focal stable repos.

mkow

Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

dimakuv

Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

.ci/ubuntu18.04.dockerfile line 90 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

@dimakuv I've added python3-tomli and python3-tomli-w to unstable-bionic and unstable-focal repositories. If you use Ubuntu 22.04, you don't need to do anything. If you use Debian 11, you need to install those from stable-backports distro repo. When we'll release, I'll move the packages to bionic and focal stable repos.

Thanks!

`toml` Python library (for parsing and generating TOML files) is buggy and doesn't support the full spec of TOML 1.0.0. This commit replaces it with more robust `tomli` (for parsing) and `tomli-w` (for generating). New LibOS regression test is added that combines all tricky/legacy TOML syntax, including the checks for TOML-syntax bugs. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

dimakuv

Reviewable status: 13 of 18 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 @mkow)

-- commits line 4 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will fix during final rebase.

Done

-- commits line 7 at r2:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Add a new para during final rebase:

New LibOS regression test is added that combines all tricky/legacy TOML syntax, including
the checks for TOML-syntax bugs.

Done

mkow

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

dimakuv commented Oct 20, 2022

View reviewed changes

dimakuv force-pushed the dimakuv/python-tomli branch from 5619486 to 32312f7 Compare October 20, 2022 11:26

dimakuv mentioned this pull request Oct 20, 2022

Move from toml library to tomli + tomli-w gramineproject/gsc#106

Merged

mkow requested changes Nov 2, 2022

View reviewed changes

dimakuv commented Nov 3, 2022

View reviewed changes

mkow reviewed Nov 21, 2022

View reviewed changes

dimakuv commented Nov 22, 2022

View reviewed changes

mkow reviewed Nov 22, 2022

View reviewed changes

woju reviewed Nov 23, 2022

View reviewed changes

mkow previously approved these changes Nov 23, 2022

View reviewed changes

dimakuv commented Nov 24, 2022

View reviewed changes

dimakuv dismissed mkow’s stale review via 740e169 November 25, 2022 07:29

dimakuv force-pushed the dimakuv/python-tomli branch from 3620c24 to 740e169 Compare November 25, 2022 07:29

dimakuv commented Nov 25, 2022

View reviewed changes

mkow approved these changes Nov 26, 2022

View reviewed changes

dimakuv merged commit 740e169 into master Nov 26, 2022

dimakuv deleted the dimakuv/python-tomli branch November 26, 2022 21:07

dimakuv mentioned this pull request Apr 24, 2023

[LibOS,PAL] Add flexible device-specific IOCTL support #671

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[python] Move from `toml` library to `tomli` + `tomli-w` #995

[python] Move from `toml` library to `tomli` + `tomli-w` #995

dimakuv commented Oct 20, 2022 •

edited by mkow

Loading

dimakuv left a comment

mkow left a comment

dimakuv left a comment

mkow left a comment

dimakuv left a comment

mkow left a comment

woju left a comment

mkow left a comment

dimakuv left a comment

dimakuv left a comment

mkow left a comment

[python] Move from toml library to tomli + tomli-w #995

[python] Move from toml library to tomli + tomli-w #995

Conversation

dimakuv commented Oct 20, 2022 • edited by mkow Loading

Description of the changes

How to test this PR?

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

woju left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

[python] Move from `toml` library to `tomli` + `tomli-w` #995

[python] Move from `toml` library to `tomli` + `tomli-w` #995

dimakuv commented Oct 20, 2022 •

edited by mkow

Loading