Fixes discover.ml to support platforms that use PKGSRC

[rneswold]

This patch tries to add support for PKGSRC in discover.ml's search path algorithm. PKGSRC installs libev files in /usr/pkg/include/ev and /usr/pkg/lib/ev. The version of discover.ml prior to this would compute the library path to be /usr/pkg/include/ev/../lib, which is wrong.

[ghost]

Shouldn't this replace the last occurrence instead of the last one? I guess this is unlikely that a patch contains twice include or lib but if it happens we'd probably want to replace the last one.

[rneswold]

Maybe use Str.global_replace?

[rneswold]

Sorry for the sloppy pull-request; it's the first time I've done this on GitHub. Next time, I'll make a branch on my forked repo so I won't be committing to master.

[ghost]

I'm not convinced by global_replace either. I think in general what makes the most sense is to replace the last occurrence.

[ghost]

That's alright. If you want to cleanup the pull request you can edit your git history locally, for instance you can squash all the commits into one and git push -f to your forked repo. This should update the pull request.

[rneswold]

If this latest version looks good to you, @diml , I'll merge all the commits together to clean things up (I'm not a big fan of rewriting repository history -- next time I'll do all my development on a branch and then apply the full diff to master.)

[ghost]

I'm still dubious about the use of regular expressions and dir_sep. If you look at the implementation of the Filename module you see:

module Win32 = struct
  ...
  let dir_sep = "\\"
  let is_dir_sep s i = let c = s.[i] in c = '/' || c = '\\' || c = ':'

The dirname and basename function are using is_dir_sep so they are better at splitting a path. I think you should use these rather than using regular expressions, it seems more portable.

I'm not a big fan of rewriting repository history -- next time I'll do all my development on a branch and then apply the full diff to master

I agree it's not good to rewrite the history of a public repo, but in case of github pull request it is considered good practice AFAIK. If you do the dev on a branch and then apply the diff to master you'll have to create a new pull request.

[rneswold]

The dirname and basename functions are using is_dir_sep so they are better at splitting a path. I think you should use these rather than using regular expressions, it seems more portable.

I respecfully disagree.

1. Filename.dir_sep "returns the directory separator". We want to compare directory names in order to replace ones that match, so splitting the path using the directory separator is correct. Under Windows, the device name will be separated out and won't match, but it doesn't matter. The algorithm will still work.
2. discover.ml works much better now than it did before. If someone installs LWT on a filesystem that isn't Windows or Unix compatible, a new issue can be opened. For instance, if someone were able to build OCaml on a VMS system, neither dir_sep nor dirname/basename would work well because VMS path names follow a crazy format: DEVICE:[DIR.DIR.DIR]FILE.EXT
3. The current version of discover.ml simply appends /../lib to the path -- which doesn't even try to determine the directory separator! EDIT: It uses Filename.concat, so the separators will be proper, but it doesn't use Filename.parent_dir_name. Plus, the default search path list is very Unix-centric.
4. When discover.ml couldn't find libev on my system, the suggestion is to set C_INCLUDE_PATH and LIBRARY_PATH. Unfortunately, if discover.ml finds C_INCLUDE_PATH, it doesn't even look at LIBRARY_PATH but instead computes the wrong value (the algorithm of which I'm trying to fix.) I think a further refinement is if both symbols are defined, just use them.

I agree it's not good to rewrite the history of a public repo, but in case of github pull request it is considered good practice AFAIK. If you do the dev on a branch and then apply the diff to master you'll have to create a new pull request.

Okay. If what I've done follows good practice, I'll keep doing it. :)

[rneswold]

For instance, if someone were able to build OCaml on a VMS system, neither dir_sep nor dirname/basename would work well because VMS path names follow a crazy format: DEVICE:[DIR.DIR.DIR]FILE.EXT

Actually, dir_sep would work better because C_INCLUDE_PATH would only specify the path portion. VMS's path layout formally specifies the directory portion and the filename portion. The dirname and basename functions wouldn't be able to recursively break it apart like they could under Windows and Unix.

[rneswold]

Wow. The Filename module is woefully inadequate.

I went back to see what functions are available for moving up and down a directory path (i.e. something like parent_of "/usr/local/include"), but found nothing. Filenames/pathnames are simply strings and all primitives assume a Windows/Unix type path style (dir_sep, current_dir_name, parent_dir_name). Hoping for pathname portability beyond Windows/Unix is out of the scope of this issue.

[ghost]

I agree some changes are required to discover.ml and I agree that splitting on : is not good here. Thought on windows both / and \ are valid directory separators so splitting only on \ only is not enough.

TBH the main reason I'm reluctant to depend on str.cma is that I can imagine scenarios where some user is running ./configure in an environment not setup properly and will get an error about loading str.cma that they won't understand. To avoid complication it is best to can avoid #load entirely in discover.ml.

I'm fine with this patch if we can avoid using str.cma.

[rneswold]

I agree, I didn't like loading "str.cma" in the source file. This version uses your recommended replace_last function. The half dozen pathnames I threw at it were handled correctly. Thanks for the help!

[ghost]

OK, looks good. Do you want to merge the commits together before it is merged?

[rneswold]

I need to verify that this is actually working. I know the replace_last function is working and the list of directories is correct. However, I'm trying to build LWT on my NetBSD system and it still doesn't see my libev files. Let me figure out what I forgot before we merge this.

[rneswold]

Okay, this patch is working on NetBSD. I merged the commits into two: the first fixes the C_INCLUDE_PATH computation and the other adds /usr/pkg to the list of default search paths.

[ghost]

Thanks!