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

Add DragonFly bootstrap support (ltsmaster) #2461

Merged

Conversation

dkgroot
Copy link
Contributor

@dkgroot dkgroot commented Dec 21, 2017

Note: This port only supports 64-bit, because DragonFlyBSD is currently x86_64 only (i386 support removed a while back).

Related PR's:

@dkgroot
Copy link
Contributor Author

dkgroot commented Dec 21, 2017

Current status (Together with the other related PR's :

mkdir build; cd build
cmake -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_BUILD_TYPE=None -DD_FLAGS="-w" -DLLVM_CONFIG=/usr/local/bin/llvm-config38 ..
...
make -j2 
...
make -j2 test
...
100% tests passed, 0 tests failed out of 663
Total Test time (real) = 2669.35 sec

@@ -17,7 +17,7 @@ function(add_testsuite config_name dflags model)
# testsuite build system provides no way to run the test cases with a
# given set of flags without trying all combinations of them.
add_test(NAME ${name}
COMMAND make -k -C ${PROJECT_SOURCE_DIR}/tests/d2/dmd-testsuite RESULTS_DIR=${outdir} DMD=$<TARGET_FILE:ldmd2> DFLAGS=${dflags} MODEL=${model} quick
COMMAND ${CMAKE_MAKE_PROGRAM} -k -C ${PROJECT_SOURCE_DIR}/tests/d2/dmd-testsuite RESULTS_DIR=${outdir} DMD=$<TARGET_FILE:ldmd2> DFLAGS=${dflags} MODEL=${model} quick
Copy link
Member

@kinke kinke Dec 21, 2017

Choose a reason for hiding this comment

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

As Travis shows, ninja as CMAKE_MAKE_PROGRAM doesn't accept -k without numeric argument. It's also not able to run Makefiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kinke Thanks for pointing out the issue. Hoping this solves the issue, nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ci/cicleci seems to be doing the right thing now
I think semaphoreci is failing because it is expecting to run against ldc:master and ltsmaster. The CMakeList.txt it is having trouble with was not changed by this PR and unrelatd to the GNUMAKE change i made.

@dkgroot
Copy link
Contributor Author

dkgroot commented Dec 21, 2017

Little question: Should i be committing the updated directory links runtime/druntime and runtime/phohos, which should be pointing to the other PR's or will this cause issues during merging later on ?

@kinke
Copy link
Member

kinke commented Dec 21, 2017

It would cause inconvenience due to the 3 parallel druntime PRs; but as the submodules are still in your repo, you cannot update them here anyway until they've been merged (need to be in our respective repo).
Yeah don't worry about AppVeyor and Semaphore; the latter can sadly only be configured via web GUI (no config file in repo) and so only works for master.

@dkgroot
Copy link
Contributor Author

dkgroot commented Dec 21, 2017

@kinke Thanks for the reply. Will not commit the links (as per your explanation).

I thought it a good idea to split up the druntime PR to reduce the size (which was requested when the druntime master PR was pushed to dlang/druntime). It's a bit annoying to deal with when running tests, but i manage.

Locally running cmake + make and/or cmake + ninja works fine now and finishes with 0 errors, so that is pretty good news.

Copy link
Contributor

@joakim-noah joakim-noah left a comment

Choose a reason for hiding this comment

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

You can take out a lot of this, only need a few of these tweaks.

dmd2/cppmangle.c Outdated
@@ -39,7 +39,7 @@
*/

#if !IN_LLVM
//#if TARGET_LINUX || TARGET_OSX || TARGET_FREEBSD || TARGET_OPENBSD || TARGET_SOLARIS
//#if TARGET_LINUX || TARGET_OSX || TARGET_FREEBSD || TARGET_OPENBSD || TARGET_DRAGONFLYBSD || TARGET_SOLARIS
Copy link
Contributor

Choose a reason for hiding this comment

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

Take all these TARGET_DRAGONFLYBSD additions out, as this is unused in ldc, as can be seen by the !IN_LLVM above. We could probably rip all these !IN_LLVM sections out altogether, as it's unlikely we'll ever need to sync against an updated dmd 2.068 frontend again, but certainly no reason to add to it.

dmd2/mars.c Outdated
@@ -17,7 +17,7 @@
#include <string>
#include <cstdarg>

#if __linux__ || __APPLE__ || __FreeBSD__ || __OpenBSD__ || __sun
#if __linux__ || __APPLE__ || __FreeBSD__ || __OpenBSD__ || __DragonFly__ || __sun
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost everything after this is not compiled in because of a giant #if !IN_LLVM block starting about 5% into this file, so you can just drop all your edits below.

@@ -11,7 +11,7 @@
#define OBJECT_H

#if !IN_LLVM
#define POSIX (__linux__ || __APPLE__ || __FreeBSD__ || __OpenBSD__ || __sun)
#define POSIX (__linux__ || __APPLE__ || __FreeBSD__ || __OpenBSD__ || __DragonFly__ || __sun)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused, take it out.

dmd2/globals.h Outdated
@@ -89,6 +89,7 @@ struct Param
bool isFreeBSD; // generate code for FreeBSD
bool isOpenBSD; // generate code for OpenBSD
bool isSolaris; // generate code for Solaris
bool isDragonFlyBSD;// generate code for DragonFlyBSD
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused for almost two years now, see !IN_LLVM one line up, no need to add this.

dmd2/mars.h Outdated
__APPLE__ Mac OSX
__FreeBSD__ FreeBSD
__OpenBSD__ OpenBSD
__DragonFly__ DragonFlyBSD
Copy link
Contributor

@joakim-noah joakim-noah Dec 22, 2017

Choose a reason for hiding this comment

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

Sorry, missed this, think you should just remove all these comment edits in this header file. This one is obvious and the later TARGET_* stuff is unused by ldc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Joakim for pointing these out. The use of !IN_LLVM was not clear to me. Reverting all of the above.

dmd2/globals.c Outdated
@@ -34,23 +34,23 @@ void Global::init()
#else
#if TARGET_WINDOS
obj_ext = "obj";
#elif TARGET_LINUX || TARGET_OSX || TARGET_FREEBSD || TARGET_OPENBSD || TARGET_SOLARIS
#elif TARGET_LINUX || TARGET_OSX || TARGET_FREEBSD || TARGET_OPENBSD || TARGET_DRAGONFLYBSD || TARGET_SOLARIS
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted before, TARGET_* are unused in ldc: note the IN_LLVM 9 lines up, which puts this in the #else, meaning it's taken out. All changes to this file can be removed.

if (HAVE_UNISTD_H)
# Needed for zlib
append("-DHAVE_UNISTD_H" CMAKE_C_FLAGS_RELEASE)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this and rebase, already added it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already rebased and the change is in.

@JohanEngelen
BTW: line 269 doesn't work well when compiling with -DCMAKE_BUILD_TYPE=xxxx other than release. It does work fine if i change it to:

append("-DHAVE_UNISTD_H" CMAKE_C_FLAGS)

Send the same comment to #2317 (which was already merged).

Copy link
Contributor

Choose a reason for hiding this comment

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

Johan's PR for this has been merged, you can remove your change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased again to pull fix #2475

@dkgroot
Copy link
Contributor Author

dkgroot commented Dec 29, 2017

@joakim-noah Thanks joakim for reviewing a approving !

@joakim-noah joakim-noah merged commit 6b92b88 into ldc-developers:ltsmaster Dec 31, 2017
@dkgroot
Copy link
Contributor Author

dkgroot commented Dec 31, 2017

Thanks everyone for your reviews and merging this port into ldc !
All the best wishes to you all for 2018

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

Successfully merging this pull request may close these issues.

3 participants