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

modularize features #52

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Conversation

timkenhan
Copy link

@timkenhan timkenhan commented Sep 19, 2023

Makes features (e.g. xfsprogs, e2fsprogs, plymouth, luks, etc) more self-contained thru the new GK_FEATURES variable

Some changes on the ebuild script will be necessary to accommodate this change.

To ensure working initramfs will always be generated, even without GK_FEATURES set, some basic functionalities such as busybox, firmware, and kernel modules will always be included.

I am aware this work is far from finish, and thus am open for input!

Some (minor) blockers:

  • I'm having issues separating NFS & netboot features since it's quite tightly coded with busybox stuff within the script
  • despite dmraid, lvm, luks, and multipath resides on their own, append_data 'devicemanager' "${DMRAID}" "${LVM}" "${LUKS}" "${MULTIPATH}" is on the main file.

@timkenhan
Copy link
Author

@reagentoo @FlyingWaffleDev feel free to toy around with this!

@reagentoo
Copy link

reagentoo commented Sep 20, 2023

Hi. Thank you for your effort!
What are the purposes of these changes?

  1. Reducing the initrd image size?
  2. Accelerate initrd image loading?
  3. Accelerating initrd image generation?
  4. Make code easier to understand?
  5. Improved code stability?
  6. Improve security?

@timkenhan
Copy link
Author

I say making the code easier to understand, especially for adding new features. For example, in the recent implementation of Plymouth feature, @FlyingWaffleDev would need to figure out where to put what within multiple files of which each file can have hundreds of lines. With this change, only one file would need to be added.

The user (or the packager) can also choose which features to have in the genkernel executables. I intend to pair this with USE flag to give Gentoo users ability to pick and choose.

@reagentoo
Copy link

If you are saying only about code easier to understand, why did you make the code generation?

@timkenhan
Copy link
Author

It's not only about making the code easier to understand. It's also about giving users ability to pick and choose features on their executables.

@timkenhan
Copy link
Author

As for the readability, I am making the source files less monolithic, thus letting developers able to do their work without sifting thru hundreds (or thousands) lines of code.

@reagentoo
Copy link

It's also about giving users ability to pick and choose features on their executables.

Does this "choising ability for users" improves at least 1 point out of those six? #52 (comment)

As for the readability, I am making the source files less monolithic, thus letting developers able to do their work without sifting thru hundreds (or thousands) lines of code.

I don't agree. Any genkernel's shell file within 100kb.

@timkenhan
Copy link
Author

i guess it does, but not directly (since if an option is included, it can still be turned off).

100kb is massive for a textfile. Some of the scripts would reach 1k loc.

it is also more prone to conflict we have fewer big files, as compared with many small files.

@thesamesam
Copy link
Member

This should at least let us make some of the large distfiles in the ebuild optional too as they'll be real build system options rather than useless USE flags just toggling a download.

@reagentoo
Copy link

This should at least let us make some of the large distfiles in the ebuild optional too as they'll be real build system options rather than useless USE flags just toggling a download.

To implement this, it is not necessary to do code generation and divide normal-sized sources into a bunch of small files.

@thesamesam
Copy link
Member

This should at least let us make some of the large distfiles in the ebuild optional too as they'll be real build system options rather than useless USE flags just toggling a download.

To implement this, it is not necessary to do code generation and divide normal-sized sources into a bunch of small files.

It is to have USE flags which aren't there for the sake of it. But besides, I agree with the general splitting up of things here. The files are huge and dividing things up (forcing us to think about their interface) is not a bad thing.

@reagentoo
Copy link

reagentoo commented Sep 27, 2023

forcing us to think about their interface

There is no need to do such code generation for the interface (using awk 🤦‍♂️).
It is enough to use smth like:

source ${module}
interface_func_${module}
interface_var+=( "item" )

@thesamesam
Copy link
Member

forcing us to think about their interface

There is no need to do such code generation for the interface (using awk 🤦‍♂️). It is enough to use smth like:

I suggest making more constructive, polite comments.

source ${module}
interface_func_${module}

Sure, propose that then.

@timkenhan
Copy link
Author

@reagentoo: If you read the awk script, you'd see the reason I did the way I did was so that we can generate any section with the same piece of code. There's also no need to install an extra package since awk is required for most system packages anyway. Can I ask what you mean by "no need for the code generation"? What is wrong with awk?

@reagentoo
Copy link

Can I ask what you mean by "no need for the code generation"?

Are there really no other approaches to creating an interface and modules?

What is wrong with awk?

Can you give an examples of a good practice for using awk in this way? Worse than awk - only perl.

@timkenhan
Copy link
Author

Are there really no other approaches to creating an interface and modules?

I guess we can have the master code calling the module code, but that would take further approach. We need to start with some degree of separation to move forward. I'd be open to suggestion on the implementation detail.

Can you give an examples of a good practice for using awk in this way? Worse than awk - only perl.

Please answer the question, what is wrong with awk? I simply need a tool for processing text, and awk simply fulfils my need with its specialized language for text processing. Once again, I am open for suggestion.

@timkenhan
Copy link
Author

@thesamesam lemme know if you have other feedbacks :)

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
Comment on lines 132 to 135
out/genkernel.conf: out/build-config
cat genkernel.conf | sed \
-e '/#BEGIN FEATURES genkernel_conf/ r out/temp/genkernel_conf' \
> out/genkernel.conf
Copy link

Choose a reason for hiding this comment

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

This won't trigger properly on updates.
If I edit something in genkernel.conf and run make again, out/genkernel.conf will not be regenerated.

It's also unclear why it depends on out/build-config when it does NOT consume that file at all.

out/genkernel.conf: genkernel.conf out/temp/genkernel_conf
    cat $< | sed -e '/#BEGIN FEATURES genkernel_conf/ r out/temp/genkernel_conf' >$@

The same issue exists for many of your other targets.

I suggest creating a helper script that supports every transformation you've coded into the Makefile, and then simplifying the makefile:

out/genkernel.conf: genkernel.conf out/build-config $(INTERMEDIATE_DEPS) helper.sh
  ./helper.sh < $< > $@

and then you can see how it's easy to convert that rule further (this is a little harder without changing the layout to have the source files in a distinct directory).

out/%.conf: %.conf out/build-config $(INTERMEDIATE_DEPS) helper.sh
  ./helper.sh < $< > $@

Copy link
Author

Choose a reason for hiding this comment

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

I created the build-config despite not being directly consumed because i was having problem getting the proper dependencies to register for preventing rebuilds. I thought about having separate dependencies for the temp files, but they are all processed at once within the awk script.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
install: BINDIR := $(file <out/BINDIR)
install: SYSCONFDIR := $(file <out/SYSCONFDIR)
install: MANDIR := $(file <out/MANDIR)
install: all
Copy link

Choose a reason for hiding this comment

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

missing deps here

Copy link
Author

Choose a reason for hiding this comment

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

can you elaborate on this? i've put the deps on top of file

@robbat2
Copy link

robbat2 commented Oct 10, 2023

Overall I like the concept you're proposing here. I'm worried about the long-term maintainability of the split implementation. One immediate loss is that we can no longer quickly do shellcheck to detect bugs - because it can only check the output files.

Make also isn't suited to this type of text munging. M4 might work, but I wouldn't wish it on anybody.

@timkenhan
Copy link
Author

@robbat2 Thank you for your massive list of feedback! This should deepen my understandin of Makefiles. I'll address them as soon as I can.

What we have here is basically the proof of concept to demonstrate that this can work. To be honest, I find the implementation itself to have a long way to go. The feature file format is far from perfect, but it is one I could come up with. Let me know if you have better ideas!

@timkenhan
Copy link
Author

hi @robbat2, I've made some restructuring changes on the features. I hope that this would suffice to address your concerns above.

@thesamesam
Copy link
Member

Robin's travelling until the end of the month btw (https://dev.gentoo.org/devaway/)

@timkenhan
Copy link
Author

Thanks for the heads-up, @thesamesam!

@timkenhan
Copy link
Author

@thesamesam @robbat2 any word on how we can proceed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants