Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

NetBSD path#1492

Merged
dlang-bot merged 1 commit intodlang:masterfrom
sibnick:netbsd_patch
Mar 11, 2017
Merged

NetBSD path#1492
dlang-bot merged 1 commit intodlang:masterfrom
sibnick:netbsd_patch

Conversation

@sibnick
Copy link
Contributor

@sibnick sibnick commented Feb 3, 2016

It is LDC based port. My pull request merged to LDC master already.
Druntime:
I decide try to repeat LDC team forkflow and cherry-pick my commit to ldc branch. There are a couple simple conflicts in src/core/stdc/locale.d & src/core/sys/posix/signal.d, and one absent (in master) file: src/rt/sections_ldc.d
I successfully build and run druntime unittests on NetBSD after resolving conflicts.

@joakim-noah
Copy link
Contributor

I suggest you split this up into 2-3 separate PRs, as it'll be a bear to review a 6k-line PR.

Are you generally just copy-pasting the FreeBSD portions, or did you check if NetBSD does it differently? For example, core.sys.netbsd appears to be a straight copy of core.sys.freebsd, did you spend any time verifying that it is the same for NetBSD?

@sibnick
Copy link
Contributor Author

sibnick commented Feb 4, 2016

Yes I checked. Actually many of them is not the same!
You can compare files:

core/sys/freebsd/time.d and core/sys/netbsd/time.d
core/sys/freebsd/sys/event.d and core/sys/netbsd/sys/event.d

Can you suggest rule for patch splitting?

@sibnick
Copy link
Contributor Author

sibnick commented Feb 4, 2016

I successfully run unit test (phobos, druntime), built dub (small issue in make file), run vibed unit tests, started several vibe.d samples. I found one trouble with web sockets (works with firefox, core dumped with chrome). I do not discover this issue yet.

@joakim-noah
Copy link
Contributor

A way to split it that makes sense to me would be to submit your core.sys.posix and core.sys.netbsd changes as separate PRs, and keep the rest in this PR.

Good to hear you got dub and vibe.d running. :)

@sibnick
Copy link
Contributor Author

sibnick commented Feb 4, 2016

I splited pull request as you suggest
netbsd patch: core/sys/netbsd #1493
netbsd patch: core/sys/posix #1494

@sibnick
Copy link
Contributor Author

sibnick commented Feb 4, 2016

Can we start from 1493, 1494 pull requests? May by I will change src/core/stdc/math.d (I am not sure). NetBSD has stubs for several long double (real type in dlang) functions. May be I will fix at least some of them. Actually I do it for logl function already.

enum EMULTIHOP = 94 /** Multihop attempted */;
enum ENOLINK = 95 /** Link has been severed */;
enum EPROTO = 96 /** Protocol error */;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you got these comments from the official NetBSD header, please replace them with blank comments, as seen for Linux. Comments are copyrightable, it is Walter's policy not to copy them.

@joakim-noah
Copy link
Contributor

Simply skimmed patch to make sure it won't mess with other platforms, ie everything is encapsulated for NetBSD, LGTM from that standpoint, plus found two formatting errors that should be fixed.

@sibnick
Copy link
Contributor Author

sibnick commented Feb 8, 2016

remove comments and empty line

@joakim-noah
Copy link
Contributor

Thanks, while I haven't checked the NetBSD declarations and functions, I see no harm in merging, since everything is encapsulated.

@sibnick sibnick force-pushed the netbsd_patch branch 7 times, most recently from fd421bc to 1118ff7 Compare March 13, 2016 11:03
@joakim-noah
Copy link
Contributor

joakim-noah commented Jan 26, 2017

Looks like this and the other NetBSD PRs have rotted, after nobody merged them. Can you update them so auto-tester passes again and we'll try to get them merged finally?

@sibnick
Copy link
Contributor Author

sibnick commented Jan 30, 2017

I updated all 3 PRs with NetBSD patches

@codecov-io
Copy link

codecov-io commented Jan 31, 2017

Codecov Report

❗ No coverage uploaded for pull request head (netbsd_patch@49391ee).


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0bf08d...49391ee. Read the comment docs.

@joakim-noah
Copy link
Contributor

joakim-noah commented Jan 31, 2017

Looks like you need to rebase.

@sibnick
Copy link
Contributor Author

sibnick commented Feb 1, 2017

Rebase is performed. Initially I performed rebase with wrong branch.

@sibnick sibnick force-pushed the netbsd_patch branch 2 times, most recently from a79e0c5 to e0f61f4 Compare February 10, 2017 11:28
@sibnick
Copy link
Contributor Author

sibnick commented Feb 11, 2017

auto-tester passes on this pull request, but other two requests have pending items in auto-tester. I am not sure what does it means

@joakim-noah
Copy link
Contributor

This NetBSD port, including #1493 and #1494, has been in limbo for a year now. Calling @MartinNowak, @ibuclaw, @WalterBright, can we get some feedback on what else needs to be done to get this merged?

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

Can't see anything wrong.

@dnadlinger
Copy link
Contributor

I just left this originally as I wanted to get some LDC-external reviews as well. @nrTQgc, could you please rebase so we can merge?

@sibnick
Copy link
Contributor Author

sibnick commented Mar 11, 2017

I performed rebase

@dlang-bot dlang-bot merged commit 7caaf7c into dlang:master Mar 11, 2017
// alias core.stdc.complex.crealf creal;
// alias core.stdc.complex.creall creal;
}

Copy link
Contributor

@joakim-noah joakim-noah Mar 11, 2017

Choose a reason for hiding this comment

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

This blank line slipped through, remove it in one of the already submitted NetBSD PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in netbsd_core_sys_posix

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants