Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

Add DragonFly bootstrap support (ltsmaster) #109

Merged

Conversation

dkgroot
Copy link

@dkgroot dkgroot commented Dec 21, 2017

All druntime files except for:

  • src/core/sys/posix/
  • src/core/sys/dragonflybsd/
    They will be posted in a separate PR

Related PR's:

@joakim-noah
Copy link

No point reviewing your ltsmaster druntime pulls yet, since you're still updating your upstream pulls. If and when those are merged, I'll port them back to this branch, then you can limit these pulls to changes you had to make to just this older branch.

@dkgroot
Copy link
Author

dkgroot commented Dec 29, 2017

@joakim-noah Thanks for the heads up.
It looks like it might be a while before all three upstream druntime PR's are through (lots of reviewers, which is a good thing, but does take a while)

@dkgroot
Copy link
Author

dkgroot commented Jan 4, 2018

Created an example CI for DragonFly, reusing most of the CI scripting i used to build the dmd version.
Note: Just a proof of concept and a way to show what would is needed to run a VM inside a CI VM, and communicate with this wrapped VM over ssh.

@joakim-noah
Copy link

Update to the upstream merged changes and I'll pull.

@dkgroot
Copy link
Author

dkgroot commented Feb 22, 2018

@joakim-noah.

  • Rebased
  • Synced up to dlang/PR/1999 (manually)

@dkgroot
Copy link
Author

dkgroot commented Feb 22, 2018

Ready to Merge
DFly SemaphoreCI

Copy link

@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.

Some tweaks and questions.

@@ -45,6 +45,13 @@ version (FreeBSD)
///
enum int FP_ILOGBNAN = int.max;
}
version (DragonFlyBSD)

Choose a reason for hiding this comment

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

Forgot an else here.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -25,6 +25,10 @@ private
{
import core.sys.posix.sys.types;
}
version (DragonFlyBSD)

Choose a reason for hiding this comment

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

An else would be good here.

Copy link
Author

Choose a reason for hiding this comment

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

Done

///
int getc(FILE* stream) { return fgetc(stream); }
///
int putc(int c, FILE* stream) { return fputc(c,stream); }
int putc(int c, FILE* stream) { return fputc(c, stream); }

Choose a reason for hiding this comment

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

I don't see the point of these two spacing tweaks.

Copy link
Author

Choose a reason for hiding this comment

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

Reverted

src/rt/dmain2.d Outdated
@@ -324,6 +328,21 @@ extern (C) int _d_run_main(int argc, char **argv, MainFunc mainFunc)
fldcw fpucw;
}
}
version (DragonFlyBSD) version (D_InlineAsm_X86)

Choose a reason for hiding this comment

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

I thought you're not supporting 32-bit? Take this out.

Copy link
Author

Choose a reason for hiding this comment

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

Correct 👍

* Authors: Martin Nowak,Diederik de Groot(port:DragonFlyBSD)
* Copied: From core/sys/freebsd/sys
*/
module core.sys.dragonflybsd.pthread;

Choose a reason for hiding this comment

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

Why remove this file if it's there upstream?

Copy link
Author

Choose a reason for hiding this comment

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

I can add it back in (but it is not used in the ltsmaster for now.
I would have to change the either the filename or the modulename. Which one do you prefer ?

Choose a reason for hiding this comment

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

The name doesn't have to match, just leave it alone without a revert. If you want to fix it, fix it upstream.

Copy link
Author

Choose a reason for hiding this comment

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

Ok...
Note: I did get an error when running the (ninja) unittests (which this file does not even contain), about pthread_np not existing.

Copy link

@joakim-noah joakim-noah Feb 23, 2018

Choose a reason for hiding this comment

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

Ninja warning or error? It can spit a warning for a file that's not in the CMake file, not a big deal.

Are you saying this file is used upstream in master but not in this branch? A grep turned up no use in master either.

Copy link
Author

Choose a reason for hiding this comment

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

Actually an error (if i remember correctly, would have to rerun to check).
Link: dlang#2107

Copy link
Author

Choose a reason for hiding this comment

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

Nope doesn't seem to be used in master yet either (copied over from the freebsd implementation, but does not seem to be required. Maybe we should remove it altogetehr (in retrospect)).

Copy link
Author

Choose a reason for hiding this comment

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

dlang#2107 got merged upstream.

@@ -832,7 +910,8 @@ extern (D) @trusted
///
int getchar() { return getc(stdin); }
///
int putchar(int c) { return putc(c,stdout); }
int putchar(int c) { return putc(c, stdout); }

Choose a reason for hiding this comment

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

I mentioned two spacing tweaks, this is the other one I don't see a need for.

Copy link
Author

Choose a reason for hiding this comment

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

:-)

@dkgroot
Copy link
Author

dkgroot commented Mar 1, 2018

Builds cleanly and all unittests passed
See: https://semaphoreci.com/dkgroot/ldc_dragonfly_ci/branches/master/builds/32

@joakim-noah
Copy link

Been busy with other stuff, will get to this this weekend.

@@ -833,6 +911,7 @@ extern (D) @trusted
int getchar() { return getc(stdin); }
///
int putchar(int c) { return putc(c,stdout); }

Choose a reason for hiding this comment

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

Take this out too.

Copy link
Author

Choose a reason for hiding this comment

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

Done :-)

@joakim-noah joakim-noah merged commit e424515 into ldc-developers:ldc-ltsmaster Mar 4, 2018
@dkgroot
Copy link
Author

dkgroot commented Mar 4, 2018

@joakim-noah Thanks a lot !

@dkgroot dkgroot deleted the dragonfly-ltsmaster branch March 4, 2018 12:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants