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

merge socket constants #646

Closed
wants to merge 1 commit into from
Closed

merge socket constants #646

wants to merge 1 commit into from

Conversation

ndusart
Copy link
Contributor

@ndusart ndusart commented Jul 5, 2017

Refactor the constant declarations such that all constants are only declared once with a #[cfg] that only enables the constant on the correct platforms.

#637

@ndusart ndusart force-pushed the master branch 3 times, most recently from 830efc3 to 7ed7cf3 Compare July 5, 2017 11:01
pub const IPPROTO_UDP: c_int = SOL_UDP;
#[cfg(any(target_os = "macos", target_os = "freebsd", target_os = "ios", target_os = "openbsd", target_os = "netbsd"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

This line's too long. Please wrap lines to 100 characters. In this case, I like aligning all the target_os sections vertically. This applies to many other lines here, so please fix it for all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I split target_os in several lines for each case, or only when the line is longer than 100 characters ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only for long lines.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 5, 2017

Don't know why but buildbot keep failing at fetching the code..

@asomers
Copy link
Member

asomers commented Jul 5, 2017

I don't know why either, but buildbot is trying a different sequence of commands for this build than it uses for most PRs. Compare https://alan.ci/buildbot/#/builders/5/builds/200/steps/0/logs/stdio (this PR) to https://alan.ci/buildbot/#/builders/5/builds/79/steps/0/logs/stdio (PR 624). I wonder if it's confused because your branch name is master? Its my only guess at this point.

@ndusart
Copy link
Contributor Author

ndusart commented Jul 5, 2017

Buildbot actually made two tests for each of my commits:

For the last one:

The first one succeeded though.

It's probably the name of my branch, I should take the habit to avoid working directly on the master branch once I fork the repo.

Do I need to create a new PR with another branch ?

@Susurrus
Copy link
Contributor

Susurrus commented Jul 5, 2017

@ndusart Could you try filing another PR using a feature branch? Then we can check if that's indeed the issue.

@ndusart ndusart mentioned this pull request Jul 5, 2017
@ndusart
Copy link
Contributor Author

ndusart commented Jul 5, 2017

This seems to be effectively the issue ! Succeed now with the new PR.

@Susurrus
Copy link
Contributor

Susurrus commented Jul 5, 2017

That's annoying. @asomers Looks like another bug that should be filed with the upstream buildbot project.

@Susurrus
Copy link
Contributor

Susurrus commented Jul 6, 2017

Closing in favor of #647.

@Susurrus Susurrus closed this Jul 6, 2017
bors bot added a commit that referenced this pull request Aug 1, 2017
647: merge socket constants r=Susurrus

> Refactor the constant declarations such that all constants are only declared once with a #[cfg] that only enables the constant on the correct platforms.

Closes #637 

(same PR as #646 but from another branch, to see if buildbot has a problem with PR made from master branch)
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.

None yet

3 participants