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

Disable conditional restriction handling by default in configs #4622

Merged
merged 1 commit into from
Nov 20, 2017

Conversation

karenzshea
Copy link
Contributor

@karenzshea karenzshea commented Oct 18, 2017

Issue

Closes #4620

Tasklist

  • review
  • adjust for comments

@karenzshea karenzshea requested a review from TheMarex October 18, 2017 16:08
@TheMarex
Copy link
Member

This needs a rebase after we merge #4623

Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

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

We can't conditionally write the .restrictions file even if its empty because that will break the using code in osrm-contract/customize.

storage::io::FileWriter writer(conditional_penalties_filename,
storage::io::FileWriter::GenerateFingerprint);
extractor::serialization::write(writer, indexed_conditionals);
if (!indexed_conditionals.empty())
Copy link
Member

Choose a reason for hiding this comment

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

The reason why we write out en empty file here is that otherwise the loading code will fail: https://travis-ci.org/Project-OSRM/osrm-backend/jobs/289928057#L2508

The .restrictions file can be empty but is not optional.

".osrm.geometry",
".osrm.fileIndex",
".osrm.properties"},
{".osrm.restrictions"},
Copy link
Member

Choose a reason for hiding this comment

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

.restrictions is actually not optional, but can be empty.

@daniel-j-h
Copy link
Member

From reading the ticket / conversation here it looks like this is a bug fix which should go into master and a release asap? What needs to be done here?

@karenzshea karenzshea force-pushed the conditionals/disable branch 2 times, most recently from 5dfb1cb to a5597c6 Compare November 2, 2017 18:21
@karenzshea karenzshea force-pushed the conditionals/disable branch from a5597c6 to 78fb519 Compare November 9, 2017 22:32
@TheMarex TheMarex merged commit 834890c into master Nov 20, 2017
@DennisOSRM DennisOSRM deleted the conditionals/disable branch November 6, 2022 14:13
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.

3 participants