Skip to content

net-mtools: Add new package to debug multicast setups#21496

Merged
BKPepe merged 1 commit intoopenwrt:masterfrom
Ansuel:add-mtools
Jul 7, 2023
Merged

net-mtools: Add new package to debug multicast setups#21496
BKPepe merged 1 commit intoopenwrt:masterfrom
Ansuel:add-mtools

Conversation

@Ansuel
Copy link
Member

@Ansuel Ansuel commented Jul 4, 2023

Add new package to debug multicast setups. This is required to use kselftests script for network testing.

net-mtools is used instead of mtools as it does conflicts with another package that is also called mtools.

@BKPepe
Copy link
Member

BKPepe commented Jul 5, 2023

net-mtools is used instead of mtools as it does conflicts with another package that is also called mtools.

Hmm. Shouldn't be there conflict in the Makefile for that package?

@Ansuel
Copy link
Member Author

Ansuel commented Jul 5, 2023

net-mtools is used instead of mtools as it does conflicts with another package that is also called mtools.

Hmm. Shouldn't be there conflict in the Makefile for that package?

mhhh no because i'm calling the package with a different name. Using the correct name and using the CONFLICT won't solve the problem that on package index we have 2 package with the same name. (with my testing using the same name resulted in only the current mtools compiled and this never parsed)

@BKPepe
Copy link
Member

BKPepe commented Jul 5, 2023

I see, mtools does not have these binaries, so that's okay. That's why I was confused in the first sight.

Anyway, if you look into test.sh files in this repository and add one to your related package, then it will be run tested inside CI/CD and it will be just fine.

@Ansuel
Copy link
Member Author

Ansuel commented Jul 5, 2023

@BKPepe do you have any example for test.sh? Also I'm sure how we can test this as it would require major setup like knowing the container network port and tcpdump to scan if the package is actually sent

@BKPepe
Copy link
Member

BKPepe commented Jul 5, 2023

Well, we do have pretty dump run testing check, but at least it is does something. It checks, if the binary prints the version.
Basically, something like this 3f00d28 will do the job.

However, it is not enabled still by default, thus adding test.sh manually is required (#13589 and #13785)

@Ansuel Ansuel force-pushed the add-mtools branch 7 times, most recently from ed22023 to 54bcf8d Compare July 6, 2023 12:22
@Ansuel
Copy link
Member Author

Ansuel commented Jul 6, 2023

@BKPepe should be ok now. Can you check?

@BKPepe
Copy link
Member

BKPepe commented Jul 7, 2023

I see run testing inside the CI looks great!

Installing net-mtools (2.3-1) to root...
Configuring net-mtools.
Use package specific test.sh
msend version 2.3
mreceive version 2.3
Test successful

Anyway, I do have one question, though. During my 1st round of review, I saw that you were using forked mtools repository, which is still left as URL, but you are downloading it from https://github.com/troglobit/mtools. Is there any reason for that?

Except from this one thing, which could be nitpicking, as I would prefer I think the forked repository (without patches here, fewer patches = less maintainance), it looks very good. Thank you @Ansuel for doing such good work and also for providing feedback during the reviews.

Add new package to debug multicast setups. This is required to use
kselftests script for network testing.

net-mtools is used instead of mtools as it does conflicts with another
package that is also called mtools.

Some additional patch from Vladimir Oltean are added to make the tool
works on kernel selftests scripts.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
@Ansuel
Copy link
Member Author

Ansuel commented Jul 7, 2023

Eh I forgot to update the other URL.

The project seems to be very old but it does have some release. And to have the CI test to work we need PKG_RELEASE.

So I thought it's better to use the original repo where the release were created and add the additional patch.

If it's a problem I can totally use the forked repo from Vladimir but it might be confusing and someone may think they are release from him (and the versioning from him) while in reality it's just forking the repo as using the original release tag.

But np, it's really a second for me to revert to the old link! Just tell me what do you prefer!

@BKPepe
Copy link
Member

BKPepe commented Jul 7, 2023

Nope, it is not an issue at all. I just asked about it. :)

It's up to you to decide what you want. I can live with both solutions.

@BKPepe
Copy link
Member

BKPepe commented Jul 7, 2023

Once CI/CD finishes its job, I will merge it.

@BKPepe BKPepe merged commit 60757f8 into openwrt:master Jul 7, 2023
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