Skip to content

Add build-sequence-summary.json #526

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Add build-sequence-summary.json #526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions e2e/test_build_order.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ $OUTDIR/sdists-repo/downloads/pbr-*.tar.gz
$OUTDIR/work-dir/logs/stevedore-*.log
$OUTDIR/work-dir/logs/setuptools-*.log
$OUTDIR/work-dir/logs/pbr-*.log

$OUTDIR/work-dir/build-sequence-summary.md
$OUTDIR/work-dir/build-sequence-summary.json
"

pass=true
Expand Down
15 changes: 15 additions & 0 deletions src/fromager/commands/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ class BuildSequenceEntry:
wheel_filename: pathlib.Path
skipped: bool = False

@staticmethod
def dict_factory(x):
return {
k: str(v) if isinstance(v, pathlib.Path | Version) else v for (k, v) in x
Copy link
Member

Choose a reason for hiding this comment

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

I would have expected the 2nd argument to be a tuple, but TIL about union type. Does that come from type hints?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my first approach - then my editor/pyright complained with the recommendation to use union type not tuple here. Looks like came in via https://peps.python.org/pep-0604/#isinstance-and-issubclass

Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks for sharing that link! I'm still learning about some of the recent changes to the language after spending a few years away from Python, so every little bit helps. :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The feature was introduced in Python 3.10 as part of PEP 604, https://docs.python.org/3.10/whatsnew/3.10.html#pep-604-new-type-union-operator

}

def __lt__(self, other):
if not isinstance(other, BuildSequenceEntry):
return NotImplemented
Expand Down Expand Up @@ -278,6 +284,15 @@ def _summary(ctx: context.WorkContext, entries: list[BuildSequenceEntry]) -> Non
console = rich.console.Console(file=f, width=sys.maxsize)
console.print(*output, sep="\n\n")

with open(ctx.work_dir / "build-sequence-summary.json", "w", encoding="utf-8") as f:
json.dump(
[
dataclasses.asdict(e, dict_factory=BuildSequenceEntry.dict_factory)
Copy link
Member

Choose a reason for hiding this comment

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

I think this could just be dataclasses.asdict(e). Did you find you needed to pass the factory function explicitly or does that do something extra?

Copy link
Contributor Author

@pnasrat pnasrat Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So both Version and pathlib.Path failed to serialize with errors like

ERROR: Object of type PosixPath is not JSON serializable

Using the custom dict_factory (set on the dataclass) here to use str for those worked. I debated switching to pydantic here, used else but this seemed sufficient given it's a report.

Copy link
Member

Choose a reason for hiding this comment

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

What you have looks good. Pydantic wouldn't be wrong, but is probably an extra complication here.

for e in entries
],
f,
)


def _create_table(entries: list[BuildSequenceEntry], **table_kwargs) -> Table:
table = Table(**table_kwargs)
Expand Down
Loading