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

[Refactor]: following elixir conventions #57

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

Conversation

sleipnir
Copy link
Collaborator

@sleipnir sleipnir commented Mar 6, 2025

No description provided.

@sleipnir sleipnir marked this pull request as draft March 6, 2025 04:16
@pronesto pronesto requested a review from Dwctor March 6, 2025 13:55
@sleipnir
Copy link
Collaborator Author

sleipnir commented Mar 7, 2025

@Dwctor Can you tell me if the changes brought by this PR (which is still a draft) have any negative impact on the compilation process? I'm still trying to understand more complex parts of the code and it's not entirely clear how everything relates.

@Dwctor
Copy link
Collaborator

Dwctor commented Mar 8, 2025

@sleipnir So far there is no changes in the compilation process!

If you want to test it, make sure you have all of the dependencies and run: mix deps.get and mix on the root directory, then mix compile --force in the examples folder.

ps: glibc version 2.41 seems to be bugged, reverting to 2.40 fixes it.

Feel free to contact me as far as what parts relate to what others. I'll be more active starting next week.

ps2: if you are in an arch-based distro, please do not reboot/restart your machine before returning glibc to it's proper version, or you will have to chroot into it from a bootable pendrive to fix it!

@sleipnir
Copy link
Collaborator Author

Awesome @Dwctor
Thanks for your help

@sleipnir sleipnir marked this pull request as ready for review March 24, 2025 14:21
@sleipnir
Copy link
Collaborator Author

This PR reorganizes the project structure to better align with Elixir/OTP conventions while maintaining eBPF compiler specifics. Key improvements:

Why Elixir Conventions Matter
✅ Standard navigation: Familiar structure helps new contributors
✅ Mix integration: Proper task location (lib/mix/tasks) enables mix compile_bpf
✅ Namespacing: Logical module hierarchy (HoneyPotion.Compiler.Pipeline)
✅ Ecosystem alignment: Plays well with dialyzer, :kernel, and code formatters
✅ Future-proofing: Clear path for adding hex.pm dependencies

Key Changes:

  • Moved Mix tasks to standard location
  • Split monolithic modules into phased components
  • Renamed files for clarity (generator.ex → code_generator.ex)
  • Decomposed utils.ex into domain-specific helpers

Sorry for large PR

@sleipnir
Copy link
Collaborator Author

ping @Dwctor

@Dwctor
Copy link
Collaborator

Dwctor commented Mar 24, 2025

@sleipnir I'm looking over the changes now. Most things seem to be great! I'd just like you thoughts on these so we are on the same page:

  • What's the preference for BpfHelper over Bpf_helper?
  • Why is the (previous) Info module labeled Runtime?
  • What's the advantage of having Guard on Utils?

I don't agree with the following:

  • Liveliness analysis shouldn't be considered a semantic analysis (as far as I know), I suggest changing the name. Currently the Analyze module only does liveliness analysis, so naming it that and having another one that calls upon it (your Analysis would be a good fit) seems like the best idea

I won't be able to finish looking through everything today, so I'll leave more comments tomorrow, when I'll also test the compilation.

@sleipnir
Copy link
Collaborator Author

sleipnir commented Mar 24, 2025

Thanks for your quick response

@sleipnir I'm looking over the changes now. Most things seem to be great! I'd just like you thoughts on these so we are on the same page:

  • What's the preference for BpfHelper over Bpf_helper?

Answer: Naming conventions. It's very strange for an Elixir developer to use that syntax, given that it's a module and the default convention is CamelCase.

  • Why is the (previous) Info module labeled Runtime?
  • What's the advantage of having Guard on Utils?

Answer: Guards are still utilities, the proposal was Decomposed utils.ex into domain-specific helpers

I don't agree with the following:

  • Liveliness analysis shouldn't be considered a semantic analysis (as far as I know), I suggest changing the name. Currently the Analyze module only does liveliness analysis, so naming it that and having another one that calls upon it (your Analysis would be a good fit) seems like the best idea

Answer: The question is, will we be able to have other types of analysis in the future? If so, the name of the folder as I suggested makes the most sense. Regarding the name of the Liveliness module, I really have no idea what name would be best, but when I wrote it, it seemed to be part of a semantic analysis, if it is not then we can rename it as we see fit. Give me a name other than "Analysis" and I think it will be the best.

I won't be able to finish looking through everything today, so I'll leave more comments tomorrow, when I'll also test the compilation.

<3

@sleipnir
Copy link
Collaborator Author

  • Why is the (previous) Info module labeled Runtime?

This is only used by Honey Potion, therefore runtime, but the module name has not changed, only the folder so that it is not directly in the project root. Any name would work here I think, do you suggest one?

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

Successfully merging this pull request may close these issues.

2 participants