-
-
Notifications
You must be signed in to change notification settings - Fork 418
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
Build PonyC using CMake #3234
Build PonyC using CMake #3234
Conversation
This is working on Windows. On Ubuntu 8.04 something's not getting linked correctly. All the tests that use JIT are failing. Debugging into the LLVM code it's unable to resolve symbols like |
I wasn't able to reproduce the problem on my Arch Linux system.
I made some changes to Makefile and CMakeLists.txt, I don't think that's why its working, but ya never know:
Anyway, what I'd like to see is the output of
|
e04cce3
to
c0f2d35
Compare
b7429e1
to
c1de032
Compare
Current status: builds on Linux (glibc & musl), FreeBSD, MacOS and Windows. There is an intermittent assertion in the hash |
Latest status: we see the following assertion when compiling the standard library tests (command line is We see it intermittently because the order of compiling things seems to be non-deterministic. @dipinhora Sean said you might have some insight into this as you have done work on hashing in
|
@kulibali the stack trace itself doesn't shed much light on what's going on. the values of i can probably take a look in 2 or 3 weeks (assuming it's not resolved by then). any chance you can point me to a windows dev/debugging for dipin's guide? i was planning on using |
When the assertion happens,
The problem happens (when it happens) on the 733rd call to It would probably be better to use the Windows debugger rather than LLDB. The easiest way would probably be via Visual Studio. Once you do |
@kulibali thanks for the quick how-to for windows debugging.
the above should not be possible because the same comparison function ( code snippet from Lines 57 to 62 in 43fc3ce
code snippet from Lines 324 to 327 in 43fc3ce
code snippet from Lines 299 to 303 in 43fc3ce
|
I believe I have found the problem (#3422). I'll clean some things up and rebase. |
To do:
|
76aba02
to
6172657
Compare
@@ -39,8 +39,8 @@ ASSET_DESCRIPTION="https://github.com/ponylang/ponyc" | |||
|
|||
# Build pony installation | |||
echo "Building ponyc installation..." | |||
make install prefix=${BUILD_PREFIX} default_pic=${PIC} arch=${ARCH} \ | |||
-j${MAKE_PARALLELISM} -f Makefile-lib-llvm symlink=no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
symlink=no
isn't needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the Makefile set symlink=no
if DESTDIR
is specified. That seemed like what the logic in the original makefile was doing. I.e. if prefix
is specified it'll be something like /usr/local
or ~/.local
and it's better install to $prefix/lib/pony/$version
and symlink, but if DESTDIR
is specified then you just want it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha.
@@ -41,9 +41,8 @@ ASSET_DESCRIPTION="https://github.com/ponylang/ponyc" | |||
|
|||
# Build pony installation | |||
echo "Building ponyc installation..." | |||
make install prefix=${BUILD_PREFIX} default_pic=${PIC} arch=${ARCH}\ | |||
link=static -j${MAKE_PARALLELISM} -f Makefile-lib-llvm symlink=no \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link=static
is now the default, yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just checking. thanks.
@@ -1,104 +1,23 @@ | |||
version: 2.1 | |||
# Turn off CircleCI integration and delete this file once CMake builds are working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NICE!
@@ -6,7 +6,7 @@ task: | |||
only_if: $CIRRUS_API_CREATED == "true" | |||
|
|||
container: | |||
image: ponylang/ponyc-ci-x86-64-unknown-linux-gnu-builder:20191105 | |||
image: ponylang/ponyc-ci-x86-64-unknown-linux-gnu-builder:20191106 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that date correct? i'm surprised for this PR that the image is that old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been working on this for far too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm sorry. that sounds like a pained message from your end.
config_script: | ||
- ps: .\make.ps1 -Command configure -Config Release -Generator "Visual Studio 16 2019" -Prefix "build\install\release" -Version nightly | ||
build_script: | ||
- ps: .\make.ps1 -Command build -Config Release -Generator "Visual Studio 16 2019" -Prefix "build\install\release" -Version nightly | ||
install_script: | ||
- ps: .\make.ps1 -Command install -Config Release -Prefix "build\install\release" | ||
package_script: | ||
- ps: .\make.ps1 -Command package -Config Release -Prefix "build\install\release" -Version nightly | ||
upload_script: | ||
- ps: $version = (Get-Date).ToString("yyyyMMdd"); cloudsmith push raw --version $version --api-key ${CLOUDSMITH_API_KEY} --summary "Pony compiler" --description "https://github.com/ponylang/ponyc" ponylang/nightlies build\ponyc-x86_64-pc-windows-msvc-nightly-$version-Release.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we roll all of this up into one script that is in .ci-scripts ala macOS and Linux scripts? Or is that not something that is desirable with windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could, although it's in some ways harder to check for process error codes inside a PS script. Perhaps we could make a new issue to do this later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, new issue sounds good. definitely not a show stopper.
.cirrus.yml
Outdated
cpu: 8 | ||
memory: 24 | ||
|
||
name: "nightly: x86-64-pc-windows-msvc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this name is incorrect. for a tag, that isn't a nightly. it should be release:
not nightly:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, will fix.
install_script: | ||
- ps: .\make.ps1 -Command install -Config Release -Prefix "build\install\release" | ||
package_script: | ||
- ps: .\make.ps1 -Command package -Config Release -Prefix "build\install\release" -Version nightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this not setting the version to nightly and some date? it appears to just be the same "nightly" for every version, am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a hack -- the make.ps1
script will fill in the date if it is given nightly
. Trying to do one-liners in PowerShell is much more verbose than in Bash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha.
only_if: $CIRRUS_PR != '' | ||
|
||
container: | ||
image: ponylang/ponyc-ci-x86-64-unknown-linux-gnu-builder:20191105 | ||
image: ponylang/ponyc-ci-x86-64-unknown-linux-gnu-builder:20191106 | ||
cpu: 8 | ||
memory: 24 | ||
|
||
name: "PR: x86-64-unknown-linux-gnu" | ||
|
||
libs_cache: | ||
folder: build/libs | ||
fingerprint_script: echo "`md5sum lib/CMakeLists.txt` glibc" | ||
populate_script: make libs build_flags=-j8 | ||
|
||
configure_script: | ||
- make configure | ||
build_script: | ||
- make build build_flags=-j8 | ||
test_script: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a duplicate of the task that starts on 232.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O i c. we don't do debug and non-debug anymore. You can kill the debug ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just re-added the debug builds because the debug build was broken and I thought it would be useful to check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was broken about it? it definitely adds a lot more time to PRs but perhaps with caching it won't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We recently added a bunch of bare inline
functions to a header in libponyrt. I learned something new yesterday: for unoptimized builds C99 evidently requires these functions to be declared in a .c
file somewhere. So the build worked for release (where the functions were being inlined because optimization) but not in debug.
ca1e2cb#diff-894a332d21d5ee3397dc394ab3dae5cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be worth leaving them in to catch things like that. Perhaps we can turn off the test step and just compile them.
.cirrus.yml
Outdated
test_stdlib_cross_script: | ||
- make test-cross-ci PONYPATH=../armv7-a/release cross_triple=arm-unknown-linux-gnueabihf cross_arch=armv7-a cross_cpu=cortex-a9 cross_linker=arm-linux-gnueabihf-gcc cross_runner="qemu-arm-static -cpu cortex-a9 -L /usr/local/arm-linux-gnueabihf/libc" | ||
|
||
# AArch64 has been broken for some time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we should just delete all the commented out stuff, sound reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Perhaps we should add an issue to take another look at AArch64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like an excellent idea. open that issue!
@kulibali i left a few comments. did you intend to delete the glibbenchmark files that you did? |
Thanks Sean, I deleted the gtest and gbenchmark code; |
Oops, I'll fix those scripts. |
Would a single debug build on any platform be good enough? I'm in favor of that especially if all the LLVM build time is handled via cache. |
Sure, sounds good. |
- Only have 1 debug PR CI job for debug mode. - Fix nightly and release scripts
- Set `LLVM_ENABLE_OCAMLDOC=OFF` for LLVM build. - Fix indentation in `.gitmodules` Unrelated: - Quote arguments with spaces on Windows when starting processes.
packages/process/_process.pony
Outdated
@@ -170,7 +170,17 @@ class _ProcessWindows is _Process | |||
fun tag _make_cmdline(args: Array[String] val): String => | |||
var cmdline: String = "" | |||
for arg in args.values() do | |||
cmdline = cmdline + arg + " " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes to process
should be in a separate commit, if it should be included in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll make a separate PR.
.cirrus.yml
Outdated
@@ -152,12 +285,21 @@ task: | |||
|
|||
install_script: | |||
- echo "FETCH_RETRY = 6" >> /usr/local/etc/pkg.conf | |||
- evho "IGNORE_OSVERSION = yes" >> /usr/local/etc/pkg.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say echo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
PonyC now builds on all platforms using CMake.
The PR includes a helper wrapper (a Makefile on Unix, and a PowerShell script on Windows) that sets up CMake build directories for you and compiles PonyC to the
build/release
(orbuild/debug
) directory as before, so scripts that depend on this behaviour don't need to change.Building happens in two steps: you will be able to run
make libs
, which will build the vendored LLVM and other libraries like Google Test and Google Benchmark. You should only need to do this once unless the vendored LLVM submodule changes.Then you can run
make build
andmake test
as before.Fixes #3204