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

Add libbpf as a submodule #163

Merged
merged 1 commit into from
Feb 29, 2024
Merged

Conversation

jordalgo
Copy link
Contributor

This is to potentinally reduce issues with folks using different versions of libbpf at runtime.

Copy link
Contributor

@Byte-Lab Byte-Lab left a comment

Choose a reason for hiding this comment

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

Nice, thanks! This will hopefully make things way less annoying for people. LGTM, just left a couple comments before stamping.

README.md Outdated
$ cd $SCX
$ git submodule init
$ make -C libbpf/src
$ meson setup build --prefix ~ -D libbpf_a=$SCX/libbpf/src/libbpf.a
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this the default behavior, and instead allow people to override libbpf_a to something else?

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 makes sense. Will update

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but if we want to make this the default, it'd be nice if the build script would automatically run a git submodule update --init --recursive, so that it'll automatically fetch/update the submodule if it's not present (i.e. we could check if the dir libbpf/src esists).

This can prevent issues with people cloning the repo without git clone --recursive for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Ideally the build script can minimize the pain of working with submodules by hiding the details when possible.

libbpf Outdated Show resolved Hide resolved
@ptr1337
Copy link
Contributor

ptr1337 commented Feb 28, 2024

Could be this directly added into meson/scx add provide a option to use statically or dynamically linked one?

@jordalgo
Copy link
Contributor Author

@ptr1337 Probably my ignorance of meson, but I don't quite understand your comment. Isn't the libbpf_a user option deciding if libbpf is statically linked or not?

@ptr1337
Copy link
Contributor

ptr1337 commented Feb 28, 2024

@ptr1337 Probably my ignorance of meson, but I don't quite understand your comment. Isn't the libbpf_a user option deciding if libbpf is statically linked or not?

See Decave's comment

@Byte-Lab
Copy link
Contributor

Byte-Lab commented Feb 28, 2024

@ptr1337 Probably my ignorance of meson, but I don't quite understand your comment. Isn't the libbpf_a user option deciding if libbpf is statically linked or not?

I think what he's requesting is:

  • Make static linking the default behavior
    • By default, statically link against the submodule libbpf
    • Else, allow user to specify another library to statically link against. Honestly, not sure we even need this?
  • Else, allow user to dynamically link against libbpf.so

@jordalgo
Copy link
Contributor Author

@ptr1337 Probably my ignorance of meson, but I don't quite understand your comment. Isn't the libbpf_a user option deciding if libbpf is statically linked or not?

I think what he's requesting is:

  • Make static linking the default behavior

    • By default, statically link against the submodule libbpf
    • Else, allow user to specify another library to statically link against. Honestly, not sure we even need this?
  • Else, allow user to dynamically link against libbpf.so

Cool. I think I did this in my latest changes - please correct me if I missed something.

The last thing I'll do is have meson do the git submodule update --init --recursive as @arighi suggested.

@jordalgo
Copy link
Contributor Author

Actually, can we land this as is and I'll add the git submodule update --init --recursive in a follow up PR (mostly so I can test a fresh clone).

@jordalgo
Copy link
Contributor Author

@Decave @htejun - Ok to merge?

Copy link
Contributor

@Byte-Lab Byte-Lab left a comment

Choose a reason for hiding this comment

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

LGTM, if you wouldn't mind please fixing the one typo, I can merge

README.md Outdated
#### Dynamic linking against libbpf
```
$ cd $SCX
$ meson setup build --prefix ~-D libbpf_a=disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a space between ~ and -D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

This is to potentinally reduce issues with folks using
different versions of libbpf at runtime.
@Byte-Lab Byte-Lab merged commit 5b9b953 into sched-ext:main Feb 29, 2024
htejun added a commit that referenced this pull request Feb 29, 2024
This reverts commit 5b9b953, reversing
changes made to a7b39f2.

The current git submodule approach is a bit cumbersome and doesn't provide a
unified build environment for both libbpf and scx scheds. Also, the build
instruction doesn't seem to work. Let's revert it for now.
htejun added a commit that referenced this pull request Feb 29, 2024
Revert "Merge pull request #163 from jordalgo/libbpf-submodule"
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.

4 participants