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

Comments

Add SO_REUSEPORT constant#1879

Merged
dlang-bot merged 1 commit intodlang:masterfrom
Burgos:soreusse
Jul 18, 2017
Merged

Add SO_REUSEPORT constant#1879
dlang-bot merged 1 commit intodlang:masterfrom
Burgos:soreusse

Conversation

@Burgos
Copy link
Contributor

@Burgos Burgos commented Jul 17, 2017

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jul 17, 2017

Thanks for your pull request, @Burgos!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jul 17, 2017
@CyberShadow
Copy link
Member

This doesn't look like it's going to fix issue 17416, as this is just the druntime-part of the change; so, please remove "Fix" from the commit message.

@Burgos
Copy link
Contributor Author

Burgos commented Jul 18, 2017

This is what I was wondering about. Looking into SocketOptions I don't see any non-portable constant there, so I didn't want to add this one here (Windows being one platform that doesn't support SO_REUSEPORT).

@CyberShadow
Copy link
Member

std.socket does have some platform-specific things in it (e.g. UnixAddress), so I think adding it to SocketOption on supported platforms would be fine.

@Burgos
Copy link
Contributor Author

Burgos commented Jul 18, 2017

Fair enough. I'll follow up with photos PR and I will amend this commit.

@Burgos
Copy link
Contributor Author

Burgos commented Jul 18, 2017

Commit reworded

@Burgos Burgos changed the title Fix issue 17416: Add SO_REUSEPORT constant Add SO_REUSEPORT constant Jul 18, 2017
@CyberShadow
Copy link
Member

CyberShadow commented Jul 18, 2017

Thanks. BTW, you didn't need to remove the bugzilla reference entirely, just the word "Fix". That would still mark the PR as related to the issue, but not close it when the PR is merged (or cause the issue to appear in the changelog should the corresponding Phobos issue somehow not make the merge window).

@dlang-bot dlang-bot merged commit d9dcfa7 into dlang:master Jul 18, 2017
@rainers
Copy link
Member

rainers commented Jul 19, 2017

This has caused an ambiguity error building libevent by the jenkins CI (from the logs it seems SO_REUSEPORT is defined there, too).
What's the procedure in this case? Update the projects that have that problem?

rainers added a commit to rainers/vibe.d that referenced this pull request Jul 19, 2017
SO_REUSEPORT is defined in core.sys.posix.sys.socket by dlang/druntime#1879, producing an ambiguity error.

Without an import, what symbol does the existing version check against?
@CyberShadow
Copy link
Member

This has caused an ambiguity error building libevent by the jenkins CI (from the logs it seems SO_REUSEPORT is defined there, too).

Aaaahh. Bugger.

The error is in vibe-core's libevent2 driver.

@wilzbach @s-ludwig What do you think, should we revert this? We still have a full release cycle ahead of us, so there may be enough time to simply update affected packages.

What's the procedure in this case? Update the projects that have that problem?

Aside from updating the packages, I think DIP 1007 is meant to solve this specific problem, but I don't know if it's worth going that route for such a small addition.

@s-ludwig
Copy link
Member

From my side it would be absolutely okay to proceed with this now. The next vibe.d release phase start is planned for the next days, so there should be plenty of time to have it forward compatible.

@wilzbach
Copy link
Contributor

@wilzbach @s-ludwig What do you think, should we revert this? We still have a full release cycle ahead of us, so there may be enough time to simply update affected packages.

I am getting a whole lot of red CI errors as I test dmd-nightly on e.g. dlang-bot or DTour, so I would prefer if we can revert this until the new vibe.d release is out.
Also Jenkins was green for one day and then you broke it again ;-)

@Burgos
Copy link
Contributor Author

Burgos commented Jul 20, 2017

Yeah, sorry, I didn't expect of Jenkins to be green ;-). Well, lessons learned, thanks!

@CyberShadow
Copy link
Member

@Burgos Don't worry about it :)

@s-ludwig
Copy link
Member

@wilzbach What about if I tag a quick pre-release and the tests for dlang-bot etc. that test on DMD nightly get changed to test with dub upgrade --prerelease? That should generally cause a less friction in cases like this one.

@wilzbach
Copy link
Contributor

@wilzbach What about if I tag a quick pre-release and the tests for dlang-bot etc. that test on DMD nightly get changed to test with dub upgrade --prerelease? That should generally cause a less friction in cases like this one.

Wouldn't be a big deal for dlang-bot or the DTour, but I am not so sure for the Project Tester - I think prereleases are filtered: https://github.com/dlang/ci/blob/master/pipeline.groovy#L60
Of course sth. like dlang/ci#54 would always be possible, but I'm not sure whether it's worth it.

FYI: I have already "reverted the revert", s.t. it's just a click away for the next release:

#1887

Btw isn't this what __future was intended for?

@Burgos
Copy link
Contributor Author

Burgos commented Jul 20, 2017

Btw isn't this what __future was intended for?

Yes, it's mentioned in this comment:

Aside from updating the packages, I think DIP 1007 is meant to solve this specific problem, but I don't know if it's worth going that route for such a small addition.

Perhaps we could use it, to prevent further breakage if there are more projects with the same problem? SO_REUSEPORT is a constant in high demand :-).

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

Labels

Bug Fix Include reference to corresponding bugzilla issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants