-
Notifications
You must be signed in to change notification settings - Fork 666
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
fix, mostly, solaris build. #2248
Conversation
changelog/2248.fixed.md
Outdated
@@ -0,0 +1,2 @@ | |||
Fixed solaris build globally, also disabling a subset of `InterfaceFlags` values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're mixing past and present tense in the same sentence here. And what does the second part mean? Without reading the code, I can't understand why you disabled a subset of InterfaceFlags
.
src/net/if_.rs
Outdated
@@ -213,26 +213,29 @@ libc_bitflags!( | |||
/// Interface is offline | |||
#[cfg(solarish)] | |||
IFF_OFFLINE; | |||
#[cfg(target_os = "solaris")] | |||
/* | |||
// FIXME: The following are from 64 bits long long type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's wrong here? Do these not work correctly at runtime? You should open a new issue for this and explain what the problem is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarize, all the values up to IFF_OFFLINE
are c_int but after that they are c_longlong thus it does not build because the nix macro expects a i32 across the board.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we cast them correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you have in mind ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you have in mind ?
Cast them with as c_int
if they are in the range of int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is, for most of them it would overflow from this one at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we cast them correctly?
Then we can not do this
b1f5257
to
f64e92d
Compare
changelog/2248.fixed.md
Outdated
@@ -0,0 +1,2 @@ | |||
Fixed solaris build globally, also removed a subset of `InterfaceFlags` values | |||
which are of i64 type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"which are of i64 type" still doesn't make sense to a typical reader. Why were they removed? Did they not work at runtime, or did they generate build errors?
Also, the ", also" creates a run-on sentence. You should use a proper conjunction or split it into two sentences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That changelog edit still doesn't answer the question. Did the interfaceflags fail at runtime, or generate build errors? Also, why did you elect to remove some of them, rather than just change the field width to 64 bits on Solarish?
d310377
to
138adca
Compare
With few exceptions as newer interfaces like preadv/pwritev unsupported only on solaris.
138adca
to
fa2aabb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That changelog edit
No description provided.