libbpf / bpftool

Automated upstream mirror for bpftool stand-alone build.
Other
377 stars 69 forks source link

Fix for issue#104 #111

Closed rameezrehman408 closed 5 months ago

rameezrehman408 commented 1 year ago

Weird indentation is corrected in all the .rst files.

Extra spaces at the start of the paragraphs are corrected. However, tabs/spaces in the examples (below '::') have been kept unchanged.

qmonnet commented 1 year ago

Heads up on the next steps, once the PR is ready:

qmonnet commented 1 year ago
  • We'll have to port it to the kernel repo (there won't be anything changing

That is, unless other patches are merged in the meantime. https://lore.kernel.org/all/20230829101838.851350-7-daan.j.demeyer@gmail.com/ looks like a good candidate for that.

rameezrehman408 commented 11 months ago

I am grateful for your detailed review earlier. I believe all of the suggestions have been included in this 2nd commit.

I am new to the open-source domain and very excited to have commits/PR that may get included upstream into the kernel. I deeply appreciate your time and guidance on this.

I am a bit unsure about submitting the patches and will look into the references you shared earlier.

I also apologize for the huge delay in the requested changes. I have been quite busy since my last commit, in my day job along with back-to-back trainings for security certs (ISMS LA & CISM for now) required by my employer.

Once again thank you for the opportunity and waiting anxiously as well as nervously for your kind review. :blush:

qmonnet commented 11 months ago

I am grateful for your detailed review earlier. I believe all of the suggestions have been included in this 2nd commit.

Yes, thanks, great work!

I am new to the open-source domain and very excited to have commits/PR that may get included upstream into the kernel. I deeply appreciate your time and guidance on this.

You're welcome, thanks for the work and for coming back to this - there's nothing complex in this PR but this is a big one and I do realise that I've been giving you a good amount of changes during the review.

I am a bit unsure about submitting the patches and will look into the references you shared earlier.

Yes please, take a look at the docs. The summary is as I described earlier. But I'm happy to help with the process and check before you send, if it's helpful.

I also apologize for the huge delay in the requested changes. I have been quite busy since my last commit, in my day job along with back-to-back trainings for security certs (ISMS LA & CISM for now) required by my employer.

This is entirely fine. We're all busy with stuff. I perfectly understand you're not 100% focusing on this PR. Neither am I - that's why you get a 2-day long delay for review :slightly_smiling_face:. Thanks for working on it and no worries about the time it takes, there's no rush. The most important is to do it well.

Once again thank you for the opportunity and waiting anxiously as well as nervously for your kind review. 😊

Oh dear, please don't feel anxious or nervous, that's not the objective! :slightly_smiling_face: I know the “Changes requested” may feel intimidating or frustrating because GitHub puts it in red, but that's just that really, some changes are necessary but the rest of the PR is great. Again I do realise I've raised a lot of things during the review, but since we're updating all bpftool docs, we should do it well, so I'm trying to be thorough!

We're nearly there, there are just a bunch of minor things to fix this time - aside from reverting the changes on the examples, maybe. I'd also recommend to rebase on the top of the master branch, although we may have more rebasing to do by the time we get the patch ready (there are at least two patch sets in flight with bpftool doc changes on the mailing list - nothing major, thankfully).

rameezrehman408 commented 11 months ago

Thanks for your review.

I have included the corrections you asked and created a commit 169871f.

I did rebase my local to the rameezrehman408:master before committing. However, I believe you meant to rebase from the libbpf:master, which I believe required synching and was visible under the "sync fork" button (showing the difference in no. of commits between both repos) as below. image

I have now merged the upstream/master and my master branch. I believe this was the thing that was required.

Requesting for your review. Thanks a lot.

qmonnet commented 11 months ago

I did rebase my local to the rameezrehman408:master before committing. However, I believe you meant to rebase from the libbpf:master, which I believe required synching and was visible under the "sync fork" button (showing the difference in no. of commits between both repos) as below.

Yes, I meant rebasing on top of upstream (libbpf/bpftool), sorry if this was unclear.

There's something wrong with your rebase though, it shouldn't change the libbpf version in use (this change). This is why the CI fails to run. Not really an issue because we'll send this patch to the ML anyway, but just so that you know.

qmonnet commented 5 months ago

Hi @rameezrehman408, any news on this PR? I'm seeing upstream contributions with people still tripping over the weird indentation in the file, and I'd like to get this fix out in the coming weeks/days.

Are you still interested in this? If not, would you mind me to pick it up and send it as joint work?

qmonnet commented 5 months ago

I've rebased as the last three commits on this branch, let me know if you're OK with me posting these.

rameezrehman408 commented 5 months ago

Dear @qmonnet,

I sincerely apologize for the delay, I have caused. I believe this branch is the most optimum way forward. I'd be more than glad to be a part of this contribution.

Thank you for your kind consideration and support.

qmonnet commented 5 months ago

https://lore.kernel.org/bpf/20240331200346.29118-1-qmo@kernel.org/T/#m60662ee17d95ab9f7fc02ac929fd816382966f52

qmonnet commented 5 months ago

Now merged upstream, I'll pull it to the GitHub mirror at the next sync-up. Thanks Rameez!