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.

Sign up for GitHub

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

Jump to bottom

x86_64-sun-solaris target was removed without warning in Rust 1.52 #85098

Closed
jhpratt opened this issue May 9, 2021 · 9 comments · Fixed by #85252
Closed

x86_64-sun-solaris target was removed without warning in Rust 1.52 #85098

jhpratt opened this issue May 9, 2021 · 9 comments · Fixed by #85252
Labels
A-target-specs Area: compile-target specifications regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Milestone
1.53.0

Comments

@jhpratt
Copy link
Member

jhpratt commented May 9, 2021 •
edited
Loading

#82216 removed the alias of x86_64-sun-solaris to x86_64-pc-solaris, but as far as I can tell did not remove the target itself.

After inquiring on Zulip, it was pointed out that the sun-solaris target was demoted to tier three, meaning builds are no longer provided. It appears this was done with no discussion, warning, or even a mention in the release notes.

Given that the pc-solaris target hasn't been around for that long (I think? feel free to tell me I'm wrong), I think this change should be reverted. My CI unexpectedly failed after being unable to download the target, and the only solution that wouldn't involve a notable increase in complexity of the CI script is to outright remove the checks for Solaris/Illumos — a let loss for everyone.

cc @Mark-Simulacrum
@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-target-specs Area: compile-target specifications labels May 9, 2021
@jonas-schievink jonas-schievink added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels May 9, 2021
@Mark-Simulacrum
Copy link
Member

I think my feeling right now is that we should likely re-enable the sun-solaris target alongside the pc-solaris, I don't think that should be a major problem for CI times or similar.

I will see if we can do this in time for 1.52.1, but if it ends up being harder than expected, I think it's likely that either a) we'll push it out to a 1.52.2 release later in the cycle, or simply leave it to 1.53. For the time being, tagging as 1.53.

@Mark-Simulacrum Mark-Simulacrum added this to the 1.53.0 milestone May 9, 2021
@jhpratt
Copy link
Member Author

jhpratt commented May 9, 2021

I will note that the change itself is fine, of course, it's the suddenness and unexpectedness of it that caught me by surprise.

@Mark-Simulacrum
Copy link
Member

I think it is likely true that we'll eventually drop the sun target, but it does seem like we should communicate that change better at least. Certainly the newly merged target tier policy suggests just that:

A tier 2 target may be demoted or removed if it no longer meets these requirements. Any proposal for demotion or removal will be CCed to the target maintainers, and will be communicated widely to the Rust community before being dropped from a stable release. (The amount of time between such communication and the next stable release may depend on the nature and severity of the failed requirement, the timing of its discovery, whether the target has been part of a stable release yet, and whether the demotion or removal can be a planned and scheduled action.)

@Mark-Simulacrum
Copy link
Member

It is not clear to me what the impact of this change was. In particular, the produced target was not changed in #82216, but I guess the target definition is sort of https://github.com/rust-lang/rust/blob/1.52.0/compiler/rustc_target/src/spec/x86_64_unknown_illumos.rs#L12 for pc...

I think I am not sufficiently confident in moving in a particular direction here - and a full revert does not seem quite right either - so I will likely say this should be left for at least 1.52.2 or 1.53.0.

cc @nagisa as well, who reviewed #82216

Thomasdezeeuw added a commit to Thomasdezeeuw/mio that referenced this issue May 10, 2021
@Thomasdezeeuw
Replace x86_64-sun-solaris with x86_64-pc-solaris
9ee25e0 
rust-lang/rust#82216 removed the
x86_64-sun-solaris target from rustup, changing it to use
x86_64-pc-solaris instead.

Related issues:
 * rust-lang/rust#85098
@Thomasdezeeuw Thomasdezeeuw mentioned this issue May 10, 2021
Closed
@kulikjak
Copy link
Contributor

kulikjak commented May 10, 2021 •
edited
Loading

Removal of the older target was certainly not my intent; if that happened due to #82216, I am sorry.

Both x86_64_sun_solaris.rs and x86_64_pc_solaris.rs are there and both targets are usable when compiling with rustc, but I guess that change to one of src/ci/docker/... files might have resulted in artifacts necessary for rustup no longer being available.

josephlr added a commit to rust-random/getrandom that referenced this issue May 13, 2021
@josephlr
Fix Solaris CI
4a2d602 
The new target is called `x86_64-pc-solaris`.

See the following issues for more info:
  rust-lang/rust#85098
  rust-lang/rust#82216

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr josephlr mentioned this issue May 13, 2021
Merged
josephlr added a commit to rust-random/getrandom that referenced this issue May 13, 2021
@josephlr
Fix Solaris CI
ef2e61a 
The new target is called `x86_64-pc-solaris`, but `cross` doesn't
support that yet, so just use `sparcv9-sun-solaris`.

See the following issues for more info:
  rust-lang/rust#85098
  rust-lang/rust#82216

Signed-off-by: Joe Richey <joerichey@google.com>
josephlr added a commit to rust-random/getrandom that referenced this issue May 13, 2021
@josephlr
Fix Solaris CI
ff72837 
The new target is called `x86_64-pc-solaris`, but `cross` doesn't
support that yet, so just use `sparcv9-sun-solaris`.

See the following issues for more info:
  rust-lang/rust#85098
  rust-lang/rust#82216

Signed-off-by: Joe Richey <joerichey@google.com>
josephlr added a commit to rust-random/getrandom that referenced this issue May 13, 2021
@josephlr
Fix Solaris CI
c94cb75 
The new target is called `x86_64-pc-solaris`, but `cross` doesn't
support that yet, so just use `sparcv9-sun-solaris`.

See the following issues for more info:
  rust-lang/rust#85098
  rust-lang/rust#82216

Signed-off-by: Joe Richey <joerichey@google.com>
@kulikjak kulikjak mentioned this issue May 13, 2021
Merged
@kulikjak
Copy link
Contributor

I created #85252, which I believe should bring back the ability to rustup the old target.

Thomasdezeeuw added a commit to Thomasdezeeuw/mio that referenced this issue May 13, 2021
@Thomasdezeeuw
Replace x86_64-sun-solaris with x86_64-pc-solaris
b21a5ee 
rust-lang/rust#82216 removed the
x86_64-sun-solaris target from rustup, changing it to use
x86_64-pc-solaris instead.

Related issues:
 * rust-lang/rust#85098
Thomasdezeeuw added a commit to Thomasdezeeuw/socket2 that referenced this issue May 13, 2021
@Thomasdezeeuw
Replace x86_64-sun-solaris with x86_64-pc-solaris
40ce945 
rust-lang/rust#82216 removed the
x86_64-sun-solaris target from rustup, changing it to use
x86_64-pc-solaris instead.

Related issues:
 * rust-lang/rust#85098
@Thomasdezeeuw Thomasdezeeuw mentioned this issue May 13, 2021
Merged
Thomasdezeeuw added a commit to rust-lang/socket2 that referenced this issue May 13, 2021
@Thomasdezeeuw
Replace x86_64-sun-solaris with x86_64-pc-solaris
fcfa119 
rust-lang/rust#82216 removed the
x86_64-sun-solaris target from rustup, changing it to use
x86_64-pc-solaris instead.

Related issues:
 * rust-lang/rust#85098
Thomasdezeeuw added a commit to tokio-rs/mio that referenced this issue May 13, 2021
@Thomasdezeeuw
Replace x86_64-sun-solaris with x86_64-pc-solaris
607a12f 
rust-lang/rust#82216 removed the
x86_64-sun-solaris target from rustup, changing it to use
x86_64-pc-solaris instead.

Related issues:
 * rust-lang/rust#85098
newpavlov pushed a commit to rust-random/getrandom that referenced this issue May 14, 2021
@josephlr
Fix Solaris CI (#216)
922d1de 
The new target is called `x86_64-pc-solaris`, but `cross` doesn't
support that yet, so just use `sparcv9-sun-solaris`.

See the following issues for more info:
  rust-lang/rust#85098
  rust-lang/rust#82216

Signed-off-by: Joe Richey <joerichey@google.com>
jhpratt added a commit to time-rs/time that referenced this issue May 15, 2021
@jhpratt
Remove checks for Solaris
f37791e 
See rust-lang/rust#85098 for details.
@bors bors closed this as completed in 54bdfa1 May 26, 2021
@Mark-Simulacrum
Copy link
Member

Reopening to track the illumos target work. I'm not sure it's a release blocker, but would like to not lose sight.

@Mark-Simulacrum
Copy link
Member

The remaining bug in this issue -- the change in the compiler used to build illumos to be based on a -pc- triple, not -sun- here https://github.com/rust-lang/rust/blob/master/src/ci/docker/scripts/illumos-toolchain.sh#L21 is not a release blocker.

bdbai added a commit to YtFlow/mio-noafd that referenced this issue Feb 9, 2022
@bdbai
Squashed commit of the following:
535717f 
commit 5234b5f
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sat Nov 13 12:57:29 2021 +0100

    Release v0.8.0

commit 41a494b
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sat Nov 13 12:59:43 2021 +0100

    Fix Clippy warning

commit a8c5756
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sun Nov 7 12:45:33 2021 +0100

    Add changelog for v0.8

commit 7029a35
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sun Nov 7 12:34:33 2021 +0100

    Add v0.7.14 change log

    From commit 064af84

commit dca2134
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sun Nov 7 11:26:23 2021 +0100

    Fix feature flags for some tests files

    The test util module requires both the "os-poll" and "net" features.

commit b9f089b
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sun Nov 7 11:19:09 2021 +0100

    Remove cfg attributes for Solaris

    We never really supported Solaris, we pretended it implemented epoll,
    but it never did see tokio-rs#1152. As no
    one ever committed to being a maintainer for the port I'm removing it
    now with this commit.

    Instead replace it with illumuos on the CI, which we do support (as it
    supports epoll) and for which we do have maintainers.

commit 7d86108
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sun Nov 7 11:53:32 2021 +0100

    Add section about raw fd to portability guidelines

commit 3be5811
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sun Nov 7 12:09:53 2021 +0100

    Add note about short receive on datagram sockets

    Talking about the differences between OSs.

commit 3ca57f3
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sat Nov 6 15:20:54 2021 +0100

    Document unconnected TcpStream returned by TcpStream::connect

commit 47cf59c
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sat Nov 6 15:04:56 2021 +0100

    Deregister connection before dropping it in TCP example

commit 05009e4
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sat Nov 6 14:53:45 2021 +0100

    Document that Mio report OOB data in Event::is_readable

    Reporting Out-of-band (OOB) as readable it could leave applications open
    to DoS attacks. However because Mio uses edge-triggers most applications
    won't actually be effected.

commit 44666e8
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sat Nov 6 14:41:54 2021 +0100

    Fix match_like_matches_macro Clippy lint

    We've updated our MSVR since the comment above it.

commit f8695a7
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sat Nov 6 14:39:42 2021 +0100

    Update Rustc nightly version in CI

commit f4b9252
Author: Ben Noordhuis <info@bnoordhuis.nl>
Date:   Sun Oct 10 16:45:25 2021 +0200

    Add sys::unix::SocketAddr::as_abstract_namespace()

    Fixes tokio-rs#1517.

commit 04e0ca4
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Tue Sep 28 18:16:01 2021 +0200

    Update change log with v0.7.x releases

    Contains the work in the following commits:
     * v0.7.8 20b7298.
     * v0.7.9 07bc32f.
     * v0.7.10 b7006d7.
     * v0.7.11 772c692.
     * v0.7.12 7adfb75.
     * v0.7.13 75f41fb.

commit e55ec59
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Thu Oct 7 20:21:22 2021 +0200

    Install nightly Rust on CI for install cargo-hack

commit 499004f
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Thu Oct 7 20:14:36 2021 +0200

    Install Cargo-hack using nightly on CI

    Cargo-hack's (transient) dependency bitflags has updated its MSRV.

commit e9e91ff
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Tue Sep 28 19:59:15 2021 +0200

    Fix Clippy warnings on Windows

    Seems this isn't check on the CI.

commit b48cce6
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Tue Sep 28 19:51:19 2021 +0200

    Fix Clippy warnings

commit 37aec3e
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Tue Sep 28 19:45:26 2021 +0200

    Fix dead_code warnings for Windows

commit 02e9be4
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Tue Sep 28 19:32:35 2021 +0200

    Remove TcpSocket type

    The socket2 crate provide all the functionality and more. Furthermore
    supporting all socket options is beyond the scope of Mio.

    The easier migration is to the socket2 crate, using the Socket or
    SockRef types.

    The migration for Tokio is tracked in
    tokio-rs/tokio#4135.

commit d4ce420
Author: Rémi Lauzier <remilauzier@protonmail.com>
Date:   Tue Jul 6 14:21:17 2021 -0400

    Update dev-dependencies

commit fbcc849
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sat Jul 3 12:44:57 2021 +0200

    Change port in connect_error

    Hopefully this port is actually not used.

    Also check Event::is_write_closed since we expect that to be true.

commit bfbcd9d
Author: Jake Shadle <jake.shadle@embark-studios.com>
Date:   Fri Jul 2 15:17:17 2021 +0200

    Move wine from unsupported

commit 21ddf94
Author: Ivan Enderlin <ivan@mnt.io>
Date:   Tue Jun 22 22:36:09 2021 +0200

    chore: Make Clippy happy (bis).

commit 6d62f5d
Author: Ivan Enderlin <ivan@mnt.io>
Date:   Mon Jun 21 16:41:21 2021 +0200

    chore: Make Clippy happy.

commit 6eb1efa
Author: Ivan Enderlin <ivan@mnt.io>
Date:   Mon Jun 21 16:22:16 2021 +0200

    feat: Move `poll::selector` to `Registry::selector`.

commit 441367b
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sun Jun 13 00:33:17 2021 +0200

    Fix Selector::try_clone

    Calls fcntl F_DUPFD_CLOEXEC expects two arguments; the command
    (F_DUPFD_CLOEXEC) and an argument for the command. In this case an lower
    bound for the resulting file descriptor. Because we didn't provide a
    value it would take whatever value was left in the register from
    whatever code used it before the system call.

    This caused Waker::new to fail, see issue
    tokio-rs#1497.

commit cbcaedf
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Sat Jun 12 20:39:12 2021 +0200

    Set FD_CLOEXEC flag on duplicated kqueue Poll

    Same as commit c52635c, but for kqueue.

commit c52635c
Author: Tim Zhang <tim@hyper.sh>
Date:   Tue May 25 11:40:54 2021 +0800

    Set the close-on-exec flag for the duplicate epoll_fd

    The close-on-exec flag (FD_CLOEXEC; see fcntl(2)) for the
    duplicate descriptor created by dup(2) is off.

    We can use fcntl + F_DUPFD_CLOEXEC to dup the epoll_fd to fix this
    issue.

    Fixes: tokio-rs/tokio#3809

    Signed-off-by: Tim Zhang <tim@hyper.sh>

commit 2246ffb
Author: Taiki Endo <te316e89@gmail.com>
Date:   Sun May 23 16:06:15 2021 +0900

    Use ubuntu-18.04 instead of ubuntu-16.04

commit 0cfba5d
Author: cdcode <cdcode@airmail.cc>
Date:   Sun Jun 6 22:42:26 2021 +0100

    Small spelling correction in example

commit 22e8858
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Thu May 13 17:09:57 2021 +0200

    Update outdated comment

commit 607a12f
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Mon May 10 12:10:28 2021 +0200

    Replace x86_64-sun-solaris with x86_64-pc-solaris

    rust-lang/rust#82216 removed the
    x86_64-sun-solaris target from rustup, changing it to use
    x86_64-pc-solaris instead.

    Related issues:
     * rust-lang/rust#85098

commit 27a6a3c
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Mon May 10 11:56:41 2021 +0200

    Avoid cast pointers to usize in windows::NamedPipe

    Changes the Inner::ptr_from_* methods to use ptr::wrapping_sub rather
    then casting to usize.

commit e316b21
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Wed May 5 12:13:47 2021 +0200

    Replace offset constants with methods in Windows NamedPipe

commit 9e13732
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Mon Apr 12 20:26:53 2021 +0200

    Reorder NamedPipe fields

    Moving the Overlapped fields to the start to make it easier to determine
    the offsets and hopefully incur less breakage once external fields
    change size.

    Note that the Overlapped fields internally uses miow::Overlapped, which
    in turn is a OVERLAPPED struct as found in the winapi crate and has a
    stable layout (as defined by the Windows API).

commit db0d74c
Author: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date:   Mon Apr 12 20:03:24 2021 +0200

    Remove unsound offset_of macro

    And replace it with constants that define the offsets to the fields.
    It's not a pretty solution, but it's one without UB.

commit 1667a70
Author: Rob Ede <robjtede@icloud.com>
Date:   Thu Apr 1 17:01:01 2021 +0100

    remove manual doc versioning
@jhpratt
Copy link
Member Author

jhpratt commented Mar 12, 2022

Realistically I don't think there's anything actionable here, so I'm going to go ahead and close this.

@jhpratt jhpratt closed this as completed Mar 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-target-specs Area: compile-target specifications regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.53.0
Development

Successfully merging a pull request may close this issue.

Bring back x86_64-sun-solaris target to rustup kulikjak/rust
5 participants
@jonas-schievink @jhpratt @Mark-Simulacrum @kulikjak @rustbot