Skip to content
This repository has been archived by the owner on Jun 8, 2024. It is now read-only.

Bundles osrm-components, resolves #283 #288

Merged
merged 1 commit into from
Feb 3, 2017
Merged

Bundles osrm-components, resolves #283 #288

merged 1 commit into from
Feb 3, 2017

Conversation

daniel-j-h
Copy link
Member

For #283. Unblocked upstream by Project-OSRM/osrm-backend#3570.

@daniel-j-h daniel-j-h requested a review from TheMarex January 20, 2017 21:29
TheMarex
TheMarex suggested changes Jan 21, 2017
View reviewed changes
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.

Some small bug, otherwise approach looks sound.

CMakeLists.txt
@@ -73,10 +73,14 @@ if(BUILD_LIBOSRM)
add_custom_command(OUTPUT ${BINDING_DIR}/osrm-datastore
COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:osrm-datastore> ${BINDING_DIR}
DEPENDS osrm-datastore ${BINDING_DIR})
add_custom_command(OUTPUT ${BINDING_DIR}/osrm-components
Copy link
Member

Choose a reason for hiding this comment

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

If cmake would be called without BUILD_COMPONENTS=On this would leave to a dependency that could not be satisfied. This could be wrapped in a conditional, or always set to On.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks! Thinking of it, I see no reason why we don't build the components tool by default now that we no longer depend on gdal. I will remove the BUILD_COMPONENTS option from the backend directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Project-OSRM/osrm-backend#3598

@daniel-j-h daniel-j-h mentioned this pull request Jan 23, 2017
Merged
@daniel-j-h daniel-j-h force-pushed the bundle-component-tool branch 2 times, most recently from f7610f3 to 281966a Compare January 23, 2017 11:19
@daniel-j-h
Copy link
Member Author

This is blocked by osrm-backend failing on XCode 7.3 debug builds: #289 and XCode 8.2 upgrade triggering assertions in Boost Interprocess #290. :sigh:

@daniel-j-h daniel-j-h force-pushed the bundle-component-tool branch from 281966a to 132c1dc Compare February 1, 2017 09:46
@daniel-j-h
Copy link
Member Author

@TheMarex you're requesting changes here - care to take a look again or Push The Button (tm).

This was blocked by macOS debug builds failing (shm issue) which should now be fixed upstream in osrm-backend.

Travis macOS builds for this pull request will take a while, though

@daniel-j-h daniel-j-h force-pushed the bundle-component-tool branch from 132c1dc to da0de22 Compare February 2, 2017 15:07
@TheMarex TheMarex merged commit ed88711 into master Feb 3, 2017
@TheMarex TheMarex deleted the bundle-component-tool branch February 3, 2017 11:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TheMarex TheMarex TheMarex approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daniel-j-h @TheMarex