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

Distance table plugin, set maximum size of input before truncation from cmake #1077 #1236

Conversation

frodrigo
Copy link
Member

Distance table plugin, set maximum size of input before truncation from cmake #1077

@emiltin
Copy link
Contributor

emiltin commented Oct 22, 2014

Wouldn't it be better to fetch the maximum from a command line option at run time, rather than set it at compile time? Ex:

osrm-routed --max-table-size 1000

@frodrigo
Copy link
Member Author

Maybe. It was my first intent, but not sure if a patch doing it would be accepted. So, I save my time and try the simplest way.

@emiltin
Copy link
Contributor

emiltin commented Oct 22, 2014

then start by asking :-)

@frodrigo
Copy link
Member Author

I previously asked on IRC and here #544
So, the answer would be yes ?

@emiltin
Copy link
Contributor

emiltin commented Oct 22, 2014

as i'm not an official maintainer, i cannot tell you if it would be accepted. @DennisOSRM?

@DennisOSRM
Copy link
Collaborator

I am a bit undecided here. The change is fairly easy, but I am not sure if this should be a compile-time parameter.

@frodrigo
Copy link
Member Author

Runtime parameter allow a more wide usage when osrm is packaged

@emiltin
Copy link
Contributor

emiltin commented Oct 22, 2014

it seems tedious having to recompile just to change a setting

@alex85k
Copy link
Contributor

alex85k commented Oct 22, 2014

Maybe the environment variable? getenv("OSRM_TABLE_MAX_LOCATIONS") somewhere in constructor (with the default value, of course)...

@DennisOSRM
Copy link
Collaborator

@alex85k which is more evil as it circumvents the CMake build script 😀

@woodbri
Copy link

woodbri commented Oct 22, 2014

I'm +1 on the command line option which a default value set to whatever you think is usable. The use case for this is that eventually this will get picked up by package managers for various distributions. This makes it much easier for users to use it output of the box. Many systems do not allow users to compile code on the server and some users don't know how to. Make it easy for the commodity user to install, configure and go. Developers and hackers can always compile and tweak code to fit their needs.

@ZupoLlask
Copy link

+1 on the command line option with a default value!

@DennisOSRM
Copy link
Collaborator

@frodrigo do you want to run point on reworking this PR into a command line option with a default value?

@frodrigo
Copy link
Member Author

Yes. I will look at.

@DennisOSRM
Copy link
Collaborator

any updates, @frodrigo?

@frodrigo
Copy link
Member Author

Still in my todo list, but a bit busy now.

@DennisOSRM
Copy link
Collaborator

This gets implemented in #1298, right? Can we close here?

@frodrigo frodrigo closed this Jan 6, 2015
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.

6 participants