#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)
Change History (33)
comment:1 by , 14 years ago
comment:3 by , 13 years ago
Milestone: | → OS Alpha 2 |
---|---|
Owner: | set to |
comment:4 by , 13 years ago
Milestone: | OS Alpha 2 → OS Alpha 3 |
---|
comment:5 by , 13 years ago
Priority: | major → trivial |
---|
comment:6 by , 13 years ago
Owner: | removed |
---|
comment:7 by , 13 years ago
Milestone: | Alpha 3 → Alpha 4 |
---|
comment:8 by , 13 years ago
Milestone: | Alpha 4 → Backlog |
---|
by , 12 years ago
Attachment: | osx_dir_watch.patch added |
---|
patch translated from https://github.com/zhiqiangxu/fam/tree/master/sysdep/osx
follow-up: 11 comment:9 by , 12 years ago
sorry to upload only a partly working patch http://trac.wildfiregames.com/attachment/ticket/514/osx_dir_watch.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
by , 12 years ago
Attachment: | update_workspaces.patch added |
---|
readlink -f doesn't work on mac,fall back to perl in that case
comment:10 by , 12 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.
follow-up: 12 comment:11 by , 12 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)
comment:12 by , 12 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.
follow-up: 14 comment:13 by , 12 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
comment:14 by , 12 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 ofgcc -dumpmachine
.Are you actually on a 32-bit CPU, or just a 32-bit kernel? http://support.apple.com/kb/HT4287
comment:15 by , 12 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 12 |
Summary: | Implement dir_watch on OS X → [PATCH] Implement dir_watch on OS X |
comment:16 by , 11 years ago
Keywords: | review removed |
---|---|
Milestone: | Alpha 12 → Backlog |
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 usingOsPath
and system path strings exclusively. Obviously this changes some things about the patch. - Also, it's not correct to assume that
VfsPath
s andOsPath
s 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 aVfsPath
as the key to a map withOsPath
keys is not semantically correct even if it compiles). - For creating
CStringRef
s fromOsPath
s, it looks simpler to use CFStringCreateWithCString with the source string beingOsString(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 aCFStringRef
from a C-string. dirname()
causes a build error because there's no conversion fromwchar_t[]
tochar*
. I thinkVfsPath::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 oft_timestamp_map_iterator
, but the code insource/lib
is weird and unconventional. Some of the function names could be more descriptive, likeUpdateWatchedPathTimestamp()
instead ofrecord_timestamps()
. - I know it's a work in progress, but it definitely needs comments to explain the logic.
comment:17 by , 11 years ago
Owner: | set to |
---|
comment:18 by , 11 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...
comment:19 by , 11 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 includingposix_filesystem.h
),g_ListenStartTime
,g_FolderMods
,FolderModDateList
, andStringToWString
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 indir_watch.cpp
? mycallback
should probably be calledfsevent_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 declaredconst
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 aconst
variable once before testing it? - Is
wcsncmp
really needed, you might be able to usepath_is_subpath
instead? (seesource/lib/path.h
) The same goes for the test indir_watch_Add
? - Using
malloc
is rather ugly inCreateEventStream
, also it seems to create a memory leak becausefree
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 anOsPath
to achar*
, 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)
- 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
- Should test if
CreateEventStream
fails and return an error status fromdir_watch_Poll
(so the error propagates) - Don't forget to remove the
LOGERROR
fromCreateEventStream
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 , 11 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 , 11 years ago
Attachment: | next_mac_wdir.patch added |
---|
newest version of patch to add mac file notifications
comment:21 by , 11 years ago
Keywords: | review added |
---|---|
Summary: | [PATCH] Implement dir_watch on OS X → [PATCH REVIEW] Implement dir_watch on OS X |
comment:22 by , 11 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 , 11 years ago
Milestone: | Backlog → Alpha 15 |
---|---|
Priority: | If Time Permits → Should Have |
Summary: | [PATCH REVIEW] Implement dir_watch on OS X → [PATCH] Implement dir_watch on OS X |
comment:24 by , 11 years ago
Keywords: | review removed |
---|
by , 11 years ago
Attachment: | mac_watchdir.v3.patch added |
---|
comment:25 by , 11 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 , 11 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 , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
OK, this is in.Thanks for all of the help.
comment:28 by , 11 years ago
Keywords: | review removed |
---|
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.