azurelinux icon indicating copy to clipboard operation
azurelinux copied to clipboard
verified publisher icon microsoft

[RFC] tools: refactor all binary packages as library packages

Open mfrw opened this issue 3 years ago • 3 comments

Request for Comments

Closes: #2982

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • [x] The toolchain has been rebuilt successfully (or no changes were made to it)
  • [x] The toolchain/worker package manifests are up-to-date
  • [x] Any updated packages successfully build (or no packages were changed)
  • [x] Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • [x] All package sources are available
  • [x] cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/tools/cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • [x] LICENSE-MAP files are up-to-date (./SPECS/LICENSES-AND-NOTICES/data/licenses.json, ./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md, ./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)
  • [x] All source files have up-to-date hashes in the *.signatures.json files
  • [x] sudo make go-tidy-all and sudo make go-test-coverage pass
  • [x] Documentation has been updated to match any changes to the build system
  • [ ] Ready to merge
Summary

What does the PR accomplish, why was it needed? In the current form, all the code in the binary packages is not re-useable to enable building on top of the great work we have. This PR abstracts all the binary packages as library packages which can be imported in other packages as well to enable code-reuse. The changes in this PR are 100% backward compatible and it does not break anything. To achieve this, the path of least resistance seemed to expose a Config structure from the library package and pass all the relevant details that are passed as flags.

Change Log
  • tools: srpmpacker: refactor as a lib instead of a main package
  • tools: specreader: refactor as a lib instead of a main package
  • tools: pkgworker: refactor as a lib instead of a main package
  • tools: graphpkgfetcher: refactor as a lib instead of main package
  • tools: grapher: refactor as a lib instead of main package
  • tools: graphpreprocessor: refactor as a lib instead of main package
  • tools: mv imagegen -> pkg/imagegen
  • tools: imageconfigvalidator: refactor as a lib instead of main package
  • tools: imagepkgfetcher: refactor as a lib instead of main package
  • tools: imager: refactor as a lib instead of main package
  • tools: isomaker: refactor as a lib instead of main package
  • tools: roast: refactor as a lib instead of main package
  • tools: scheduler: refactor as a lib instead of main package
  • tools: liveinstaller: refactor as a lib instead of main package
  • tools: validatechroot: refactor as a lib instead of main package
  • tools: boilerplate: remove un-needed dir
Does this affect the toolchain?

YES

Associated issues
  • #2982
Links to CVEs
  • NA
Test Methodology
  • Local Build

mfrw avatar May 11 '22 09:05 mfrw

I love the clean-up done here but would it be possible to split this change into smaller chunks? It's somewhat challenging to review all of this at once while maintaining full focus.

The individual commits in the PR are the smaller changes that achieve it. Given this is such a big refactor, the reason for sending this huge PR is to chunk it as a logical change. In my opinion, when we merge this, we should do a merge commit rather than a squash-and-merge to save the history.

mfrw avatar May 19 '22 01:05 mfrw

I left a few comments. The ones I'm mostly interested in:

  • It seems we're not entirely consistent in how we deal with logger.log.Panic. In most cases we've left them unchanged, which seems fine, since we don't want to change the logic, just refactor, but there's at least one commit where I've seen them converted to logger.log.Info.
  • Related to the above comment: it's insanely difficult to go through all the changes and make sure the logic was not modified. Since the granularity of accepted changes is a whole PR, not separate commits, I'm finding it hard to maintain enough focus to feel safe about approving the whole thing. I'd strongly recommend splitting this into separate PRs to ease with te review process.

I appreciate the feedback comments. I think Yes, Let me pluck them one-by-one and try to create seperate PRs for an individual tool, using the same PR. I agree that this is a ginormous PR and reviewing it while keep full focus and not breaking anything is difficult :)

Again appreciate all the reviewers spending time on this PR.

mfrw avatar Aug 19 '22 04:08 mfrw

I left a few comments. The ones I'm mostly interested in:

  • It seems we're not entirely consistent in how we deal with logger.log.Panic. In most cases we've left them unchanged, which seems fine, since we don't want to change the logic, just refactor, but there's at least one commit where I've seen them converted to logger.log.Info.
  • Related to the above comment: it's insanely difficult to go through all the changes and make sure the logic was not modified. Since the granularity of accepted changes is a whole PR, not separate commits, I'm finding it hard to maintain enough focus to feel safe about approving the whole thing. I'd strongly recommend splitting this into separate PRs to ease with te review process.

I appreciate the feedback comments. I think Yes, Let me pluck them one-by-one and try to create seperate PRs for an individual tool, using the same PR. I agree that this is a ginormous PR and reviewing it while keep full focus and not breaking anything is difficult :)

Again appreciate all the reviewers spending time on this PR.

Thanks for your understanding, @mfrw! I understand this PR is an immense amount of work on your end. It looks super helpful and IMO close to done, so I'm going to keep an eye out for your updates and help with the review as much as possible.

PawelWMS avatar Aug 22 '22 20:08 PawelWMS