Skip to content

WIP: Add nFPM backend for building apk, archlinux, deb, rpm packages#18914

Closed
cognifloyd wants to merge 74 commits intopantsbuild:mainfrom
cognifloyd:nfpm
Closed

WIP: Add nFPM backend for building apk, archlinux, deb, rpm packages#18914
cognifloyd wants to merge 74 commits intopantsbuild:mainfrom
cognifloyd:nfpm

Conversation

@cognifloyd
Copy link
Member

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Really nice!

I like the organization with fields in its own submodule, given there are so many of them, and for different targets. Makes for a nice and tidy structure imo.

Haven't read all the docstrings and help texts yet, but that shouldn't impact the structure of the code at all.. :)

continue

# handle nested fields (eg: deb.triggers)
keys = cast(_NfpmField, field).nfpm_alias.split(".")
Copy link
Member

Choose a reason for hiding this comment

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

rather than a cast here, aren't mypy satisfied if you declare a required_fields: ClassVar[Collection[_NfpmField]] in this fieldset class? (or does it complain about the type already being defined in the base class then?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh. I like that idea. I'll try that

Copy link
Member Author

Choose a reason for hiding this comment

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

I could also drop the protocol and do something like:

if not hasattr(field, "nfpm_alias"):
    continue

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't figure out how to make a type hint for required_fields that was compatible with the base class type and added the field. So, I went with hasattr and getattr.

class GoOS(Enum):
# GOOS possible values come from `okgoos` var at:
# https://github.com/golang/go/blob/go1.20.3/src/cmd/dist/build.go#L81-L98
# TODO: maybe filter this down to only what nFPM can handle
Copy link
Member

Choose a reason for hiding this comment

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

I'm torn. On one hand it would be bad to list a value that we know is going to fail, otoh a newer version of nFPM might add support for a value and then the user is blocked until a new release of this backend is out adding it..

cognifloyd added 27 commits May 25, 2023 12:37
Also, add type hints for some of the field.alias properties
because mypy was getting confused.
This is needed for the field_set calculations
A common field that all nfpm packages will share,
making it easier to build a common FieldSet base.
Almost all fields are "required" in that, if present, they are
used in the generation of the package.

Users do not have to supply them, but the fields have to exist
on the Nfpm*Package targets.
@cognifloyd
Copy link
Member Author

Closing in favor of #19308 which uses a branch I pushed to pantsbuild/pants instead of to my fork.

@cognifloyd cognifloyd closed this Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants