Skip to content
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.

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

Allow more granular control on which artifacts are output #3131

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NekkoDroid
Copy link
Contributor

@NekkoDroid NekkoDroid commented Oct 17, 2024

This PR repurposes SplitArtifacts= to be able to specify multiple artifact types that should be split out of the image. By specifying one or more of uki, kernel, initrd and partitions the respective components are extracted from the image. The truthy and falsy values still map to the equivalent of the old values to preserve backwards compatibility.

@NekkoDroid NekkoDroid force-pushed the no-more-split-uki branch 3 times, most recently from 52ce4eb to e399187 Compare October 17, 2024 23:19
Copy link
Contributor

@behrmann behrmann left a comment

Choose a reason for hiding this comment

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

I like the refactor in the first commit (sans the style nits), but I'm a bit -0 on the second commit, since it breaks backwards compatibility and I'm not quite sure I understand why the superfluous artifacts cannot be deleted with a postoutput script, but I don't have too strong feelings there.

The backwards compat could probably be kept by making ArtifactsOutput a enum.Flag instead of a StrEnum, which would then probably look something like

class ArtifactOutput(enum.Flag):
    false = 0
    uki = 1
    kernel = 2
    initrd = 4
    partitions = 8
    true = 7

mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Show resolved Hide resolved
mkosi/resources/man/mkosi.1.md Outdated Show resolved Hide resolved
@NekkoDroid NekkoDroid force-pushed the no-more-split-uki branch 2 times, most recently from 4aa845e to 4fcb7f8 Compare October 18, 2024 09:26
@NekkoDroid
Copy link
Contributor Author

still looking into using enum.Flag, seems promising but I am still having a few problem with it :)

mkosi/config.py Fixed Show fixed Hide fixed
mkosi/config.py Fixed Show fixed Hide fixed
@NekkoDroid
Copy link
Contributor Author

NekkoDroid commented Oct 18, 2024

I am unable to figure out how to serialize the value to JSON and actually make it assign the value when returning from the parse function. When setting any value it still keeps resulting in the default (ArtifactOutput.no) when printing the summary

@NekkoDroid NekkoDroid force-pushed the no-more-split-uki branch 3 times, most recently from 6fc69f7 to e0dbde1 Compare October 18, 2024 11:38
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
@NekkoDroid NekkoDroid force-pushed the no-more-split-uki branch 2 times, most recently from 49b301c to caadcca Compare October 18, 2024 12:00
@NekkoDroid
Copy link
Contributor Author

NekkoDroid commented Oct 18, 2024

the JSON problem is "solved" (happy for any recommendation that makes it easier to understand than the raw flag number).

The issue with not being able to set the values in the config still persists though. (I was looking at the wrong config 😓)

@NekkoDroid NekkoDroid force-pushed the no-more-split-uki branch 2 times, most recently from d5abbf6 to be3875a Compare October 18, 2024 12:23
@NekkoDroid
Copy link
Contributor Author

Some CI fails due to dnf, but others seem to not be able to in an IntFlag :/ Unsure what is causing this

  Traceback (most recent call last):
    File "/home/runner/work/mkosi/mkosi/mkosi/run.py", line 64, in uncaught_exception_handler
      yield
    File "/home/runner/work/mkosi/mkosi/mkosi/run.py", line 105, in fork_and_wait
      target(*args, **kwargs)
    File "/home/runner/work/mkosi/mkosi/mkosi/__init__.py", line 4416, in run_build
      build_image(
    File "/home/runner/work/mkosi/mkosi/mkosi/__init__.py", line 3661, in build_image
      if ArtifactOutput.partitions in context.config.split_artifacts:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  TypeError: argument of type 'int' is not iterable

@behrmann
Copy link
Contributor

behrmann commented Oct 18, 2024 via email

@NekkoDroid NekkoDroid force-pushed the no-more-split-uki branch 7 times, most recently from 56bdc34 to 570ac0f Compare October 18, 2024 16:39
@NekkoDroid
Copy link
Contributor Author

Needing to use ArtifactOutput(split_artifacts) to get CI to be happy isn't the prettiest but I guess it gets the job done. If anyone knows what is causing it or how I could fix it other than what I did I'd like to hear.


def copy_uki(context: Context) -> None:
if (context.staging / context.config.output_split_uki).exists():
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just return the path here and then get rid of get_uki() and use copy_uki() everywhere instead? I'd prefer to have this be a single function instead of two.

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 would result in the uki always getting copied when it exists, even when it wasn't specified. Just conditioning the copy part and still return the path would feel a bit weird, but I could do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good as is with the name change. I don't think copy_uki shouldn't copy the uki in some scenarios, that would just be false advertising. I could see just always copying out the UKI and having the kernel and initrd be options in addition to the uki. I think that would also be fair, but I lean more towards the way it is now.

tests/test_json.py Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
A follow-up commit will introduce the ability to disable copying these
to the output directory, so refactor all the logic so that they are
contained within their respectiv functions.
@NekkoDroid NekkoDroid force-pushed the no-more-split-uki branch 2 times, most recently from 8545f80 to 239ff25 Compare October 20, 2024 11:45
This allows more precision on which artifacts are actually split out of
the image and placed into the output directory. Defaults to splitting
the UKI, vmlinuz and the initrd out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants