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

fix, mostly, solaris build. #2248

Merged
merged 1 commit into from
Dec 8, 2023
Merged

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Dec 3, 2023

No description provided.

@devnexen devnexen marked this pull request as ready for review December 3, 2023 18:16
@@ -0,0 +1,2 @@
Fixed solaris build globally, also disabling a subset of `InterfaceFlags` values
Copy link
Member

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.
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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 ?

Copy link
Member

@SteveLauC SteveLauC Dec 4, 2023

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

Copy link
Contributor Author

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.

Copy link
Member

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

@devnexen devnexen force-pushed the fix_solaris_build branch 2 times, most recently from b1f5257 to f64e92d Compare December 6, 2023 19:25
@@ -0,0 +1,2 @@
Fixed solaris build globally, also removed a subset of `InterfaceFlags` values
which are of i64 type.
Copy link
Member

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.

Copy link
Member

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?

@devnexen devnexen force-pushed the fix_solaris_build branch 2 times, most recently from d310377 to 138adca Compare December 8, 2023 19:06
With few exceptions as newer interfaces like preadv/pwritev unsupported
only on solaris.
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

That changelog edit

@asomers asomers added this pull request to the merge queue Dec 8, 2023
Merged via the queue into nix-rust:master with commit 2ab5558 Dec 8, 2023
35 checks passed
@SteveLauC SteveLauC mentioned this pull request Dec 9, 2023
3 tasks
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