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

Fix lib/llvm to support MacOS #3181

Merged
merged 1 commit into from
Jun 16, 2019

Conversation

winksaville
Copy link
Contributor

lib/llvm/Makefile: OSX does not support LLVM_USE_LINKER so do not specify
a LLVM_LINKER and instead use the default system linker.

  • Default LLVM_LINKER to empty value
  • Only set LLVM_USE_LINKER if LLVM_LINKER is not empty

lib/llvm/llvm-base.rules: OSX does not support the -T parameter so just
use the position parameters. The first parameter is the target
and the second parameter is the name of the symlink, so removing
the -T fixes this incompatibility.

@winksaville
Copy link
Contributor Author

Testing only DO NOT MERGE.

@SeanTAllen, please test this on Mac OSX

@SeanTAllen SeanTAllen self-assigned this Jun 15, 2019
@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Jun 15, 2019
@SeanTAllen
Copy link
Member

This is awesome @winksaville. Thanks. I'll give this a trial run sometime this weekend.

@SeanTAllen
Copy link
Member

Testing this now.

I've identified one small additional issue:

MacOS find readlink as the readlink binary used for LLVM_READ_LINK.
When that is used on line 91 of the lib/llvm/Makefile, it uses the -f option that isn't supported on MacOS.

@SeanTAllen
Copy link
Member

@winksaville this worked fine, there is that one readlink issue.

@winksaville
Copy link
Contributor Author

@SeanTAllen could you past the readlink --help I want to see if there is -e, --canonicalize-existing options. Here is what I have on my Arch Linux system:

$ readlink --help
Usage: readlink [OPTION]... FILE...
Print value of a symbolic link or canonical file name

  -f, --canonicalize            canonicalize by following every symlink in
                                every component of the given name recursively;
                                all but the last component must exist
  -e, --canonicalize-existing   canonicalize by following every symlink in
                                every component of the given name recursively,
                                all components must exist
  -m, --canonicalize-missing    canonicalize by following every symlink in
                                every component of the given name recursively,
                                without requirements on components existence
  -n, --no-newline              do not output the trailing delimiter
  -q, --quiet
  -s, --silent                  suppress most error messages (on by default)
  -v, --verbose                 report error messages
  -z, --zero                    end each output line with NUL, not newline
      --help     display this help and exit
      --version  output version information and exit

GNU coreutils online help: <https://www.gnu.org/software/coreutils/>
Full documentation <https://www.gnu.org/software/coreutils/readlink>
or available locally via: info '(coreutils) readlink invocation'

@SeanTAllen
Copy link
Member

@winksaville it doesn't even support --help

I tried -e and got:

readlink: illegal option -- e
usage: readlink [-n] [file ...]

It appears from the manpage that it only support -n

This FreeBSD manpage is the same as what I get on MacOS:

https://man.openbsd.org/FreeBSD-11.1/stat.1

@winksaville
Copy link
Contributor Author

Would it be reasonable to coreutils it should be available via brew or macports, see this? If you can install coreutils try printing help and lets see what it supports.

@SeanTAllen
Copy link
Member

coreutils installs greadlink which is in /usr/local/bin.

Usage: /usr/local/bin/greadlink [OPTION]... FILE...
Print value of a symbolic link or canonical file name

  -f, --canonicalize            canonicalize by following every symlink in
                                every component of the given name recursively;
                                all but the last component must exist
  -e, --canonicalize-existing   canonicalize by following every symlink in
                                every component of the given name recursively,
                                all components must exist
  -m, --canonicalize-missing    canonicalize by following every symlink in
                                every component of the given name recursively,
                                without requirements on components existence
  -n, --no-newline              do not output the trailing delimiter
  -q, --quiet,
  -s, --silent                  suppress most error messages (on by default)
  -v, --verbose                 report error messages
  -z, --zero                    end each output line with NUL, not newline
      --help     display this help and exit
      --version  output version information and exit

GNU coreutils online help: <http://www.gnu.org/software/coreutils/>
Full documentation at: <http://www.gnu.org/software/coreutils/readlink>
or available locally via: info '(coreutils) readlink invocation'

Seems like checking for greadlink before readlink would be a good idea?

@winksaville winksaville force-pushed the Fix-lib-llvm-for-osx branch from 826aa0b to f528bef Compare June 16, 2019 03:29
@SeanTAllen SeanTAllen changed the title Fix lib/llvm to support OSX Fix lib/llvm to support MacOS Jun 16, 2019
@winksaville
Copy link
Contributor Author

@SeanTAllen,

Tweaks to prefer greadlink and if OSTYPE is osx and report an error if greadlink is not available as the readlink that might exist isn't going to work.

@SeanTAllen
Copy link
Member

@winksaville sounds good.

@SeanTAllen SeanTAllen added changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge and removed do not merge This PR should not be merged at this time labels Jun 16, 2019
@SeanTAllen
Copy link
Member

@winksaville tested again. looking good. you happy with now and ready for a merge?

lib/llvm/Makefile: OSX does not support LLVM_USE_LINKER so do not specify
a LLVM_LINKER and instead use the default system linker.

 - Add OSTYPE from the code in Makefile-ponyc
 - Check for greadlink if OSTYPE is osx
 - Default LLVM_LINKER to empty value
 - Only set LLVM_USE_LINKER if LLVM_LINKER is not empty

lib/llvm/llvm-base.rules: OSX does not support the -T parameter so just
use the position parameters. The first parameter is the target
and the second parameter is the name of the symlink, so removing
the -T fixes this incompatibility.
@winksaville winksaville force-pushed the Fix-lib-llvm-for-osx branch from f528bef to 6919a81 Compare June 16, 2019 16:16
@winksaville
Copy link
Contributor Author

LGTM, this last change just removed the debug I'd enabled. So lets run the tests one last time and you do one last test before merging.

@SeanTAllen
Copy link
Member

@winksaville I left one comment re: ld-gold. Not sure if the line I noted is intentionally or left over from debugging.

@winksaville
Copy link
Contributor Author

winksaville commented Jun 16, 2019

@winksaville I left one comment re: ld-gold. Not sure if the line I noted is intentionally or left over from debugging.

I'd missed that comment, I've now commented on it and marked it as resolved.

@SeanTAllen
Copy link
Member

Failure is unrelated. Merging. Thanks @winksaville.

@SeanTAllen SeanTAllen merged commit 4a9977a into ponylang:master Jun 16, 2019
SeanTAllen added a commit that referenced this pull request Jun 16, 2019
@winksaville
Copy link
Contributor Author

@SeanTAllen, you're most welcome!

@winksaville winksaville deleted the Fix-lib-llvm-for-osx branch June 16, 2019 20:55
patches11 pushed a commit to patches11/ponyc that referenced this pull request Jun 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants