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

Add cygwin target. #134999

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Berrysoft
Copy link
Contributor

r? compiler-team

This PR simply adds cygwin target together with msys2 target, based on @ookiineko 's (the account has been deleted) work on cygwin target. My full work is here: master...Berrysoft:rust:dev/cygwin

I have succeeded in building a new rustc for cygwin target, and eventually distributed a new version of fish-shell (rewritten by Rust) for MSYS2.

I will open a new PR to fix std if this PR is accepted.

@rustbot
Copy link
Collaborator

rustbot commented Jan 1, 2025

Failed to set assignee to compiler-team: cannot assign: response: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/assignees#add-assignees-to-an-issue","status":"404"}

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 1, 2025

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

r? compiler

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

This should be x86_64-pc-windows-cygwin, if anything.

@mati865
Copy link
Contributor

mati865 commented Jan 1, 2025

I'm not sure if two separate triples are necessary given msys2/MSYS2-packages#3012, but I think there is no solution for having to distinguish between Cygwin and MSYS2 DLL yet.

@Berrysoft
Copy link
Contributor Author

@workingjubilee

This should be x86_64-pc-windows-cygwin, if anything.

@jieyouxu

Question: why does this need to be empty, that seems wrong?

I will answer these two questions here. Ookiineko chose x86_64-pc-cygwin instead of x86_64-pc-windows-cygwin, and I agree with this choice. The target_os is cygwin but not windows, and msys2 is only an variant (or, target_env, so the original cygwin target has an empty target_env). Here are some advantages of this choice:

  • Cygwin is a POSIX-compatible layer on Windows. From an engineering view, it would be more convenient to assume it as "unix" but not "windows". A typical cygwin program uses file descriptor instead of file handle, uses malloc instead of HeapAlloc, and uses fork. Cygwin is not a different native ABI just like MinGW and MSVC, but much like a subsystem.
  • Make the maintainer (me) easier. Most code in the std assumes that "windows" is not "unix", and "unix" has it's common ways to do many affairs. For example, RawFd is defined for unix, and RawHandle is defined for windows. If cygwin is defined as an independent unix os, I don't have to change and maintain a lot cfgs to make it work.
  • To make others' porting easier. If "cygwin" is added as a new target_os in unix family, most crates only need small changes or even no changes to port to this new target. However, if "cygwin" is added as a new ABI on "windows", the whole environment needs a relative massive change to port to Cygwin. All code with cfg(unix) might need to be changed to cfg(any(unix, target_env = "cygwin")) or even cfg(any(unix, all(windows, target_env = "cygwin"))), and all code with cfg(windows) need to be changed to cfg(all(windows, not(target_env = "cygwin"))). That's too ugly.

Therefore, I would suggest that just treat Cygwin as an Unix OS.

@Berrysoft
Copy link
Contributor Author

I'm not sure if two separate triples are necessary given msys2/MSYS2-packages#3012, but I think there is no solution for having to distinguish between Cygwin and MSYS2 DLL yet.

Well... To be honest MSYS2 is just a fork of Cygwin, with some user-friendly changes. From the compiler's view, the differences are only

  • Linker name
  • Runtime lib name

However, the cygwin dll and msys2 dll should not be mixed. They all maintain their own services and layers. It might cause some misc errors when a cygwin exe linked to a msys2 dll trying to call fork.

I add the -msys2 target for convenience. If it's not added, the users of MSYS2 might need to tweak their environment a little bit - symlinking the linkers and libs, maybe.

P.S. I test the cygwin target on MSYS2, actually. I just symlinked the linker and libcygwin.a.

@jieyouxu
Copy link
Member

jieyouxu commented Jan 2, 2025

@Berrysoft could you file an MCP for adding these two targets? Usually we wouldn't ask for MCPs for adding Tier 3 targets, but in this case I think it's warranted to allow compiler team members to discuss how to sort out the target_os vs target_env situation for cygwin and msys2 targets (in addition to what to use for the target tuple in lieu of other windows-ish targets). Because it feels to me that with this "treat it as Unix OS" scheme, we might have some issues with "Unix-like but not Unix enough" and "Windows-like but not Windows enough" simultaneously.

For instance, say if I have something that is conditioned on cfg(target_os = "windows") or cfg(not(target_os = "windows")), will the cygwin or msys2 target qualify or not?

@Berrysoft
Copy link
Contributor Author

Berrysoft commented Jan 2, 2025

we might have some issues with "Unix-like but not Unix enough" and "Windows-like but not Windows enough" simultaneously.

Well, I would say that Cygwin is Unix enough in most times, and also Windows enough all the time (because it is really running on Windows). However, from the development view, I don't think target_os = "windows" should include Cygwin target. cfg(target_os = "windows") usually means

  • There are file handles and socket handles, not file descriptors. However, Cygwin uses file descriptors just like other Unix.
  • There is CreateProcess instead of fork + exec or posix_spawn. However, Cygwin provides all POSIX methods.
  • The filesystem is Windows-like (backslash, drive letters, UNC paths, etc.). However, Cygwin uses Unix-like paths. It even provides /dev and /proc.

Yes, treating Cygwin as Unix has some drawbacks:

  • fork is very slow on Cygwin. The unix shells work on Cygwin, but usually slower than WSL1 & WSL2.
  • IO is not that fast, compared to native Win32 methods. Cygwin doesn't provide kqueue or epoll, while Windows provides IOCP for high-performance IO. I would say that even basic filesystem IO is not that fast either, but it's fine. Everyone using cygwin or MSYS2 knows that.
  • I'm not even sure if X11 still runs well in Cygwin. Some cygwin executables (e.g., mintty) calls Win32 APIs directly to create GUI. If we treat cygwin as unix, the windows-rs crate might need to think how to port for cygwin target... but anyway they need to write some code for a new windows target, isn't it?

Therefore I would say that treating cygwin as unix is better. Cygwin runs on Windows, but it's better for us to not know it's Windows. If we add a new windows-cygwin target, and try to use native Win32 API instead of the POSIX layer provided by Cygwin, then the thing is not that meaningful. I would like "unix code" running on Windows. If a unix program, e.g., fish-shell, could not be compiled to Windows with this target, it would be meaningless.

@jieyouxu Do you still think that I need to file an MCP? It's not hard for me to just opening an issue, but I'm afraid of a large debate, only focusing on whether cygwin is Unix or Windows.

Philosophers have only interpreted the world, in various ways; the point, however, is to change it.

I am going to change it.

@jieyouxu
Copy link
Member

jieyouxu commented Jan 2, 2025

Do you still think that I need to file an MCP? It's not hard for me to just opening an issue, but I'm afraid of a large debate, only focusing on whether cygwin is Unix or Windows.

That's exactly why I am requesting an MCP when usually Tier 3 targets don't need to, because this aspect is controversial. I intend to give the MCP some time, but if we fail to reach consensus, I'll re-nominate the MCP for compiler triage meeting to decide how to proceed.

I am going to change it.

That's completely fair. I'd like other compiler team members to vibe-check how this change is made.

@workingjubilee
Copy link
Member

hmm. I mean, I can run Windows programs on Linux, but

  • that doesn't make my Linux computer have target_os = "windows"
  • they don't really need modifications (usually), they either work or don't

@jieyouxu jieyouxu added the needs-mcp This change is large enough that it needs a major change proposal before starting work. label Jan 2, 2025
@Berrysoft
Copy link
Contributor Author

MCP opened: rust-lang/compiler-team#826

Zulip: https://rust-lang.zulipchat.com/#narrow/stream/233931-xxx/topic/Add.20new.20targets.20for.20Cygwin.20.28and.20MSYS2.29.20compiler-team.23826

@Noratrieb Noratrieb added S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2025
@jieyouxu jieyouxu removed the needs-mcp This change is large enough that it needs a major change proposal before starting work. label Jan 2, 2025
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Ah, I was not aware of the thread about MSYS2 planning to become Cygwin compatible which, upon studying, makes it clear that

  1. people do use the x86_64-pc-cygwin tuple right now
  2. Cygwin and MSYS2 are converging and may be indistinct soon

In the light of that then I retract my commentary, aside from that perhaps we do not need both a Cygwin and MSYS2 target, as hopefully MSYS2 packages will become fully Cygwin compatible. That would also solve current questions like

  • Q. What are the differences between the two Cygwin targets?
  • A. What do you mean, "two"?

src/doc/rustc/src/platform-support/x86_64-pc-cygwin.md Outdated Show resolved Hide resolved
compiler/rustc_target/src/spec/base/cygwin.rs Show resolved Hide resolved
@mati865
Copy link
Contributor

mati865 commented Jan 3, 2025

Ah, I was not aware of the thread about MSYS2 planning to become Cygwin compatible which, upon studying, makes it clear that

  1. people do use the x86_64-pc-cygwin tuple right now
  2. Cygwin and MSYS2 are converging and may be indistinct soon

In the light of that then I retract my commentary, aside from that perhaps we do not need both a Cygwin and MSYS2 target, as hopefully MSYS2 packages will become fully Cygwin compatible. That would also solve current questions like

  • Q. What are the differences between the two Cygwin targets?
  • A. What do you mean, "two"?
  1. Nit: I think compilers still use msys triple by the default. This is still under the todo section.
  2. By no means would I call them indistinct. Automatic path conversion for native Win32 applications is a significant difference. There are other smaller differences as well.
    This is only about simplifying build systems and avoiding monstrous patches like this one https://github.com/msys2/MSYS2-packages/pull/3004/files#diff-a9bbc8aac9ae2b67b25a6615db50970be1b8586ee4f866fadabf5786b0327146

@workingjubilee
Copy link
Member

By no means would I call them indistinct. Automatic path conversion for native Win32 applications is a significant difference. There are other smaller differences as well.

Hm, that is pretty big? I think?

Still, if these are two things, then ideally we add the tuple that the maintainer of the target, that also wants to distribute software using the tuple, will actually be using for such builds. That way the target benefits most from their expertise and we know it is exactly accurate for someone's use-case. Then it's a smaller pull to add the other.

@Berrysoft
Copy link
Contributor Author

I don't think it's such big for a developer. I suggest adding one target for now, and if it really matters in the future, we can add for msys2 variant.

@bors
Copy link
Contributor

bors commented Jan 26, 2025

☔ The latest upstream changes (presumably #136087) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 26, 2025
Co-authored-by: Ookiineko <[email protected]>
Co-authored-by: nora <[email protected]>
Co-authored-by: Jubilee <[email protected]>
@Berrysoft Berrysoft force-pushed the dev/new-cygwin-target branch from 619a2a9 to eb4efcd Compare January 27, 2025 13:11
@Berrysoft Berrysoft changed the title Add cygwin and msys2 targets. Add cygwin target. Jan 27, 2025
@Berrysoft
Copy link
Contributor Author

The MCP rust-lang/compiler-team#826 has been approved. I rebased the PR and require review.

@ognevny
Copy link
Contributor

ognevny commented Jan 27, 2025

require review.

you should trigger @rustbot to change labels...

edit: @rustbot ready (but without '`')

@Berrysoft
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2025
@Berrysoft
Copy link
Contributor Author

@rustbot label -S-waiting-on-MCP

@rustbot rustbot removed the S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. label Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.