Add DragonFlyBSD support for phobos by dkgroot · Pull Request #5941 · dlang/phobos

dkgroot · 2017-12-18T14:04:37Z

Notes:

Exclude assertion for dragonfly in std/experimental/allocator/mallocator.d (Needs further investigation, not sure why aligned_realloc is causing trouble).

Exclude assertion for dragonfly in std/experimental/allocator/mallocator.d (Needs further investigation, not sure why alliagned_realloc is causing trouble) cleanup in std/datetime/timezone.d

Notes: - Exclude assertion for dragonfly in std/experimental/allocator/mallocator.d (Needs further investigation, not sure why alliagned_realloc is causing trouble)

…dragonflybsd-master

dlang-bot · 2017-12-18T14:04:37Z

Thanks for your pull request, @dkgroot! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	18092	Can't combine take and takeExactly

wilzbach

Thanks a lot for making this happen! A quick, initial review ;-)

wilzbach · 2017-12-18T14:48:59Z

posix.mak

+# OS can be linux, win32, win32wine, osx, freebsd, netbsd or dragonflybsd. The system will be
 # determined by using uname

+#QUIET:=@


Any reason for this to be added?

It was needed while debugging, accidentally committed. Retracted now.

wilzbach · 2017-12-18T14:49:29Z

posix.mak

 # Configurable stuff, usually from the command line
 #
-# OS can be linux, win32, win32wine, osx, or freebsd. The system will be
+# OS can be linux, win32, win32wine, osx, freebsd, netbsd or dragonflybsd. The system will be


Nit: break it at 80 chars here (i.e. after dragonflybsd).

wilzbach · 2017-12-18T14:49:42Z

std/conv.d


 // Test attribute propagation for UDTs
-pure nothrow @safe /* @nogc */ unittest
+pure nothrow @safe /+@nogc+/ unittest


Unrelated change - remove?

Made it easier to exclude ranges of unittests, while trying to track down a particular issue.
But reverting the change.

wilzbach · 2017-12-18T14:53:13Z

std/math.d

        }
+        else version (DragonFlyBSD)
+        {
+            asm pure nothrow @nogc // assembler by W. Bright


For the record: I'm not a huge fan of copying blocks as this means a bug fix will very likely miss one of these bits.
Anyhow as far as I know is Walter a huge proponent of using version(A) and not of static if( A || B)

I completely agree. I did see this 'argument' pop up several time in different PR's and Forum discussions. I even wanted to suggest a generic 'BSD' version flag, as they all have common origins, similar to the 'Posix' version. But i did not want to reopen this discussion.

Or maybe we do away with the asm implementation and implement a better poly algorithm.

https://issues.dlang.org/show_bug.cgi?id=12084

It's possible too to put the implementation in a token string enum polyAsm = q{ asm pure nothrow @nogc { ... } }; and to mix it

I think this discussion should be had outside this PR. Sounds more like a structural (ie ASM) discussion, than pertaining to this particular PR (correct ?).

wilzbach · 2017-12-18T14:55:05Z

std/parallelism.d

        }
+        else version(DragonFlyBSD)
+        {
+            auto nameStr = "hw.ncpu\0".ptr;


Off topic: this looks like a very old style. IIRC are strings literals always zero-terminated?

Don't know, maybe someone else can answer this one.

Yes, that's true, the string literals are always zero-terminated. I was wondering if something is also doing nameStr.length in which case it makes sense putting this \0 but it doesn't look like it.

@nemanja-boric-sociomantic
So should i rewrite this to: auto nameStr = "hw.ncpu".ptr;, or just leave it as is.

The surrounding lines are doing the same, though. and i do not want to correct them (Only DragonFlyBSD related lines should be touched in this PR).

I would leave them as is IMHO and do this separately (meaning just leave the "hw.ncpu\0".

…dragonflybsd-master

Notes: - FIXME message related to dragonfly malloc issue (issue reported on upstream dragonfly issue database)

jmdavis

The std.datetime portions definitely look fine, and I don't see any issues in the other changes either. It'll be nice to see another BSD added to the list of systems that work with D.

…dragonflybsd-master

PetarKirov

Overall everything looks good to me, except for two changes, that I would prefer if you could figure out why they are needed.

PetarKirov · 2017-12-18T18:02:00Z

std/regex/internal/parser.d

 {//@@@BUG@@@ write is @system
    with(zis)
    {
+        import std.regex.internal.kickstart;


Are you sure that this import is needed? I wonder why it hasn't shown up as problem on other platforms.

I had some trouble in line 1095 when running without this added import. I will recheck.

Looks like the issue was resolved (possible/maybe by the git rebase i did before committing the PR). Will remove the import.
Thanks for finding this one.

PetarKirov · 2017-12-20T09:05:12Z

std/experimental/allocator/mallocator.d

    assert(c.ptr);

+    version (DragonFlyBSD) {} else                    /* FIXME: assertion below fails, have not been able to figure out why, yet */
    assert(!AlignedMallocator.instance.alignedReallocate(c, size_t.max, 4096));


If this assert fails it follows that alignedReallocate successfully called posix_memalign, allocating more virtual memory than what could be reasonably available on the system. Can you try to debug this test and see what exactly breaks down?

Reported this error to dragonfly (dragonfly bug report)
See PR Discussion
Will update this entry to add both these links, so the issue can be tracked.

dkgroot · 2017-12-20T11:34:44Z

@ZombineDev Thanks for Reviewing BTW
@jmdavis Thanks !

PetarKirov

Thanks @dkgroot for the swift reply and changes. Please squash your commits into one and this PR will be good to go.

dkgroot · 2017-12-20T13:20:59Z

@ZombineDev Thanks for the review. I guess i will still have to wait a little, to allow other to review, before squashing, right ? How do you guys normally like to see this squash done, using 'rebase -i HEAD~5'?

ibuclaw · 2017-12-20T13:58:03Z

@dkgroot. Yes, git rebase and then fix up all commits into one. Amending the original commit message if necessary.

PetarKirov · 2017-12-20T13:58:24Z

I think the PR is good to go, so we don't need to wait more. I prefer doing squashing with git rebase -i HEAD~<number of commits on top of master> (in your case git rebase -i HEAD~8) and marking all commits except for the first one as squash or fixup (http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html).

dkgroot · 2017-12-20T14:14:48Z

@ZombineDev I did the git rebase -i HEAD~8 as you suggested, but now this PR includes other/unrelated commits (ie: #a11f63ff,#4a1065e1,#1ad40c60,#cae4a305), See. I hope this is not going to cause issues.

nemanja-boric-sociomantic · 2017-12-20T14:16:44Z

Just do git fetch upstream followed by git rebase -i upstream/master, given that the upstream is the name of the blessed remote.

PetarKirov · 2017-12-20T14:22:24Z

@dkgroot Yes, as @nemanja-boric-sociomantic said, first rebase your branch on top of upstream master and then squash it. (Sorry for not mentioning this before.)

nemanja-boric-sociomantic · 2017-12-20T14:47:20Z

I see you lost all of your commits :-). git reflog will save you in case you don't know it already.

andralex · 2017-12-20T14:47:33Z

@dkgroot what I do when I have unrelated commits in my PR is I remove them during git rebase -i

andralex · 2017-12-20T14:54:18Z

@dkgroot you may want to send me email (see http://erdani.com/index.php/contact) and I'll invite you to https://dlang.slack.com. Then you can get community help in real time via text chat in the browser.

ibuclaw · 2017-12-20T15:14:01Z

@andralex - He is already on slack. :-)

dkgroot · 2017-12-20T15:41:29Z

Sorry for the 'git mess' I created. Accoding to @Geod24 it is merge able as is, so leaving it alone for now.
Thanks for all your help guys

andralex · 2017-12-20T16:11:08Z

Thanks for the work and many thanks to the reviewers! I'll do the honors unless someone beats me to it.

andralex · 2017-12-20T16:20:01Z

Oh, was too hasty - many commits. Will undo this.

dkgroot · 2017-12-20T16:26:29Z

Thanks a lot everyone for the help / review / git stuff and the final merge !

MartinNowak · 2017-12-21T16:07:47Z

Thanks for your contribution.

Oh, was too hasty - many commits. Will undo this.

Well a missing squash isn't the end of the world, it's only a single merge commit on master after all.

dkgroot added 5 commits December 11, 2017 16:27

Port of phobos to DragonFlyBSD

6c7031d

Reinstate unittest for dragonfly in std/traits.d

26cf405

Exclude assertion for dragonfly in std/experimental/allocator/mallocator.d (Needs further investigation, not sure why alliagned_realloc is causing trouble) cleanup in std/datetime/timezone.d

Fix indentation / tab instead of space

5157abc

Port of phobos to DragonFlyBSD

143b511

Notes: - Exclude assertion for dragonfly in std/experimental/allocator/mallocator.d (Needs further investigation, not sure why alliagned_realloc is causing trouble)

Merge branch 'dragonflybsd-master' of github.com:dkgroot/phobos into …

33034d2

…dragonflybsd-master

dkgroot requested review from CyberShadow, JackStouffer, MartinNowak, PetarKirov, andralex, dnadlinger, ibuclaw, jmdavis, kyllingstad, schveiguy and wilzbach as code owners December 18, 2017 14:04

wilzbach reviewed Dec 18, 2017

View reviewed changes

Revert unrelated changes requested by @wilzbach

58b5569

dkgroot changed the title ~~Dragonflybsd master~~ Add DragonFlyBSD support for phobos Dec 18, 2017

dkgroot and others added 10 commits December 20, 2017 04:07

Update FIXME message related to dragonfly malloc issue

bf06f45

Remove extraneous import (Review @ZombineDev)

ca27cdf

Make imports in std.net.curl local & lazy

a11f63f

Address review

4a1065e

Fix Issue 18092 - Can't combine take and takeExactly

1ad40c6

Remove docs build from the Windows Makefiles

cae4a30

Merge branch 'dragonflybsd-master' of github.com:dkgroot/phobos into …

931854f

…dragonflybsd-master

Port of phobos to DragonFlyBSD

31ca73d

Notes: - FIXME message related to dragonfly malloc issue (issue reported on upstream dragonfly issue database)

Make imports in std.net.curl local & lazy

c5b7317

Address review

106954f

Fix Issue 18092 - Can't combine take and takeExactly

b712abc

jmdavis approved these changes Dec 20, 2017

View reviewed changes

wilzbach and others added 2 commits December 20, 2017 08:07

Remove docs build from the Windows Makefiles

24f0c95

Merge branch 'dragonflybsd-master' of github.com:dkgroot/phobos into …

7245be5

…dragonflybsd-master

PetarKirov reviewed Dec 20, 2017

View reviewed changes

PetarKirov approved these changes Dec 20, 2017

View reviewed changes

dlang-bot added the Severity:Bug Fix label Dec 20, 2017

dkgroot force-pushed the dragonflybsd-master branch from aa3822f to cae4a30 Compare December 20, 2017 14:40

andralex merged commit fac1693 into dlang:master Dec 20, 2017

wilzbach mentioned this pull request Jan 9, 2018

Add a new DragonFlyBSD build host braddr/d-tester#62

Closed

Uh oh!

Comments

Conversation

dkgroot commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nemanja-boric-sociomantic Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdavis left a comment

Choose a reason for hiding this comment

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot commented Dec 20, 2017

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

dkgroot commented Dec 20, 2017

Uh oh!

ibuclaw commented Dec 20, 2017

Uh oh!

PetarKirov commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkgroot commented Dec 20, 2017

dkgroot commented Dec 18, 2017 •

edited

Loading

dlang-bot commented Dec 18, 2017 •

edited

Loading

dkgroot Dec 19, 2017 •

edited

Loading

nemanja-boric-sociomantic Dec 18, 2017 •

edited

Loading

dkgroot Dec 20, 2017 •

edited

Loading

PetarKirov commented Dec 20, 2017 •

edited

Loading

PetarKirov commented Dec 20, 2017 •

edited

Loading