Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSX Catalina (10.15) Issues/Fixes #173

Open
DavidMChan opened this issue Oct 20, 2019 · 14 comments
Open

OSX Catalina (10.15) Issues/Fixes #173

DavidMChan opened this issue Oct 20, 2019 · 14 comments

Comments

@DavidMChan
Copy link

DavidMChan commented Oct 20, 2019

While OSX is not officially supported - I wanted to point out the issues and solutions I found necessary to get Deepmind lab running on 10.15 on the macos branch with python3 for those who are interested. I'm not sure if any of these should be merged, but the documentation could be adapted.

Issues (Or at least, those I was able to find)

  1. RESOLVED (AS OF 12/6): The "//:deepmind/support:realpath" script no longer functions on 10.15. Fix: Install the coreutils brew package (which includes a functioning realpath) and replace all lines in BUILD of the form ln -s $$($(location //deepmind/support:realpath) ... with ln -s $$(realpath .... The patch file is here: https://gist.github.com/DavidMChan/6654a82c658480f8b983b9afabe4502c

  2. SDL 2.0.10 renders the window in only 1/4 of the screen. Fix: Use SDL 2.0.12 by installing with --HEAD on brew. This is related to high DPI support in the upstream (See: https://hg.libsdl.org/SDL/rev/46b094f7d20e and macOS Catalina: Only bottom left quarter of the screen is used by the game ioquake/ioq3#422)

@DavidMChan DavidMChan changed the title OSX Catalina Issues/Fixes OSX Catalina (10.15) Issues/Fixes Oct 20, 2019
@tkoeppe
Copy link
Collaborator

tkoeppe commented Oct 21, 2019

Have you considered the macos branch? https://github.com/deepmind/lab/tree/macos

It addresses some of those issues.

@DavidMChan
Copy link
Author

I had these issues specific to 10.15 on the Macos branch as specified in the post - everything is working well prior to 10.15 on the macos branch.

@tkoeppe
Copy link
Collaborator

tkoeppe commented Oct 21, 2019

Ah, I see, sorry! Is 10.15 available on Travis?

@DavidMChan
Copy link
Author

No worries! I don't believe it is yet - at least according to https://docs.travis-ci.com/user/reference/osx/

@tkoeppe
Copy link
Collaborator

tkoeppe commented Oct 21, 2019

What are the actual errors with my replacement realpath?

@DavidMChan
Copy link
Author

The realpath implementation isn't locating the asset files - that is, it produces a file not found/cannot be resolved error. I'll get the actual bazel debug logs when I'm next in front of my home PC.

I believe the issue may have to do with the realpath not resolving symlinks correctly - I experimented some with it, and it seemed that when the path of a file is hidden behind a symlink, the realpath implementation provided could not find the file.

@DavidMChan
Copy link
Author

The short form:

INFO: Writing tracer profile to '/private/var/tmp/_bazel_davidchan/1ed2868fb4e7e440ee2a2775754846e4/command.profile.gz'
INFO: Analyzed target //:deepmind_lab.so (33 packages loaded, 3221 targets configured).
INFO: Found 1 target...
INFO: From Executing genrule //:non_pk3_assets:
Error resolving path 'assets/default.cfg', error was: 'No such file or directory'
ERROR: /Users/davidchan/Projects/lab/BUILD:681:1: declared output 'baselab/default.cfg' was not created by genrule. This is probably because the genrule actually didn't create this output, or because the output was a directory and the genrule was run remotely (note that only the contents of declared file outputs are copied from genrules run remotely)
ERROR: /Users/davidchan/Projects/lab/BUILD:681:1: not all outputs were created or valid
Target //:deepmind_lab.so failed to build
Use --verbose_failures to see the command lines of failed build steps.
ERROR: /Users/davidchan/Projects/lab/BUILD:961:1 not all outputs were created or valid
INFO: Elapsed time: 21.543s, Critical Path: 11.40s
INFO: 44 processes: 44 darwin-sandbox.
FAILED: Build did NOT complete successfully

Verbose Failure here: https://gist.github.com/DavidMChan/ca1c9b443d6d6056c35916968be98972

@tkoeppe
Copy link
Collaborator

tkoeppe commented Dec 2, 2019

I'd like to understand why the realpath behaviour has changed on 10.15. Just to be sure, could you try out whether it has to do with the extension that allows passing a null pointer for the output, by instead using the following (non-extension) variation:

diff --git a/deepmind/support/BUILD b/deepmind/support/BUILD
index c369673..860ba3a 100644
--- a/deepmind/support/BUILD
+++ b/deepmind/support/BUILD
@@ -32,5 +32,6 @@ cc_binary(
         "-std=c99",
         "-D_DEFAULT_SOURCE",
         "-D_POSIX_C_SOURCE",
+        "-D_DARWIN_BETTER_REALPATH",
     ],
 )
diff --git a/deepmind/support/realpath.c b/deepmind/support/realpath.c
index aad9512..5522dcc 100644
--- a/deepmind/support/realpath.c
+++ b/deepmind/support/realpath.c
@@ -21,24 +21,27 @@
 // step. This implements the equivalent of "realpath -e" on Linux.
 
 #include <errno.h>
+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 
+char p_storage[PATH_MAX] = {};
+
 int main(int argc, char* argv[]) {
   int num_errors = 0;
   errno = 0;
 
   for (int i = 1; i < argc; ++i) {
-    char* p = realpath(argv[i], NULL);
+    char* p = realpath(argv[i], p_storage);
+    p_storage[PATH_MAX - 1] = '\0';
     if (p == NULL) {
-      fprintf(stderr, "Error resolving path '%s', error was: '%s'\n",
-              argv[i], strerror(errno));
+      fprintf(stderr, "Error resolving path '%s', error was: '%s', details: '%s'\n",
+              argv[i], strerror(errno), p_storage);
       errno = 0;
       ++num_errors;
     } else {
       fprintf(stdout, "%s\n", p);
-      free(p);
     }
   }
 

@DavidMChan
Copy link
Author

After testing this morning, I can confirm, this patch does seem to fix the realpath issues.

@tkoeppe
Copy link
Collaborator

tkoeppe commented Dec 5, 2019

Wonderful, thanks! That's a very trivial fix then :-) I'll get that committed.

@tkoeppe
Copy link
Collaborator

tkoeppe commented Dec 5, 2019

(It's hard to know who's being portable and who isn't here. The linux manual says that the use of NULL is Posix-2008 compliant, and that the previous Posix-2001 version is "broken by design", but macos apparently seems to have regressed in Posix compliance and no longer supports the NULL form? I'd welcome further information, if anyone can dig that up.)

@tkoeppe
Copy link
Collaborator

tkoeppe commented Dec 5, 2019

I committed this change in 760ab54, and I've cherrypicked that onto the macos branch (for now, until I next rebase that). Does that work for you?

@DavidMChan
Copy link
Author

Yep! The realpath implementation is working now on 10.15. There are still issues with SDL2 2.0.10, but there's nothing really that we can do about that until they release a new stable version.

@tkoeppe
Copy link
Collaborator

tkoeppe commented Dec 6, 2019

Great :-)

Yes, SDL isn't under our control at all, I just set some reasonable defaults for Travis to work. But you need to adapt it to whatever you have locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants