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

Default kiwix-serve's rootLocation to "/" #571

Merged
merged 2 commits into from Jul 28, 2022
Merged

Default kiwix-serve's rootLocation to "/" #571

merged 2 commits into from Jul 28, 2022

Conversation

rgaudin
Copy link
Member

@rgaudin rgaudin commented Jul 28, 2022

Usage mentions that the rootLocation defaults to / while it was not. It defaulted to an empty string.

While it defaulting to "" or "/" has no technical consequence: libkiwix's server normalizes the requested rootLocation ; it leads to users setting a "/"-prefixed rootLocation when customizing it, resulting in double-slashed print of the URL
to access kiwix-serve at.

This harmonizes usage and actual default, fixing the URL print.

Usage mentions that the rootLocation defaults to `/` while it was not.
It defaulted to an empty string.

While it defaulting to "" or "/" has no technical consequence: libkiwix's server
normalizes the requested rootLocation ; it leads to users setting a "/"-prefixed
rootLocation when customizing it, resulting in double-slashed print of the URL
to access kiwix-serve at.

This harmonizes usage and actual default, fixing the URL print.
@rgaudin rgaudin self-assigned this Jul 28, 2022
@kelson42 kelson42 requested review from veloman-yunkan and removed request for mgautierfr July 28, 2022 11:59
@kelson42 kelson42 added this to the 3.4.0 milestone Jul 28, 2022
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

This change assumes that --urlRootLocation must be specified as an absolute path (starting with /). Omitting the leading / will still work but the printed URL will be absolutely incorrect. Better just strip the leading / (if any) from rootLocation.

Reusing libkiwix's normalizeRootUrl() to display a more reliable representation of the
rootLocation the InternalServer will be using.

Can't reuse server.m_root as it is private.
@rgaudin
Copy link
Member Author

rgaudin commented Jul 28, 2022

This change assumes that --urlRootLocation must be specified as an absolute path (starting with /). Omitting the leading / will still work but the printed URL will be absolutely incorrect. Better just strip the leading / (if any) from rootLocation.

You're right. I've copied the normalizeRootUrl function from libkiwix (server.m_root is private) so we can print a more reliable rootLocation… What do you think?

@kelson42 kelson42 merged commit 092be45 into master Jul 28, 2022
@kelson42 kelson42 deleted the rootLocation branch July 28, 2022 17:11
mgautierfr added a commit that referenced this pull request Nov 30, 2022
 * Remove last reference to kiwix-read tool (@legoktm #569)

kiwix-serve
-----------

 * Fix broken indentation in usage (@kelson42 #560)
 * Exit if wrong arguments are passed (@kelson42 #567)
 * Do not allow multiple values for same option (@juuz0 #564)
 * Fix default location of "rootLocation" (@rgaudin #571)
 * [DOCKER] Change default port to 8080 (@neyder #581)
 * [DOCKER] Simplify dockerfile (@rgaudin #582)

kiwix-manage
------------

 * Fix man page (@kelson42 #576)
@mgautierfr mgautierfr mentioned this pull request Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants