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

define feature sets for make targets in files #1259

Merged
merged 4 commits into from Sep 21, 2022
Merged

Conversation

nkraetzschmar
Copy link
Collaborator

How to categorize this PR?

/kind enhancement
/area os
/os garden-linux

What this PR does / why we need it:

define feature sets for make targets in files, rather than having duplicate code in the Makefile and use generic make targets (% and %-dev) instead

features lists in make_targets are identical to those from the Makefile prior to this change, generated via the following script:

#!/bin/bash

set -Eeufo pipefail

makefile="$(cat Makefile)"

grep -F ': build-environment $(SECUREBOOT_CRT)' <<< "$makefile" \
| cut -d : -f 1 \
| grep -v '\-dev$' \
| while read -r target; do
	features="$(grep -F "$target"': build-environment $(SECUREBOOT_CRT)' -A 1 <<< "$makefile" \
	| tail -n 1 \
	| grep -Po '(?<=--features )[a-z0-9\-_,]+')"
	echo "$features" > "make_targets/$target"
done

@nkraetzschmar nkraetzschmar requested a review from a team as a code owner September 6, 2022 14:46
@gardener-robot gardener-robot added area/os enhancement Enhancement, improvement, extension labels Sep 6, 2022
@gardener-robot
Copy link

@nkraetzschmar Thank you for your contribution.

@gardener-robot
Copy link

@MrBatschner You have pull request review open invite, please check

@gyptazy
Copy link
Collaborator

gyptazy commented Sep 9, 2022

Sorry for sliding in... Currently, I'm rewriting the garden-feat in Python where the features are being evaluated.

When building any image, the overall base is represented by a feature of the platform type. This might be e.g. KVM.

According to this PR we define these sub features (see also: https://github.com/gardenlinux/gardenlinux/pull/1259/files#diff-c2604d0d6c1ce427dd5436aae130b8cd7bf9014be12d4ec807dd8318685ec608)

target: kvm
    features:
        - server
        - cloud
        - kvm

I think we do not need to maintain these ones within the Makefiles, while this gets already done by the include features.
Given by the base kvm, it includes cloud, which includes server, that includes base and finally includes _slim.

However, by just calling the make target with kvm we get:

target: kvm
    features:
        - cloud
        - server
        - base
        - _slim

AFAIK, this should be the case for all available platforms. This works by calling garden-feat during the build time and is implemented within the currently used Go version and the upcoming Python version.

To avoid further code, it might be enough to call the platform with the desired elements and flags (if not already provided by the include chain). However, this could result in:
kvm,cis,_dev
instead of:
kvm,cloud,server,cis,_dev
with the same result.

jfyi @nanory

@nkraetzschmar
Copy link
Collaborator Author

Yes, we can definitely slim down the list of features explicitly given for each make target 👍

For this change I just wanted to keep the feature lists exactly as they were in the Makefile to ensure we don't accidentally drop any. I'd suggest we merge this as is and slim down the list of features in a separate PR to keep each change more concise.

@gardener-robot
Copy link

@nkraetzschmar You need rebase this pull request with latest master branch. Please check.

in its currents state _readonly causes problems as described in https://github.com/gardenlinux/gardenlinux/issues/1285
as well as being incompatible with the firecracker platform
therefore for now it makes sense to have _prod flag without enabling _readonly
@gehoern gehoern merged commit 3c35cda into main Sep 21, 2022
@gehoern gehoern deleted the feat/makefile_targets branch September 21, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement, improvement, extension needs/review Needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants