Re: [PATCH v4 01/12] Move rpm manifest to its own subdir

["Fredrik Gustafsson" <fredrik.gustafsson@axis.com>]

Hi Paul,
thank you for your review, I hope you're feeling better!
I think I've managed to fix all your comments.
Some imports was not possible to move to the top of the file but those
who where possible to move I've moved.

Please note that I'll be away from monday and four weeks forward. Please
don't take my silence then as a lack of interest. This has my highest priority.

Best regards
Fredrik
________________________________________
From: Paul Barker <pbarker@konsulko.com>
Sent: Saturday, July 18, 2020 5:29 PM
To: Fredrik Gustafsson
Cc: openembedded-core@lists.openembedded.org; tools-cfpbuild-internal; Richard Purdie
Subject: Re: [PATCH v4 01/12] Move rpm manifest to its own subdir

On Tue, 14 Jul 2020 at 08:02, Fredrik Gustafsson
<Fredrik.Gustafsson@axis.com> wrote:
>
> Thank you, just let me know if I can make it any easier to review this,
> I know it's a big patch.

Hi Fredrik,

Sorry for the delays here, I've been unable to get back to this until
today due to other commitments and a few health issues.

Here's my feedback on the series as a whole. Overall I think it's
excellent, I much prefer the result where the code is grouped by
package manager (opkg, dpkg or rpm) to the existing code where it's
spread across package_manager.py, rootfs.py, manifest.py and sdk.py.

1) Please edit the first line of each commit message to follow the
style guide at https://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines.

2) Please also fix the following whitespace errors reported when
applying these patches:

$ git am ../v4-Move-rpm-manifest-to-its-...-and-11-more.mbox
Applying: Move rpm manifest to its own subdir
.git/rebase-apply/patch:148: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: Move ipk manifest to its own subdir
.git/rebase-apply/patch:187: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: Move deb manifest to its own subdir
Applying: Move rpm rootfs to its own dir
.git/rebase-apply/patch:161: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: Move ipk rootfs to its own dir
Applying: Move deb rootfs to its own dir
.git/rebase-apply/patch:222: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: Move rpm sdk to its own dir
.git/rebase-apply/patch:127: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: Move ipk sdk to its own dir
.git/rebase-apply/patch:42: trailing whitespace.
self.d.getVar("ALL_MULTILIB_PACKAGE_ARCHS"),
.git/rebase-apply/patch:109: new blank line at EOF.
+
warning: 2 lines add whitespace errors.
Applying: Move deb sdk to its own dir
.git/rebase-apply/patch:108: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: Move rpm package manager to its own dir
.git/rebase-apply/patch:840: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: Move ipk package manager to its own dir
.git/rebase-apply/patch:951: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: Move deb package manager to its own dir
.git/rebase-apply/patch:1042: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

3) I'd recommend the new module keeps the name package_manager. You
can do this by moving the existing code into
package_manager/__init__.py and then splitting it up inside that
directory. Imports and wrappers in the __init__.py file can be used to
ensure no other bit of the code needs to worry about the changes.

4) Please avoid unnecessarily using imports inside functions. In
rootfs.py, manifest.py and sdk.py you can import the required classes
at the top level. Functions like create_rootfs(), populate_sdk(), etc
should not require any changes as part of this patch series.

I'll run as much of the selftest suite as I can on this series as-is
to see if there are any other issues.

Thanks,

--
Paul Barker
Konsulko Group