Bug 215524

Summary: [NEW PORT] www/osrm-backend: Open Source Routing Machine (Backend)
Product: Ports & Packages Reporter: Andreas Andersson <a.andersson.thn>
Component: Individual Port(s)Assignee: Kurt Jaeger <pi>
Status: Closed FIXED    
Severity: Affects Only Me CC: a.andersson.thn, dim, koobs, pi, tz, ultima
Priority: --- Keywords: feature
Version: LatestFlags: pi: maintainer-feedback+
Hardware: Any   
OS: Any   
See Also: https://github.com/Project-OSRM/osrm-backend/issues/3719
Description Flags
Newest version, port renamed to mirror name in GH project
Update to 5.9.0
5.9.0 added user and group
Updated with choosen user and group
Poudriere log
Fully working
Update to diff
Update to 5.10.0
Passes portlint -AC
Corrects maintainer
a.andersson.thn: maintainer-approval+
Fixes build issue on 10.3
New Makefile
More cleanup
Update to makefile, rc-script
Update to rc-script
Clarification in rc-script
Remove post extract
Update GIDs and UIDs
Update to 1.15.2 update UID and GID
Populate pkg-plist none

Description Andreas Andersson 2016-12-23 18:24:57 UTC
Created attachment 178234 [details]

This adds support for osrm  http://project-osrm.org/ to ports. Please let me know if I am doing something incorrectly.
Comment 1 Andreas Andersson 2017-07-19 11:34:50 UTC
Created attachment 184501 [details]
Newest version, port renamed to mirror name in GH project

This updates to latest version: 5.8.0, tried and tested in production. It also renames the port to osrm-backend to better match the project name in Project-OSRM.

If this is in the incorrect category, let me know and I will change the port category.
Comment 2 Andreas Andersson 2017-08-02 20:21:48 UTC
Created attachment 184968 [details]
Update to 5.9.0
Comment 3 Andreas Andersson 2017-08-03 05:27:12 UTC

root@newtop:/usr/ports/www/osrm-backend # portlint
looks fine.
Comment 4 Andreas Andersson 2017-08-03 06:28:34 UTC
I might need help to choose a proper uid/gid for this one as well.
Comment 5 Andreas Andersson 2017-08-03 12:23:46 UTC
Created attachment 184985 [details]
5.9.0 added user and group

As per koobs wishes here are some changes to submittal of the port.
Comment 6 Andreas Andersson 2017-08-03 12:59:30 UTC
Created attachment 184987 [details]
Updated with choosen user and group
Comment 7 Kubilay Kocak freebsd_committer freebsd_triage 2017-08-06 04:18:36 UTC
Review items:

PORTVERSION is invalid. Use DISTVERSIONPREFIX for the 'v' prefix of the GitHub tagname. GH_TAGNAME (doesnt need to be set explicitly) will then be set correctly.

MASTER_SITES=GH is not required when using USE_GITHUB=yes

See Also: https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/book.html#makefile-master_sites-github-ex3

Pending QA (poudriere) testing/confirmation
Comment 8 Andreas Andersson 2017-08-07 08:20:25 UTC
Created attachment 185111 [details]
Poudriere log
Comment 9 Andreas Andersson 2017-08-07 08:27:31 UTC
Created attachment 185112 [details]
Fully working

Changed user, corrected UIDs file.
Comment 10 Andreas Andersson 2017-08-07 08:52:27 UTC
Created attachment 185113 [details]
Update to diff

Removes GITHUB_PROJECT and +@${MKDIR}  ${WRKSRC}/build as per suggestions by koobs.
Comment 11 Andreas Andersson 2017-08-07 09:40:42 UTC
Created attachment 185115 [details]
Update to 5.10.0
Comment 12 Andreas Andersson 2017-08-07 13:31:54 UTC
Created attachment 185127 [details]
Passes portlint -AC

This diff makes the Makefile pass portlint -AC
Comment 13 Andreas Andersson 2017-08-08 05:42:28 UTC
Created attachment 185143 [details]
Corrects maintainer
Comment 14 Andreas Andersson 2017-08-09 18:38:59 UTC
Comment 15 Kurt Jaeger freebsd_committer 2017-08-10 05:46:39 UTC
Comment 16 Kurt Jaeger freebsd_committer 2017-08-10 19:20:58 UTC
Builds are fine on 11a, 12a, but fail on 10.3i:

Comment 17 Torsten Zuehlsdorff freebsd_committer 2017-08-11 10:30:17 UTC
Thanks Kurt!

Andreas: please update the patch to correct the build-issues. I will have a look at it than.
Comment 18 Kubilay Kocak freebsd_committer freebsd_triage 2017-08-11 11:52:21 UTC
Upstream issue (may or may not be exactly the same issue, please verify), closed without changes:

Invalid use of non-static data member 'offset'

Also noted:

* -std=c++1y is included in compiler args

CC dim who may be able to shed light on (or remind us about) FreeBSD 10.x (10.3 in this case), Clang and C++11 support or issues
Comment 19 Andreas Andersson 2017-08-12 13:55:57 UTC
I could make it build with using llvm40

BUILD_DEPENDS+=    clang++40:devel/llvm40
BUILD_DEPENDS+=    clang++40:devel/llvm40
BUILD_DEPENDS+=    llvm-config40:devel/llvm40


CC= clang40
CXX= clang++40

But then there's: https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/dads-cc.html advising against it.

What's the best way to proceed with this? Is it okay for me to submit a patch with the above lines?
Comment 20 Dimitry Andric freebsd_committer 2017-08-12 15:25:19 UTC
I took a look at the compilation error, and it seems to have been fixed in newer versions of clang, but it's rather hard to come up with a small test case, and figuring out which exact upstream revision fixed it.  So for now the easiest workaround would be to build it with a newer clang from ports, or gcc.
Comment 21 Andreas Andersson 2017-08-12 19:29:55 UTC
Created attachment 185331 [details]
Fixes build issue on 10.3

Fixes build issue on 10.3. Also tested with 11.1, not tested with 12.
Comment 22 Kurt Jaeger freebsd_committer 2017-08-12 19:39:56 UTC
Comment 23 Kurt Jaeger freebsd_committer 2017-08-12 20:19:19 UTC
Testbuilds are fine on 12a, 11a and 10i 8-)
Comment 24 Richard Gallamore freebsd_committer 2017-08-14 07:23:09 UTC
* Because you are using DISTVERSIONPREFIX, should probably use DISTVERSION instead of PORTVERSION.
* Add LICENSE_FILE if it is available in the source.
* Instead of calling var with append flag with *_DEPENDS, this should be like so

LIB_DEPENDS= foo.so:devel/foo \
 <tab> <tab> bar.so:devel/bar \
... ... and so on.

* CMAKE_BUILD_TYPE Should be after USES section.

Would have to test the port to verify the compiler_type bit after bsd.port.pre.mk. I think there is a better way to set the minimum version of clang. Pretty sure testing COMPILER_TYPE should be CHOSEN_COMPILER_TYPE, but not positive on this.
Comment 25 Andreas Andersson 2017-08-14 08:57:46 UTC
Richard I have in my local copy fixed most of the issues you commented on. I would like to get help with specifying the compiler version properly. 

Ideally I would like to be able to set a variable for this, such as COMPILER= clang and COMPILER_VERSION= >=40 or of it's likes. But looking at compiler.mk it seems to to only be used for comparisons. What is a cleaner way of doing this?
Comment 26 Andreas Andersson 2017-08-14 11:01:35 UTC
Created attachment 185402 [details]
New Makefile

For some reason my ports tree seems messed up and doesn't catch the changes so I am not getting the full diff. Is there some weird caching going on with svnlite? deleting tree and and checking it out again did not help.

Hence attaching Makefile.

Seems that when I do compiler:c++14-lang ports pulls down llvm40 all by it self. Magic. 

Hence there is no more need for the COMPILER/COMPILER_VERSION conditional or overriding CC and CXX vars.

I also added LICENSE_FILE and re-arranged and changed vars in the Makefile as per Richards suggestions.
Comment 27 Andreas Andersson 2017-08-14 13:20:35 UTC
Created attachment 185405 [details]

* Pulls in llvm40 by specifying compiler:c++14-lang. 
* Removes conditional, adjusts LIB_DEPENDS as per Richards suggestions. 
* portlint -AC gives no warnings/errors. 
* poudriere porttest runs fine on 10.3 and 11.
Comment 28 Richard Gallamore freebsd_committer 2017-08-14 17:15:12 UTC
(In reply to Andreas Andersson from comment #27)
That was one solution I was considering may fix the problem. I'm not sure if the port actually requires c++14 though, but it is better than setting cc and cxx.

> For some reason my ports tree seems messed up and doesn't catch the changes so I am not getting the full diff. Is there some weird caching going on with svnlite? deleting tree and and checking it out again did not help.

I'm not sure I really understand, Usually svnlite diff foo bar devel/foobar >foobar.diff

Assuming it has already been added svnlite add ...

A couple other things,
* OSRMSHAREDIR is not needed, ${DATADIR} by default is set to ${PREFIX}/share/${PORTNAME}.

* I'm not sure I understand the osrm_options variable in in the rc file. It is overridable and doesn't really set anything so the vars inside it should probably just be moved to command_args.

* install -o $osrm_user /dev/null ${pidfile}. Why is this creating a file in /dev/null? this shouldn't be this services task.

* Should this services default shell be /bin/sh or /usr/sbin/nologin? Does the program need this specifically? Same question with the set home directory.
Comment 29 Andreas Andersson 2017-08-14 18:27:20 UTC
(In reply to Richard Gallamore from comment #28)

> * OSRMSHAREDIR is not needed, ${DATADIR} by default is set to ${PREFIX}/share/${PORTNAME}.

Will be fixing this shortly

> * install -o $osrm_user /dev/null ${pidfile}. Why is this creating a file in /dev/null? this shouldn't be this services task.

What do you suggest instead?

> * Should this services default shell be /bin/sh or /usr/sbin/nologin? Does the program need this specifically? Same question with the set home directory.

I'll try without those two.
Comment 30 Richard Gallamore freebsd_committer 2017-08-14 18:55:04 UTC
(In reply to Andreas Andersson from comment #29)
Err, I wasn't properly caffeinated when looking at this this morning. The install pid looks fine. Could probably have more restrictive perms like 644.

I was actually one line off, chown ${osrm_user}:${osrm_group} ${osrm_file} this command will fail without testing osrm_file variable is set or setting this to a default. Should have a test before running.

[ -n "${osrm_file" ] && chown ...
Comment 31 Andreas Andersson 2017-08-14 20:32:37 UTC
Created attachment 185417 [details]
More cleanup

Turns out there's even more magic in Cmake / ports.

* Removed some of the manual copying of files and dir creation.
* Added check in osrm rc.d script to make sure file exist before trying to chown datafiles.
* Changed osrm user to use /usr/sbin/nologin
* Set /nonexistent as dir for osrm user.
* Set permissions 644 on the datadir.
Comment 32 Richard Gallamore freebsd_committer 2017-08-14 21:42:07 UTC
I think you forgot the osrm_options, this isn't really doing anything and can be user modifiable. It should probably be removed.
Comment 33 Andreas Andersson 2017-08-14 22:20:04 UTC
(In reply to Richard Gallamore from comment #32)
>I think you forgot the osrm_options, this isn't really doing anything and can be user modifiable. It should probably be removed. 

It is intended to be used when for instance one would want to modify the number of threads osrm would use, or to set the listening ip, for instance if you would like to use shared memory.

osrm-routed <base.osrm> [<options>]:

  -v [ --version ]                      Show version
  -h [ --help ]                         Show this help message
  --trial [=arg(=1)]                    Quit after initialization

  -i [ --ip ] arg (=            IP address
  -p [ --port ] arg (=5000)             TCP/IP port
  -t [ --threads ] arg (=8)             Number of threads to use
  -s [ --shared-memory ] [=arg(=1)] (=0)
                                        Load data from shared memory
  -a [ --algorithm ] arg (=CH)          Algorithm to use for the data. Can be 
                                        CH, CoreCH, MLD.
  --max-viaroute-size arg (=500)        Max. locations supported in viaroute 
  --max-trip-size arg (=100)            Max. locations supported in trip query
  --max-table-size arg (=100)           Max. locations supported in distance 
                                        table query
  --max-matching-size arg (=100)        Max. locations supported in map 
                                        matching query
  --max-nearest-size arg (=100)         Max. results supported in nearest query
  --max-alternatives arg (=3)           Max. number of alternatives supported 
                                        in the MLD route query
Comment 34 Torsten Zuehlsdorff freebsd_committer 2017-08-17 11:42:12 UTC
So - did you reach a conclusion here? Is the port in a shape it could be committed?
Comment 35 Richard Gallamore freebsd_committer 2017-08-17 17:43:03 UTC
Here is the problem with the rc script:


osrm_options="--max-table-size 50"

The command ran would translate to this:
/usr/sbin/daemon -f -c -p /var/run/osrm.pid /usr/local/bin/osrm-routed --max-table-size 50

As you can see, osrm_flags and osrm_file have been ignored because osrm_options defaults to using osrm_flags and osrm_files. Changing osrm_options makes these variables unused in the script. The _flags and _file option should be moved to command_args with _option being empty by default.

Also, something I missed
: ${osrm_enable:="NO"}
should be the first user modifiable variable for this script.

Also, some other things I just found glancing at the diff,
* CMAKE_BUILD_TYPE=	Release, this line is redundant when defining cmake in USES and should be removed.
* Consider adding DEBUG to OPTIONS_DEFINE, this will make it possible to select the option and will change BUILD_TYPE to Debug.

(In reply to Torsten Zuehlsdorff from comment #34)
Thanks Torsten, forgot to reply back. Its getting close assuming I didn't miss anything, which is always possible.
Comment 36 Andreas Andersson 2017-08-17 18:15:35 UTC
(In reply to Richard Gallamore from comment #35)

Hi, thanks for your input. I will look at how to implement this better.

Basically one of the things I thought of. If you are running with --shared--memory setting you do **not** need to setup an osrm-file. What is happening with --shared-memory setting is that you want to load the osrm into memory by using osrm-datastore and then just start osrm-routed with osrm-routed --shared-memory. See https://github.com/Project-OSRM/osrm-backend/wiki/Configuring-and-using-Shared-Memory

One of the things I've been thinking about how to solve this is to ignore the osrm file flag if osrm_shared_memory is set to "YES".

Does it make sense to have osrm_options for all the different settings of osrm-routed or only for the shared memory setting?
Comment 37 Andreas Andersson 2017-08-17 18:16:30 UTC
Meanwhile I am also awating 5.11.0 which is in rc3 right now.
Comment 38 Richard Gallamore freebsd_committer 2017-08-17 20:50:31 UTC
(In reply to Andreas Andersson from comment #36)
I think it would be better to have the shared_memory variable. Also, can you please take a look at [1] and follow the example a bit more closely? The main item I refer to is the comment section at the top. This will help dispel any confusion one may have. Add a note that the shared_memory variable will override X.

I think the _option variable should be removed, those can be defined in flags. Making it clear in the comment about shared_memory option should be enough. Thanks again for your work on this.

[1] https://www.freebsd.org/doc/en/books/porters-handbook/rc-scripts.html
Comment 39 Andreas Andersson 2017-08-19 18:38:02 UTC
Created attachment 185581 [details]
Update to makefile, rc-script

This update to diff does the following:

 * Updates to 5.11.0
 * Removes CMAKE_BUILD_TYPE= Release
 * Adds DEBUG option 

RC-script changes:
Checks if osrm_shared_memory is enabled, ignores osrm_file if osrm_shared_memory is set to yes. If not it checks if file exists, exits with error message if it doesnt.
Comment 40 Andreas Andersson 2017-08-19 18:53:25 UTC
Created attachment 185582 [details]
Comment 41 Andreas Andersson 2017-08-19 18:55:00 UTC
Created attachment 185583 [details]
Update to rc-script
Comment 42 Andreas Andersson 2017-08-19 19:00:30 UTC
Created attachment 185584 [details]
Clarification in rc-script

Clarification rc script.

portlint -AC runs without errors.

poudriere testport ok on 10 and 11.
Comment 43 Richard Gallamore freebsd_committer 2017-08-20 04:29:30 UTC
Looks good to me, only thing that comes to mind is I'm not sure the post-extract is actually necessary.
Comment 44 Andreas Andersson 2017-08-20 05:53:45 UTC
(In reply to Richard Gallamore from comment #43)
IIRC when I first created the port back in december I needed to remove those files or cmake would fail.
Comment 45 Andreas Andersson 2017-08-20 16:45:20 UTC
Created attachment 185608 [details]
Remove post extract

Just tried without post-extract and stopped removing those files. Seems to work, poudriere testport fine on 10 and 11.
Comment 46 Richard Gallamore freebsd_committer 2017-08-20 16:49:07 UTC
(In reply to Andreas Andersson from comment #45)
Yeah, that's what I thought. cmake:outsource usually takes care of all that.
Comment 47 Andreas Andersson 2017-08-20 18:13:18 UTC
So, is this ready to be committed now? :)
Comment 48 Torsten Zuehlsdorff freebsd_committer 2017-08-23 12:43:27 UTC
(In reply to Andreas Andersson from comment #47)

> So, is this ready to be committed now? :)

Hey, i was going to say that! ;)
Comment 49 Richard Gallamore freebsd_committer 2017-08-23 14:09:31 UTC
Everything looks good to me, port looks beautiful.
Comment 50 Torsten Zuehlsdorff freebsd_committer 2017-09-04 13:50:57 UTC

can you please update the patch? It did not apply anymore to the ports-tree. UID and GID files are rejected. 

I'm planning to have a lock at it this week and get it finally committed.

Comment 51 Andreas Andersson 2017-09-04 14:07:51 UTC
Created attachment 186062 [details]
Update GIDs and UIDs

Due to another project taking  the UID and GID I choose (zap) I am updating the diff with another UID and GID.
Comment 52 Torsten Zuehlsdorff freebsd_committer 2017-09-05 10:54:37 UTC
The port doesn't build on FreeBSD 11.0 amd64 nor i386. Did you test it only for 10.3 after the last change?
Comment 53 Andreas Andersson 2017-09-06 07:02:39 UTC
(In reply to Torsten Zuehlsdorff from comment #52)

This seems to be because of clang 3.8.0. Appearantly the `compiler:c++14-lang` flag does not cause LLVM40 to be installed in 11.0 while it does so for 10.3 ?
Comment 54 Torsten Zuehlsdorff freebsd_committer 2017-09-11 11:07:15 UTC
(In reply to Andreas Andersson from comment #53)

> This seems to be because of clang 3.8.0. Appearantly the `compiler:c++14-lang` 
> flag does not cause LLVM40 to be installed in 11.0 while it does so for 10.3 ?

Yes - it seems so. That is quite irritating. So you need to use a switch based on the FreeBSD version :(

Question beside: does it build with GCC 6? This is now the new default and i saw it being mentioned in the thread.
Comment 55 Torsten Zuehlsdorff freebsd_committer 2017-11-21 15:01:36 UTC
Hello Andreas,

are you still working at the port?

Comment 56 Torsten Zuehlsdorff freebsd_committer 2017-11-23 13:00:42 UTC
Hello Andreas,

i did a test through all plattforms. It works on 10.3, 10.4, 11.1 but not on 11.0. Since the support for 11.0 ends next friday, i have no problem to commit it than.

I needed to update GID and UID for the test.

Since the current release is 5.13: would you mind to update the patch for next week?

Comment 57 Andreas Andersson 2017-11-23 14:06:07 UTC
(In reply to Torsten Zuehlsdorff from comment #56)

Hi, I can certainly do that. Expect a new patch tomorrow. Sorry for not responding earlier. But I've had a really busy few months at work due to merger.
Comment 58 Torsten Zuehlsdorff freebsd_committer 2017-12-01 11:59:33 UTC
(In reply to Andreas Andersson from comment #57)

> Hi, I can certainly do that. Expect a new patch tomorrow. Sorry for not 
> responding earlier. But I've had a really busy few months at work due to 
> merger.

I was busy the last few months, too. No problem!

If you come up with a new patch i'm going to commit it.

Comment 59 Torsten Zuehlsdorff freebsd_committer 2018-01-29 11:26:36 UTC
Any news here? 

Comment 60 Andreas Andersson 2018-02-10 12:18:07 UTC
(In reply to Torsten Zuehlsdorff from comment #59)

Yes! Currently doing porttest on 10.3, 11.1 already passed. When 10.3 is done I will test on 10.4. This is with the newest version (5.15.2). When that is done I'll submit a new pr with updated GID and UID as well.  

This might take a few hours though because I don't have a 10.4 jail yet and I am assuming it will build all packages for me for that jail.
Comment 61 Andreas Andersson 2018-02-10 17:26:05 UTC
I am unable to test 10.4.

Compilation of LLVM50 ends with this:

-- Installing: /wrkdirs/usr/ports/devel/llvm50/work/stage/usr/local/llvm50/share/doc/clang/html/./_images/DriverArchitecture.png
-- Installing: /wrkdirs/usr/ports/devel/llvm50/work/stage/usr/local/llvm50/share/doc/clang/html/./DiagnosticsReference.html
CMake Error at tools/clang/docs/cmake_install.cmake:36 (file):
  file INSTALL cannot copy file
Call Stack (most recent call first):
  tools/clang/cmake_install.cmake:60 (include)
  tools/cmake_install.cmake:43 (include)
  cmake_install.cmake:61 (include)

FAILED: CMakeFiles/install/strip.util 
cd /wrkdirs/usr/ports/devel/llvm50/work/.build && /usr/local/bin/cmake -DCMAKE_INSTALL_DO_STRIP=1 -P cmake_install.cmake
ninja: build stopped: subcommand failed.
*** Error code 1

make: stopped in /usr/ports/devel/llvm50
=>> Cleaning up wrkdir
===>  Cleaning for llvm50-5.0.1
build of devel/llvm50 | llvm50-5.0.1 ended at Sat Feb 10 16:27:30 UTC 2018
build time: 00:10:19
!!! build failure encountered !!!

Any ideas?
Comment 62 Andreas Andersson 2018-02-10 17:26:41 UTC
(In reply to Andreas Andersson from comment #61)

10.3 and 11.1 working fine though.
Comment 63 Andreas Andersson 2018-02-10 21:49:40 UTC
Created attachment 190493 [details]
Update to 1.15.2 update UID and GID

Updates to 1.15.2 , sets osrm UID and GID to 228
Comment 64 Andreas Andersson 2018-02-10 21:50:01 UTC
(In reply to Andreas Andersson from comment #63)

Builds fine on 10.3, 10.4 and 11.1
Comment 65 Andreas Andersson 2018-02-11 09:33:37 UTC
Also builds on 12-CURRENT
Comment 66 Kurt Jaeger freebsd_committer 2018-02-11 10:01:29 UTC
The pkg-plist file is empty, looks strange ?
Comment 67 Andreas Andersson 2018-02-11 10:07:23 UTC
Created attachment 190501 [details]
Populate pkg-plist

Here is the correct diff _with_ pkg-plist. Sorry about that.
Comment 68 Kurt Jaeger freebsd_committer 2018-02-11 13:28:11 UTC
testbuilds are fine.
Comment 69 commit-hook freebsd_committer 2018-02-13 19:56:43 UTC
A commit references this bug:

Author: pi
Date: Tue Feb 13 19:56:38 UTC 2018
New revision: 461738
URL: https://svnweb.freebsd.org/changeset/ports/461738

  New port: www/osrm-backend

  This is a port of osrm-backend of the open-streetmap project.

  WWW: http://project-osrm.org/

  PR:		215524
  Submitted by:	Andreas Andersson <a.andersson.thn@gmail.com>
  Reviewed by:	koobs, ultima, tz

Comment 70 Kurt Jaeger freebsd_committer 2018-02-13 19:57:05 UTC
Committed, thanks for your patience!