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

Create libponyc-standalone on MacOS #4303

Merged
merged 6 commits into from
Jan 17, 2023
Merged

Create libponyc-standalone on MacOS #4303

merged 6 commits into from
Jan 17, 2023

Conversation

mfelsche
Copy link
Contributor

@mfelsche mfelsche commented Jan 14, 2023

It will not link libstdc++ statically, as this is not available on MacOS. When linking against libponyc-standalone one also needs to link against libc++, but at least we have all the LLVM symbols in there.

An example pony program linking against it would need to look like this:

use "lib:ponyc-standalone" if posix or osx
use "lib:c++" if osx

While i think this is acceptable, as those libs are always available on MacOS, i would be happy about any hint on how to include those in libponyc-standalone.

I was not able to test this on an M1 mac.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jan 14, 2023
@SeanTAllen
Copy link
Member

@mfelsche zlib shouldn't be required. i was able to turn it off in the LLVM setup for Linux, well really I turned it off for everything in theory. But that was for LLVM 12. I think there should be a way (probably additional option in LLVM that changed in LLVM 13 or 14) to not require.

https://github.com/ponylang/ponyc/blob/main/lib/CMakeLists.txt#L143 and 144 were what was required as of LLVM 12. Note that prior to LLVM 12, only Line 143 was required.

# on macos we dont have no static libc++
# so when we want to use this lib
# we need to additionally link libc++ and libz (for llvm)
file(GLOB_RECURSE LLVM_OBJS "${PROJECT_SOURCE_DIR}/../../build/build_libs/llvm/src/llvm//lib/libLLVM*.a")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why, but the GLOB_RECURSE had problems on Linux once I started using.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is finding all the libs and the resulting static library has all we need. And this is scoped to Darwin only, so, unless this becomes a problem on M1 mac, i think this is fine, but thanks for the heads-up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How was this tested on MacOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was tested by taking a pony program and linking it to the resulting libponyc-standalone.a.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what program? does the program fully exercise the API or could there be errors that get hidden by dead code elimination?

@SeanTAllen SeanTAllen added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Jan 14, 2023
@ponylang-main
Copy link
Contributor

Hi @mfelsche,

The changelog - added label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4303.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

.release-notes/4303.md Outdated Show resolved Hide resolved
.release-notes/4303.md Outdated Show resolved Hide resolved
@@ -141,7 +141,7 @@ set(LLVM_INCLUDE_BENCHMARKS OFF)
set(LLVM_INCLUDE_TESTS OFF)
set(LLVM_TOOL_REMARKS_SHLIB_BUILD OFF)
set(LLVM_ENABLE_ZLIB OFF)
set(LLVM_ENABLE_ZLIB:STRING OFF)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this tested on Linux? this doesn't seem correct. that we now have LLVM_ENABLE_ZLIB OFF and "OFF".

If it was tested, how was it tested?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ive found something concerning with this @mfelsche. So locally using the latest pre-packaged ponyc, I can't link the "work" branch of ponydoc due to zlib linking errors, but if I build from source with the exact same LLVM settings, it works, so... there seems to be something going on that i'm unsure about but that is making testing hard and is quite possibly impacting on anyone who tests this PR (including you).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was tested by taking a pony program and linking it to libponyc-standalone.a and trying to compile it successfully.

Take this one as a minimal example:

use "lib:ponyc-standalone"

actor Main
  new create(env: Env) =>
    env.out.print("it works")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not sufficient to test for linker errors. if it doesn't exercise the various functions that need say, zlib, then this won't show any linker errors. try testing with the work branch of ponydoc. You'll need to update the Makefile to use a your ponyc and then can do make to try building ponydoc. If it compiles and links you should be good at least on your test machine.

Also note, for others in the future, if you don't have zlib installed in a way that llvm can detect then you can have incorrect entries for turning off ZLIB in the LLVM setup and you won't be able to detect in your test environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So locally using the latest pre-packaged ponyc, I can't link the "work" branch of ponydoc due to zlib linking errors

I am able to reproduce this issue. I was always testing this with the ponyc that i built in the project. The ponyc installed with ponyup didn't work for me. We can (and should) investigate this in another PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ive found something concerning with this @mfelsche. So locally using the latest pre-packaged ponyc, I can't link the "work" branch of ponydoc due to zlib linking errors, but if I build from source with the exact same LLVM settings, it works, so... there seems to be something going on that i'm unsure about but that is making testing hard and is quite possibly impacting on anyone who tests this PR (including you).

The issue that I found that folks need to be aware of for testing is that your test environment must have Zlib installed in a way that LLVM finds it otherwise, no amount of screwing up the LLVM config will result in the errors.

I think before we merge this PR we should have a test in place as part of our CI that verifies that we have this working for supported platforms (Linux and MacOS). There's some serious ways to mess up the CI setup though that would need to be resolved.

I think we should have that CI support as you opened this PR as there is now someone other than me who would be relying on the standalone library working and not getting broken (which appears to be easy to mess up).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on MacOS the combination of:

set(LLVM_ENABLE_ZLIB OFF)
set(LLVM_ENABLE_ZLIB:STRING OFF)

resulted in llvm still requiring libz. Only this worked for me on MacOs:

set(LLVM_ENABLE_ZLIB OFF)
set(LLVM_ENABLE_ZLIB "OFF")

Linux still needed the version without the quotes. It could also be a difference in cmake versions used on both systems...

@SeanTAllen SeanTAllen changed the title Create libponyc-standalone on macos. Create libponyc-standalone on MacOS Jan 15, 2023
@SeanTAllen
Copy link
Member

@ergl @jemc Can you test this on MacOS? You'd need a test program that exercises the API to make sure that linker issues aren't hidden by dead code removal.

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Jan 15, 2023
@SeanTAllen
Copy link
Member

I just tested existing libponyc-standalone on linux with the "work" branch of the ponydoc repo and it doesn't link due to zlib, so, we have a general issue with that being broken that I am looking into. I think it would be good to have a conversation at sync about how we can keep libponyc-standalone from getting broken with LLVM upgrades.

@mfelsche
Copy link
Contributor Author

A more full-blown example to use for checking this on a mac is this one: https://github.com/mfelsche/pony-ast/blob/main/examples/compile/compile.pony

It can be executed e.g. like this (It needs the PONYPATH variable to point to the stdlib:

PONYPATH="$PWD/../ponyc/packages/:$PONYPATH" ./compile ../ponyc/packages/pony_check

@ergl
Copy link
Member

ergl commented Jan 15, 2023

@mfelsche That runs OK for me and prints "OK". I wasn't able to make it work for a "bad" project that contains compilation errors. Is that expected?

This is what I get:

$ PONYPATH="$PWD/../ponyc/packages/:$PONYPATH" ./compile ../ponyc/packages/pony_check
OK

# I introduce an error in a file on purpose
$ ../ponyc/build/release/ponyc ./examples/
Building builtin -> /Users/ponyc-m1/dev/ponyc/packages/builtin
Building ./examples/ -> /Users/ponyc-m1/dev/pony-ast/examples
Error:
/Users/ponyc-m1/dev/pony-ast/examples/main.pony:3:16: could not infer literal type, no valid types found
    let test = 0
               ^

$ PONYPATH="$PWD/../ponyc/packages/:$PONYPATH" ./compile ./examples/
Found 0 Errors:

@SeanTAllen
Copy link
Member

@mfelsche and I have figured out the "why isn't this working" fuckery.

PRs will be coming of which, this PR will be one.

@SeanTAllen
Copy link
Member

@ergl we have some PRs coming to address this, we'll ping you when we are ready for "ok actually ready testing" which should be soon-ish (day or two at the most).

@mfelsche what is the proper test for Borja to run to make sure the linking is working correctly?

@mfelsche
Copy link
Contributor Author

@SeanTAllen @ergl this should do the trick: #4303 (comment)

@mfelsche
Copy link
Contributor Author

@ergl if you see 0 Errors this is usually due to you using a release build of ponyc / libponyc-standalone.a

There is a PR in the pipeline for fixing this. Right now the lib only works with a debug build of ponyc.

But the fact that it printed OK is enough to verify that this somehow works now.

@SeanTAllen
Copy link
Member

@mfelsche I think that this information:

image

should be worked into the release notes.

@SeanTAllen
Copy link
Member

@mfelsche #4304 is merged so you can rebase this against main and bring it up to date and then @ergl can test on M1.

Matthias Wahl and others added 6 commits January 16, 2023 08:20
It will not link libstdc++ statically, as this is not available on osx.
When linking against libponyc-standalone one also needs to link against libc++ and libz,
but at least we have all the llvm symbols in there.
@mfelsche mfelsche force-pushed the osx-ponyc-standalone branch from 18f7487 to 4ac6caa Compare January 16, 2023 07:26
@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label Jan 16, 2023
@SeanTAllen
Copy link
Member

@ergl this is ready for testing.

@ergl
Copy link
Member

ergl commented Jan 17, 2023

Confirmed that this works. I was able to follow @mfelsche's advice of using a debug build of ponyc, and now compile also shows the correct errors:

$ PONYPATH="$PWD/../ponyc/packages/:$PONYPATH" ./compile ../ponyc/packages/pony_check
OK

# I introduce an error in a file on purpose
$ ../ponyc/build/debug/ponyc examples/
Building builtin -> /Users/ponyc-m1/dev/ponyc/packages/builtin
Building ./examples/ -> /Users/ponyc-m1/dev/pony-ast/examples
Error:
/Users/ponyc-m1/dev/pony-ast/examples/main.pony:3:16: could not infer literal type, no valid types found
    let test = 0
               ^

$ PONYPATH="$PWD/../ponyc/packages/:$PONYPATH" ./compile ./examples/
Found 1 Errors:
[ /Users/ponyc-m1/dev/pony-ast/examples/main.pony:3:16 ] could not infer literal type, no valid types found

@SeanTAllen SeanTAllen merged commit 02398a9 into main Jan 17, 2023
@SeanTAllen SeanTAllen deleted the osx-ponyc-standalone branch January 17, 2023 13:37
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Jan 17, 2023
github-actions bot pushed a commit that referenced this pull request Jan 17, 2023
github-actions bot pushed a commit that referenced this pull request Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants