Split up Solaris and illumos targets #1716

pfmooney · 2020-04-03T02:50:09Z

While they share a common lineage, Solaris and illumos are distinct (and diverging) platforms which should be handled differently in libc. This PR splits them up and addresses some all of the libc-test failures which appeared on illumos. I have a separate branch for the libc-test updates, but given that ctest does not support illumos today, and the maintainer appears to be absent, those updates only function with that dependency overridden.

This is one of the last remaining pre-req updates required for rust-lang/rust#55553

rust-highfive · 2020-04-03T02:50:12Z

r? @JohnTitor

(rust_highfive has picked a reviewer for you, use r? to override)

pfmooney

Here are some comments to help contextualize some of the changes.

pfmooney · 2020-04-03T02:50:57Z

src/unix/solarish/mod.rs

@@ -1361,7 +1312,7 @@ pub const RLIMIT_AS: ::c_int = RLIMIT_VMEM;

 #[deprecated(since = "0.2.64", note = "Not stable across OS versions")]
 pub const RLIM_NLIMITS: rlim_t = 7;
-pub const RLIM_INFINITY: rlim_t = 0x7fffffff;
+pub const RLIM_INFINITY: rlim_t = 0xfffffffffffffffd;


This was previously using the 32-bit value. Given that all of the illumos/Solaris targets are 64-bit today, it made sense to use that value.

Wouldn’t it be a good idea to make this definition different for 32 and 64 bit targets ?

The existing builds for rust on both Solaris and illumos target only x86_64. If/when someone is inclined to set up a 32-bit build, they'll need to check all of the libc interfaces, this constant included, for conformance.

pfmooney · 2020-04-03T02:51:20Z

src/unix/solarish/mod.rs

@@ -1018,7 +969,7 @@ pub const O_CREAT: ::c_int = 256;
 pub const O_EXCL: ::c_int = 1024;
 pub const O_NOCTTY: ::c_int = 2048;
 pub const O_TRUNC: ::c_int = 512;
-pub const O_NOFOLLOW: ::c_int = 0x200000;
+pub const O_NOFOLLOW: ::c_int = 0x20000;


This appears to have been a typo during the initial import.

pfmooney · 2020-04-03T02:51:59Z

src/unix/solarish/mod.rs

+pub const CSTART: ::tcflag_t = 0o21;
+pub const CSTOP: ::tcflag_t = 0o23;
+pub const CSWTCH: ::tcflag_t = 0o32;


These were transcription errors, given they were 0-prefixed octal values in the source header

pfmooney · 2020-04-03T02:54:11Z

src/unix/solarish/solaris.rs

+s_no_extra_traits! {
+    #[cfg_attr(feature = "extra_traits", allow(missing_debug_implementations))]
+    pub union door_desc_t__d_data {
+        pub d_desc: door_desc_t__d_data__d_desc,
+        d_resv: [::c_int; 5], /* Check out /usr/include/sys/door.h */
+    }
+
+    #[cfg_attr(feature = "extra_traits", allow(missing_debug_implementations))]
+    pub struct door_desc_t {
+        pub d_attributes: door_attr_t,
+        pub d_data: door_desc_t__d_data,
+    }
+
+    #[cfg_attr(feature = "extra_traits", allow(missing_debug_implementations))]
+    pub struct door_arg_t {
+        pub data_ptr: *const ::c_char,
+        pub data_size: ::size_t,
+        pub desc_ptr: *const door_desc_t,
+        pub dec_num: ::c_uint,
+        pub rbuf: *const ::c_char,
+        pub rsize: ::size_t,
+    }
+}


Being not thrilled with the type naming mess, I've hidden the doors API from the new illumos target for now. We can circle back in the future to address it. At least one error was made when it was imported (dec_num should be desc_num in door_arg_t, for example)

JohnTitor · 2020-04-03T06:27:14Z

Thanks for the PR!

I have a separate branch for the libc-test updates, but given that ctest does not support illumos today, and the maintainer appears to be absent, those updates only function with that dependency overridden.

I'd like to fork ctest repo and use that repo until @gnzlbg gets back since I have no permission on the original repo. Would you mind sending PR(s) to the repo?

pfmooney · 2020-04-03T12:22:39Z

Thanks for the PR!

I have a separate branch for the libc-test updates, but given that ctest does not support illumos today, and the maintainer appears to be absent, those updates only function with that dependency overridden.

I'd like to fork ctest repo and use that repo until @gnzlbg gets back since I have no permission on the original repo. Would you mind sending PR(s) to the repo?

Yes, of course! Some of these issues were frustrating to find, as they would have been caught by the tests (had they actually been run).

pfmooney · 2020-04-07T21:41:34Z

The additional ctest-related commit fixes the test suite for illumos, and presumably improves the situation for Solaris. I have no Solaris(tm) environment to test in. It does require a local ctest dependency with JohnTitor/ctest2#1 merged (or equivalent).

pfmooney · 2020-04-07T22:15:41Z

If/when this is merged, what are your thoughts about a patch version bump for the package? Do you have some preferred criteria for what's necessary to meet that threshold?

JohnTitor · 2020-04-07T22:48:21Z

If/when this is merged, what are your thoughts about a patch version bump for the package? Do you have some preferred criteria for what's necessary to meet that threshold?

Usually, I release a new version if requested as mentioned in contributing guide. So if you want me to release it once this PR gets merged, I'm happy to do so :)

pfmooney · 2020-04-07T22:51:01Z

Usually, I release a new version if requested as mentioned in contributing guide. So if you want me to release it once this PR gets merged, I'm happy to do so :)

Once this is merged, I'll circle back to do another round of testing with the rust-lang/rust#55553 wad and then open a ticket here requesting the version bump.

Thanks!

pfmooney · 2020-04-10T18:48:31Z

With the tests added, are there any other changes you'd like to see made to this before it goes in?

JohnTitor

Looks good to me, sorry for the delay!

pfmooney · 2020-04-11T22:38:54Z

Looks good to me, sorry for the delay!

No worries! Thanks.

rust-highfive assigned JohnTitor Apr 3, 2020

pfmooney commented Apr 3, 2020

View reviewed changes

pfmooney mentioned this pull request Apr 3, 2020

Create a separate target for illumos rust-lang/rust#55553

Closed

Split up Solaris and illumos targets

3d65979

pfmooney force-pushed the illumos-target branch from 374704a to 3d65979 Compare April 3, 2020 03:34

Update ctest for illumos and Solaris

48594dc

JohnTitor approved these changes Apr 11, 2020

View reviewed changes

JohnTitor merged commit 9782b7f into rust-lang:master Apr 11, 2020

pfmooney deleted the illumos-target branch July 17, 2020 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split up Solaris and illumos targets #1716

Split up Solaris and illumos targets #1716

pfmooney commented Apr 3, 2020

rust-highfive commented Apr 3, 2020

pfmooney left a comment

pfmooney Apr 3, 2020

GuillaumeDIDIER Apr 10, 2020

pfmooney Apr 10, 2020

pfmooney Apr 3, 2020

pfmooney Apr 3, 2020

pfmooney Apr 3, 2020

JohnTitor commented Apr 3, 2020 •

edited

Loading

pfmooney commented Apr 3, 2020

pfmooney commented Apr 7, 2020

pfmooney commented Apr 7, 2020

JohnTitor commented Apr 7, 2020

pfmooney commented Apr 7, 2020

pfmooney commented Apr 10, 2020

JohnTitor left a comment

pfmooney commented Apr 11, 2020

Split up Solaris and illumos targets #1716

Split up Solaris and illumos targets #1716

Conversation

pfmooney commented Apr 3, 2020

rust-highfive commented Apr 3, 2020

pfmooney left a comment

Choose a reason for hiding this comment

pfmooney Apr 3, 2020

Choose a reason for hiding this comment

GuillaumeDIDIER Apr 10, 2020

Choose a reason for hiding this comment

pfmooney Apr 10, 2020

Choose a reason for hiding this comment

pfmooney Apr 3, 2020

Choose a reason for hiding this comment

pfmooney Apr 3, 2020

Choose a reason for hiding this comment

pfmooney Apr 3, 2020

Choose a reason for hiding this comment

JohnTitor commented Apr 3, 2020 • edited Loading

pfmooney commented Apr 3, 2020

pfmooney commented Apr 7, 2020

pfmooney commented Apr 7, 2020

JohnTitor commented Apr 7, 2020

pfmooney commented Apr 7, 2020

pfmooney commented Apr 10, 2020

JohnTitor left a comment

Choose a reason for hiding this comment

pfmooney commented Apr 11, 2020

JohnTitor commented Apr 3, 2020 •

edited

Loading