Opened 13 years ago

Closed 9 years ago

Last modified 9 years ago

#514 closed task (fixed)

[PATCH] Implement dir_watch on OS X

Reported by: Philip Taylor Owned by: stwf
Priority: Should Have Milestone: Alpha 15
Component: Core engine Keywords: simple osx patch
Cc: Patch:

Description

See source/lib/sysdep/dir_watch.h and source/lib/sysdep/os/osx/dir_watch.cpp. The interface needs to be implemented on OS X, so we can automatically reload changed files (for quick testing during development).

See linux/dir_watch_fam.cpp and win/wdir_watch.cpp for possible inspiration.

Attachments (5)

osx_dir_watch.patch (14.2 KB ) - added by alan 11 years ago.
patch translated from https://github.com/zhiqiangxu/fam/tree/master/sysdep/osx
update_workspaces.patch (801 bytes ) - added by alan 11 years ago.
readlink -f doesn't work on mac,fall back to perl in that case
mac_watchdir.patch (34.7 KB ) - added by stwf 10 years ago.
FileInfo and mac watchdir patch
next_mac_wdir.patch (33.1 KB ) - added by stwf 10 years ago.
newest version of patch to add mac file notifications
mac_watchdir.v3.patch​ (33.5 KB ) - added by stwf 9 years ago.

Download all attachments as: .zip

Change History (33)

comment:1 by Jan Wassenberg, 13 years ago

Here's some advice on how to implement this, including a link to sample code: http://stackoverflow.com/questions/1772209/file-level-filesystem-change-notification-in-mac-os-x

Apparently, the recommended way involves scanning each watched directory's files, watching for directory changes, and then finding out which file changed.

To determine if the file changed, a combination of file size and modification time ought to suffice. They can be combined into one value (for convenient storage) by XORing the latter into the high bits of the 64-bit file size.

comment:2 by (none), 12 years ago

Milestone: Unclassified

Milestone Unclassified deleted

comment:3 by Andrew, 12 years ago

Milestone: OS Alpha 2
Owner: set to Andrew

comment:4 by Kieran P, 12 years ago

Milestone: OS Alpha 2OS Alpha 3

comment:5 by Kieran P, 12 years ago

Priority: majortrivial

comment:6 by Kieran P, 12 years ago

Owner: Andrew removed

comment:7 by Kieran P, 12 years ago

Milestone: Alpha 3Alpha 4

comment:8 by Kieran P, 12 years ago

Milestone: Alpha 4Backlog

comment:9 by alan, 11 years ago

sorry to upload only a partly working patch,

help is needed to further integrate this into the codebase:(

dir_watch.cpp
g++ -Iobj/test_Debug -include obj/test_Debug/precompiled.h -MMD -MP -DDEBUG -DLIB_STATIC_LINK -DUSING_PCH -D_REENTRANT -DNVTT_SHARED=1 -I/usr/X11R6/include/X11 -I/usr/X11R6/include -I/usr/include/X11 -I/opt/local/include -I../../../source/pch/test -I../../../source -I../../../libraries/spidermonkey/include-unix -I../../../libraries/cxxtest/include -I../../../libraries/enet/include -I../../../libraries/valgrind/include -I../../../libraries/nvtt/include -g -Wall -Wno-switch -Wno-reorder -Wno-invalid-offsetof -Wextra -Wno-missing-field-initializers -Wunused-parameter -Wredundant-decls -Wnon-virtual-dtor -Wundef -fstack-protector-all -D_FORTIFY_SOURCE=2 -fstrict-aliasing -fpch-preprocess -fno-omit-frame-pointer -msse -msse2 -fvisibility=hidden `wx-config --unicode=yes --cxxflags` `sdl-config --cflags` `pkg-config libxml-2.0 --cflags`  -MF obj/test_Debug/test_root.d -MT "obj/test_Debug/test_dir_watch.o" -o "obj/test_Debug/test_dir_watch.o" -c "../../../source/lib/sysdep/os/osx/dir_watch.cpp"
In file included from /System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/CarbonCore.h:33,
                 from /System/Library/Frameworks/CoreServices.framework/Frameworks/AE.framework/Headers/AE.h:20,
                 from /System/Library/Frameworks/CoreServices.framework/Headers/CoreServices.h:21,
                 from ../../../source/lib/sysdep/os/osx/dir_watch.cpp:25:
/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/Finder.h:243: error: redefinition of ‘struct FileInfo’
../../../source/lib/file/file_system.h:42: error: previous definition of ‘struct FileInfo’
../../../source/lib/sysdep/os/osx/dir_watch.cpp: In function ‘Status record_timestamps(const VfsPath&, const FileInfo&, uintptr_t)’:
../../../source/lib/sysdep/os/osx/dir_watch.cpp:297: error: cannot convert ‘wchar_t*’ to ‘char*’ for argument ‘1’ to ‘char* dirname(char*)’
../../../source/lib/sysdep/os/osx/dir_watch.cpp: In function ‘Status remove_timestamps(const VfsPath&, const FileInfo&, uintptr_t)’:
../../../source/lib/sysdep/os/osx/dir_watch.cpp:321: error: cannot convert ‘wchar_t*’ to ‘char*’ for argument ‘1’ to ‘char* dirname(char*)’
make: *** [obj/test_Debug/test_dir_watch.o] Error 1
Last edited 11 years ago by alan (previous) (diff)

by alan, 11 years ago

Attachment: update_workspaces.patch added

readlink -f doesn't work on mac,fall back to perl in that case

comment:10 by Jan Wassenberg, 11 years ago

For the FileInfo redefinition error, the easiest way to 'fix' that is to #define FileInfo CARBON_FileInfo before including CarbonCore. For converting wchar_t to char (UTF-8), you can use source/lib/utf8.h utf8_from_wstring and then call c_str on the resulting std::string. Note that c_str()'s return value is only temporarily valid.

in reply to:  9 ; comment:11 by historic_bruno, 11 years ago

Replying to alan:

Did you mean to post the update-workspaces patch on #916? You probably should ;)

About the OsPath strings, Philip had this to say in IRC a few days ago:

11:31 <@Philip`> historicbruno/alan_: OsPath on Unixes is a sequence of 8-bit values, encoded as a std::wstring with a wchar_t character per byte in the path
11:32 <@Philip`> They're not encoded as any kind of UTF or anything
11:33 <@Philip`> (because Unix filesystems don't have an encoding at all - paths are just byte strings, so there's a risk of data loss if you try decoding them into real Unicode strings)
11:34 <@Philip`> Use OsString(ospath).c_str() to get a char* that you can pass to system APIs
11:36 <@Philip`> (On Windows that'll give a wchar_t* instead, since Windows' native path type is a sequence of 16-bit values)

in reply to:  11 comment:12 by alan, 11 years ago

Replying to historic_bruno:

Replying to alan:

Did you mean to post the update-workspaces patch on #916? You probably should ;)

About the OsPath strings, Philip had this to say in IRC a few days ago:

11:31 <@Philip`> historicbruno/alan_: OsPath on Unixes is a sequence of 8-bit values, encoded as a std::wstring with a wchar_t character per byte in the path
11:32 <@Philip`> They're not encoded as any kind of UTF or anything
11:33 <@Philip`> (because Unix filesystems don't have an encoding at all - paths are just byte strings, so there's a risk of data loss if you try decoding them into real Unicode strings)
11:34 <@Philip`> Use OsString(ospath).c_str() to get a char* that you can pass to system APIs
11:36 <@Philip`> (On Windows that'll give a wchar_t* instead, since Windows' native path type is a sequence of 16-bit values)

thanks for the tip:)

I now get rid of the compile error, but encountered a new one

when I compile it this way(extracted from test.make):

$(OBJDIR)/test_dir_watch.o: ../../../source/lib/sysdep/os/osx/dir_watch.cpp $(GCH) | prebuild
      @echo $(notdir $<) 
      $(SILENT) $(CXX) $(PCHINCLUDES) $(CXXFLAGS) -MF $(OBJDIR)/test_root.d -MT "$@" -o "$@" -c "$<"       

got this error:

ld: warning: ignoring file obj/test_Release/test_dir_watch.o, file was built for unsupported file format which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libnetwork.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libsimulation2.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libscriptinterface.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libengine.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libgraphics.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libatlas.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libgui.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/liblowlevel.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libmongoose.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libmocks_test.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libAtlasObject.a, file was built for archive which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../libraries/spidermonkey/lib/libmozjs185-ps-release.dylib, file was built for unsupported file format which is not the architecture being linked (i386)
ld: warning: ignoring file /usr/local/lib/libboost_filesystem-mt.dylib, file was built for unsupported file format which is not the architecture being linked (i386)
ld: warning: ignoring file /usr/local/lib/libboost_system-mt.dylib, file was built for unsupported file format which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libnvcore.dylib, file was built for unsupported file format which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libnvmath.dylib, file was built for unsupported file format which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libnvimage.dylib, file was built for unsupported file format which is not the architecture being linked (i386)
ld: warning: ignoring file ../../../binaries/system/libnvtt.dylib, file was built for unsupported file format which is not the architecture being linked (i386)
Undefined symbols for architecture i386:
  "_SDL_main", referenced from:
      -[SDLMain applicationDidFinishLaunching:] in libSDLmain.a(SDLMain.o)
ld: symbol(s) not found for architecture i386
collect2: ld returned 1 exit status

This is also the case when I run make -f test.make:(

here's info of my gcc/g++:

matoiMac:gcc alan$ gcc --version
i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.1.00)
Copyright (C) 2007 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

matoiMac:gcc alan$ g++ --version
i686-apple-darwin11-llvm-g++-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.1.00)
Copyright (C) 2007 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Last edited 11 years ago by alan (previous) (diff)

comment:13 by historic_bruno, 11 years ago

11:14 < alan_> I found the compiling problem may related to the -march=i686 option in CFLAGS of *.make, why is this arch mandated here?
11:15 < alan_> (talking about the compiling problem under os x, i386)

I ran into some arch problems when trying to cross compile a 32-bit executable on 64-bit Snow Leopard. For one thing, our Premake script has a rudimentary arch detection and it may simply get it wrong. Ex: it was using the HOSTTYPE environment variable if set in the shell, otherwise it falls back to parsing the results of gcc -dumpmachine.

Are you actually on a 32-bit CPU, or just a 32-bit kernel? http://support.apple.com/kb/HT4287

in reply to:  13 comment:14 by alan, 11 years ago

My case may be weird:

matoiMac:gcc xuzhiqiang$ arch
i386
matoiMac:gcc xuzhiqiang$ echo $HOSTTYPE
x86_64
matoiMac:workspaces xuzhiqiang$ gcc -dumpmachine
i686-apple-darwin11

say both $HOSTTYPE and gcc -dumpmachine says it's i686, but arch says it's i386.

And according to the error message, it should be i386 actually.

(in the mac menu, I only found my CPU is Intel Core i5)

Replying to historic_bruno:

11:14 < alan_> I found the compiling problem may related to the -march=i686 option in CFLAGS of *.make, why is this arch mandated here?
11:15 < alan_> (talking about the compiling problem under os x, i386)

I ran into some arch problems when trying to cross compile a 32-bit executable on 64-bit Snow Leopard. For one thing, our Premake script has a rudimentary arch detection and it may simply get it wrong. Ex: it was using the HOSTTYPE environment variable if set in the shell, otherwise it falls back to parsing the results of gcc -dumpmachine.

Are you actually on a 32-bit CPU, or just a 32-bit kernel? http://support.apple.com/kb/HT4287

Last edited 11 years ago by alan (previous) (diff)

comment:15 by historic_bruno, 11 years ago

Keywords: patch review added
Milestone: BacklogAlpha 12
Summary: Implement dir_watch on OS X[PATCH] Implement dir_watch on OS X

comment:16 by historic_bruno, 10 years ago

Keywords: review removed
Milestone: Alpha 12Backlog

I've looked into this patch finally, but it has some issues, apart from compilation failure.

  • It shouldn't be using VFS at all, whether VfsPath, g_VFS, etc., because it's not required that a watched path be in the VFS. Instead we should be using OsPath and system path strings exclusively. Obviously this changes some things about the patch.
  • Also, it's not correct to assume that VfsPaths and OsPaths are equivalent for your maps (a VFS path is like "/art/meshes/" relative to the mod's root, while a OS path is the actual system path corresponding to it. So using a VfsPath as the key to a map with OsPath keys is not semantically correct even if it compiles).
  • For creating CStringRefs from OsPaths, it looks simpler to use CFStringCreateWithCString with the source string being OsString(path).c_str() (as Philip described). It seems better IMO to avoid using e.g. wcscpy() by using C++ strings when we can. I'm not sure if the UTF32 encoding is correct, but apparently we do need an encoding when creating a CFStringRef from a C-string.
  • dirname() causes a build error because there's no conversion from wchar_t[] to char*. I think VfsPath::Parent() would be an equivalent VFS operation, but we shouldn't be using VFS anyway. We could use Boost.Filesystem or OS X APIs for getting the base directory from an arbitrary path, if necessary.
  • For enumerating files in system paths, we might be able to use CFURLEnumerator which is available on OS X 10.6+ (or NSFileManager's contentsOfDirectoryAtPath:error:, if we used Cocoa instead).
  • I don't know if it will work even after making these changes, but it looks like too complicated to me, mostly because the Carbon/C++ interface is so cumbersome.
    • There's an example of a Cocoa/Objective-C++ file monitor here that looks relatively simple. The hardest part might be hooking up the init and deinit code, figuring out how events are processed and how threads work. But it's pretty easy to integrate C++ and Object-C++, and our build system handles it well, see the *.mm files in source/lib/sysdep/os/osx for examples.
  • As a minor aesthetic concern, I think it's nicer to have e.g. TimestampMapIterator instead of t_timestamp_map_iterator, but the code in source/lib is weird and unconventional. Some of the function names could be more descriptive, like UpdateWatchedPathTimestamp() instead of record_timestamps().
  • I know it's a work in progress, but it definitely needs comments to explain the logic.

comment:17 by stwf, 10 years ago

Owner: set to stwf

comment:18 by stwf, 10 years ago

Here is a new patch, much simpler although it requires Mac OS10.7 and above. It also does seem that the current implementation in FileSystems.cpp does only apply to the virtual file system, on all platforms, although I could be wrong.

This patch also incorporates the change of FileInfo to CFileInfo as discussed on IRC... 

by stwf, 10 years ago

Attachment: mac_watchdir.patch added

FileInfo and mac watchdir patch

comment:19 by historic_bruno, 10 years ago

Some comments on the patch

  • Our convention is that single statement blocks don't need braces around them, so please don't add them to vfs_lookup.cpp
  • The way it's written now requires building against 10.7 API to work, so our bundle still won't have hotloading (we support 10.6). Maybe instead you could do:
    #ifndef kFSEventStreamCreateFlagFileEvents
    #define kFSEventStreamCreateFlagFileEvents 0x00000010
    #endif
    
    so the constant is always defined, but you won't use it unless it's actually running on 10.7.
  • mode_t (explained in comment for including posix_filesystem.h), g_ListenStartTime, g_FolderMods, FolderModDateList, and StringToWString all appear to be unused?
  • If you got the code for GetSystemVersion here, you should provide a link in the comments and maybe mention it's in the public domain. Also you need the standard WFG header block for the new source files.
  • Why is CFileInfos typedef'd again in dir_watch.cpp?
  • mycallback should probably be called fsevent_callback or something, to aid debugging.
  • The callback event logic could be simplified slightly, each watched case requires kFSEventStreamEventFlagItemIsFile, so why not check for that first, then test all the others individually. The event flag variables should be declared const but I doubt you'll even need them if they become single flags.
  • You're only handling the first event matching a watched dir in the callback, shouldn't it be handling all of them? (I don't know how often there are multiple events in practice, but it's clearly possible) It may be more efficient to set eventFlags[i] to a const variable once before testing it?
  • Is wcsncmp really needed, you might be able to use path_is_subpath instead? (see source/lib/path.h) The same goes for the test in dir_watch_Add?
  • Using malloc is rather ugly in CreateEventStream, also it seems to create a memory leak because free is never called?
    • In fact all the string logic there is ugly and makes some assumptions about encodings that I don't know are true. Could you instead use OsString(ospath).c_str() to convert an OsPath to a char*, which as Philip explained has no particular encoding, then create the string you want with CFStringCreateWithFileSystemRepresentation? I haven't tested but that function seems intended for that purpose and is available on 10.4+.
    • And CFArrayCreate allocates memory again, we own the resulting CFArrayRef (see The Create Rule) so we have to release it. (FSEventStreamCreate is probably the same, but without shutdown code, we can't help that)
  • Should test if CreateEventStream fails and return an error status from dir_watch_Poll (so the error propagates)
  • Don't forget to remove the LOGERROR from CreateEventStream

Then of course the things we discussed in IRC :) Sorry I didn't know it was a WIP patch when I began looking at it.

comment:20 by stwf, 10 years ago

Thanks for all of the input. I think I've gotten everything added here. Probably not perfect but much closer to being committed I'd say....

by stwf, 10 years ago

Attachment: next_mac_wdir.patch added

newest version of patch to add mac file notifications

comment:21 by stwf, 10 years ago

Keywords: review added
Summary: [PATCH] Implement dir_watch on OS X[PATCH REVIEW] Implement dir_watch on OS X

comment:22 by historic_bruno, 10 years ago

Hi, I tried building on 10.7 but get the following errors:

../../../source/lib/sysdep/os/osx/osx_sys_version.mm:31:17: error: expected
      method to read array element not found on object of type 'NSArray *'
      mMajor = [versions[0] integerValue];
                ^
../../../source/lib/sysdep/os/osx/osx_sys_version.mm:34:17: error: expected
      method to read array element not found on object of type 'NSArray *'
      mMinor = [versions[1] integerValue];
                ^
../../../source/lib/sysdep/os/osx/osx_sys_version.mm:37:18: error: expected
      method to read array element not found on object of type 'NSArray *'
      mBugfix = [versions[2] integerValue];

building with

i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.11.00)

The array subscript notation appears to be newer compiler functionality, but the objectAtIndex method should do the same thing.

Making that change, I was able to test modifying the main menu GUI page, and it works nicely! :)

Building against 10.6 SDK, I get the following errors about undeclared constants:

../../../source/lib/sysdep/os/osx/dir_watch.cpp: In function ‘void fsevent_callback(const __FSEventStream*, void*, size_t, void*, const FSEventStreamEventFlags*, const FSEventStreamEventId*)’:
../../../source/lib/sysdep/os/osx/dir_watch.cpp:101: error: ‘kFSEventStreamEventFlagItemIsFile’ was not declared in this scope
../../../source/lib/sysdep/os/osx/dir_watch.cpp:103: error: ‘kFSEventStreamEventFlagItemRemoved’ was not declared in this scope
../../../source/lib/sysdep/os/osx/dir_watch.cpp:105: error: ‘kFSEventStreamEventFlagItemRenamed’ was not declared in this scope
../../../source/lib/sysdep/os/osx/dir_watch.cpp:107: error: ‘kFSEventStreamEventFlagItemCreated’ was not declared in this scope
../../../source/lib/sysdep/os/osx/dir_watch.cpp:109: error: ‘kFSEventStreamEventFlagItemModified’ was not declared in this scope

so not all the values of FSEventStreamEventFlags are available on 10.5+ and we'll have to use the same trick we did for kFSEventStreamCreateFlagFileEvents (I also think it would be cleaner not to use a different name for them, but just #define the constants after testing API version).

I built a 10.6 bundle and it worked other than hotloading of course.

As a test, I used the same bundle on 10.7, and since it was a bundle (with archived public.zip), I added a new mainmenu.xml in ~/Library/Application\ Support/0ad/mods/public/gui/pregame. I modified the new file with the game running and got an unknown event error:

ERROR: unknown event captured /Users/Ben/Library/Application Support/0ad/mods/public/gui/pregame/mainmenu.xml and flags66560

but the GUI did reload properly. The flags are 0x10400 or kFSEventStreamEventFlagItemIsFile and kFSEventStreamEventFlagItemInodeMetaMod (it would help to print the flags as %lx for debugging). I don't know what that event means but it's not really an error, I guess we should silently ignore or log them at most.

comment:23 by historic_bruno, 10 years ago

Milestone: BacklogAlpha 15
Priority: If Time PermitsShould Have
Summary: [PATCH REVIEW] Implement dir_watch on OS X[PATCH] Implement dir_watch on OS X

comment:24 by Jorma Rebane, 9 years ago

Keywords: review removed

by stwf, 9 years ago

Attachment: mac_watchdir.v3.patch​ added

comment:25 by stwf, 9 years ago

Keywords: review added

OK, if you don't mind just one more review. Just to make sure it compiles on the older systems. I took all of your good advice, the unknown messages should just be ignored, and it is neater just redefining the constants, its not like they are going to go back and update those APIs.

thanks for the help.

comment:26 by historic_bruno, 9 years ago

Looks good! I tested with OS X 10.6 and 10.7 SDKs, the build succeeded and hotloading worked ok.

One small thing, there is a FileInfos remaining in /ps/trunk/source/lib/sysdep/os/win/wsnd.cpp causing the Windows build to fail.

comment:27 by stwf, 9 years ago

Resolution: fixed
Status: newclosed

OK, this is in.Thanks for all of the help.

comment:28 by historic_bruno, 9 years ago

Keywords: review removed
Note: See TracTickets for help on using tickets.