search

pfr / VideoSpeedTracker

Track vehicles on one lane one way, or two lane bidirectional streets; record direction, compute speed and size
64 stars 9 forks source link

Make it build on Mac and Linux #8

Closed piki closed 8 years ago

piki commented 8 years ago

I've made it compile on Mac. It can read videos with some codecs (e.g., MJPEG but not H.264) but can't write highlight videos.

It compiles on Linux but has no idea how to read video, because my opencv is missing a bunch of codecs.

My intention is that these files will all still build on Windows with MSVC. The differences are small enough that all three platforms should be able to build out of a single directory. To that end, can one of the Windows folks (@pfr?) tell me what I've managed to break here?

For all the gory details, read the commit messages. The biggest change was replacing system("dir ...") with calls to opendir and readdir.

pfr commented 8 years ago

I went through the details of your commits and everything looked OK. I did not try compiling your branch before doing the pull, but it was because everything looked OK. After altering the path to ../common to ../../common because that fits better with MSVC project organization and commenting out the include of sys/dir.h because it doesn't exist in MSVC and it appears the needed functions are included in an already included library (probably std, but didn't determine) everything compiled but I got a linker error for an unresolved external, related to class vector. I could not track that one down easily. So I reverted the pull I shouldn't have done on your portable branch. It looks like things are OK, except I wasn't expecting the branch named revert-8-portable to come into being. I understand the 8, but why is it necessary for the branch to exist?

We're leaving the country in a few days and I don't know if I'll get back to it before we leave.

If there's a better place at Github to carry on this discussion I don't know it. I'm still working my way into the protocol when it involves anything other than my own pushes and pulls.

On Mon, Feb 29, 2016 at 5:46 PM, Patrick Reynolds notifications@github.com wrote:

I've made it compile on Mac. It can read videos with some codecs (e.g., MJPEG but not H.264) but can't write highlight videos.

It compiles on Linux but has no idea how to read video, because my opencv is missing a bunch of codecs.

My intention is that these files will all still build on Windows with MSVC. The differences are small enough that all three platforms should be able to build out of a single directory. To that end, can one of the Windows folks (@pfr https://github.com/pfr?) tell me what I've managed to break here?

For all the gory details, read the commit messages. The biggest change was

replacing system("dir ...") with calls to opendir and readdir.

You can view, comment on, or merge this pull request online at:

https://github.com/pfr/VideoSpeedTracker/pull/8 Commit Summary

  • Fix the slashes in the includes
  • Fix warnings and whitespace
  • Lower-case vector
  • Add Makefile and .gitignore for OSX
  • Handle blank lines in config
  • Enable compile-time debug symbols
  • Fix an infinite loop
  • Improve portability, fix bugs:
  • Use c++11 on Linux, too
  • Refactor Makefile to share with ProcessHilites
  • Refactor some shared code
  • Add Makefile and .gitignore for ProcessHilites
  • Make it work on Mac
  • Handle blank lines and \r in config
  • Fix signed/unsigned comparisons
  • Drop empty switch statements
  • Handle empty frames more gracefully
  • Use getchar instead of sleep, for portability

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/pfr/VideoSpeedTracker/pull/8.

piki commented 8 years ago

After altering the path to ../common to ../../common because that fits better with MSVC project organization

Did you move the common directory up a level? I don't see a commit for that. If you didn't move it, the #include should still be ../common, because #include "..." paths are relative to the directory of the C file doing the including.

commenting out the include of sys/dir.h

#ifndef _WIN32 ... #endif would be right for that. Ideally, we should have a single repo that compiles out of the box on all targeted platforms.

unresolved external, related to class vector

Paste it here? vector is STL, so I'd expected it to be defined the same anywhere.

So I reverted the pull I shouldn't have done on your portable branch. It looks like things are OK, except I wasn't expecting the branch named revert-8-portable to come into being. I understand the 8, but why is it necessary for the branch to exist?

That's the way reverting a PR works on GitHub: it doesn't undo the existence of the prior PR. It creates a new PR, with a new branch, that applies the opposite set of changes.

If there's a better place at Github to carry on this discussion I don't know it. I'm still working my way into the protocol when it involves anything other than my own pushes and pulls.

Here is fine.

The protocol, described in brief here, is that when I want to suggest a change, I:

Here, the maintainer is you. I guess it's a little unusual for you to jump right into the maintainer role, rather than start out in the contributor role. Most of our documentation is written for new contributors, with the assumption that once you've seen the process from the contributor side, it will be easy to switch to the maintainer side.

A key part of your role as maintainer is to make sure that my proposal (my PR) is right for all users before merging it. To do that, git pull, git co portable (my branch name), and try to clean and rebuild. If it doesn't compile, don't merge the PR.

You can also short-cut things a bit, as the maintainer, by just pushing commits directly to my branch if you think they're uncontroversial. But commenting on the PR is safer, because who knows what might be controversial?

I've opened another PR, #10, to continue the discussion and get the portability changes in. Clearly, they'll require some iteration as you mentioned above, since it didn't build for you as-is. The branch there is awkwardly named revert-9-revert-8-portable, because that's what it does.

piki commented 8 years ago

We're leaving the country in a few days and I don't know if I'll get back to it before we leave

No hurry. I have and can work with my own branch. I am pushing up portability changes for the benefit of anyone else who wants this code to work with Mac, Linux, Pi, Android, whatever. If someone's in a hurry, they can check out the revert-9-revert-8-portable, too.