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 <pbar...@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.gustafs...@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
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#140867): https://lists.openembedded.org/g/openembedded-core/message/140867 Mute This Topic: https://lists.openembedded.org/mt/75277396/21656 Group Owner: openembedded-core+ow...@lists.openembedded.org Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
