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

[REGRESSION] kiwix-serve URL helper does not display iPv4 URL anymore #703

Open
kelson42 opened this issue Sep 9, 2024 · 10 comments · May be fixed by #704 or kiwix/libkiwix#1132
Open

[REGRESSION] kiwix-serve URL helper does not display iPv4 URL anymore #703

kelson42 opened this issue Sep 9, 2024 · 10 comments · May be fixed by #704 or kiwix/libkiwix#1132
Assignees
Labels
Milestone

Comments

@kelson42
Copy link
Contributor

kelson42 commented Sep 9, 2024

With cutting-edge dev kiwix-serve the URL help to join the server via HTTP is not anymore really what it should.

I get:

$ kiwix-serve --port=8081 /home/kelson/Downloads/*zim
The Kiwix server is running and can be accessed in the local network at: http://[::1]:8081

My system has both IPv4 AND IPv6, so both should be displayed in the helper. It should look like (if -i=all or default):

$ kiwix-serve --port=8081 /home/kelson/Downloads/*zim
The Kiwix server is running and can be accessed in the local network at: http://127.0.0.1:8081 or http://[::1]:8081
@kelson42 kelson42 added the bug label Sep 9, 2024
@kelson42 kelson42 added this to the 3.8.0 milestone Sep 9, 2024
@kelson42
Copy link
Contributor Author

kelson42 commented Sep 9, 2024

@sgourdas Can you please fix quickly this small regression?

@sgourdas
Copy link
Collaborator

sgourdas commented Sep 11, 2024

My system has both IPv4 AND IPv6, so both should be displayed in the helper. It should look like (if -i=all or default):

@kelson42 default (no -i parameter given), with the current implementation, just straight up uses only ipv4. There is no check of how the system is configured to determine what to use. Do we want to change that and check what the system supports and go based on that?

Edit: Let me get back to this.

@kelson42
Copy link
Contributor Author

just straight up uses only ipv4

I don't want to change the default behaviour, but it's not normal that I get a helper with an IPv6 URL if IPv4 is used!

@sgourdas
Copy link
Collaborator

Yeah I am investigating further. Disregard what I said above for now. Will get back ASAP.

@sgourdas
Copy link
Collaborator

@kelson42 just to confirm, because this needs refactoring on libkiwix's side as well, what we want to be displayed is all the available urls that the server is running on. Just like the example you gave, if attached to both ipv4 and ipv6 with --address=all the output should give:

The Kiwix server is running and can be accessed in the local network at: http://127.0.0.1:8081 or http://[::1]:8081

Or, other example, if --address=all is used but ipv6 is not configured only the ipv4 host should be displayed. And so on.

@kelson42
Copy link
Contributor Author

@sgourdas I just ask that the helper line printed by kiwix-serve is phrased according to the --address option.

What is clear to me is that if the user requests something which can not be delivered (like listening on an inexisting IPv6 address) then an error should probably be triggered.

I would be surprised that any of this should require a refactoring.

@sgourdas
Copy link
Collaborator

@sgourdas I just ask that the helper line printed by kiwix-serve is phrased according to the --address option.

What is clear to me is that if the user requests something which can not be delivered (like listening on an inexisting IPv6 address) then an error should probably be triggered.

I would be surprised that any of this should require a refactoring.

Do we want to blindly print hosts just based on the 'address' variable? If yes, that is easy. If we want to be sure what we print works and is correct we need to refactor 'getBestPublicIp()' from libkiwix.

@kelson42
Copy link
Contributor Author

@sgourdas I would recommend to open a dedicated issue with all the details if this needs serious rewriting.

@sgourdas
Copy link
Collaborator

@kelson42 I will send a PR within today attached to this issue and we can evaluate then maybe.

@sgourdas
Copy link
Collaborator

@kelson42 I made these PRs which are up for discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: REVIEW
2 participants