-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
base: master
Are you sure you want to change the base?
Add cygwin target. #134999
Conversation
Failed to set assignee to
|
These commits modify compiler targets. Some changes occurred in src/doc/rustc/src/platform-support cc @Noratrieb |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
r? compiler |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This should be |
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. |
I will answer these two questions here. Ookiineko chose
Therefore, I would suggest that just treat Cygwin as an Unix OS. |
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
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 I add the P.S. I test the cygwin target on MSYS2, actually. I just symlinked the linker and |
@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 For instance, say if I have something that is conditioned on |
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
Yes, treating Cygwin as Unix has some drawbacks:
Therefore I would say that treating @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.
I am going to change it. |
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.
That's completely fair. I'd like other compiler team members to vibe-check how this change is made. |
hmm. I mean, I can run Windows programs on Linux, but
|
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.
Ah, I was not aware of the thread about MSYS2 planning to become Cygwin compatible which, upon studying, makes it clear that
- people do use the
x86_64-pc-cygwin
tuple right now - 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"?
// FIXME(davidtwco): Support Split DWARF on Cygwin - may require LLVM changes to | ||
// output DWO, despite using DWARF, doesn't use ELF.. | ||
debuginfo_kind: DebuginfoKind::Pdb, |
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.
Hrm. This looks like it's using PDB, not Dwarf? I don't understand this comment. I know it's a holdover from the other Windows GNU targets, but...
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 original PR: #98051
Actually the debug info of MinGW and Cygwin is Dwarf, but it's always merged into the executable and could not be splitted. I'm not sure that the msvc debugger could recognize dwarf, but lldb can handle both dwarf and pdb. I guess that here it just tries to keep the original behavior on Windows, and let llvm to choose the proper debug info kind. In my test, the mingw and cygwin targets still use dwarf even they're set to pdb.
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.
Modern Windbg can handle DWARF.
This line is the same for *-windows-gnu*
targets. I never understood it, but hey, as long as it works…
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.
It was fixed recently: #135790
Please incorporate it here.
|
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. |
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. |
☔ The latest upstream changes (presumably #136087) made this pull request unmergeable. Please resolve the merge conflicts. |
619a2a9
to
eb4efcd
Compare
The MCP rust-lang/compiler-team#826 has been approved. I rebased the PR and require review. |
you should trigger edit: |
@rustbot ready |
@rustbot label -S-waiting-on-MCP |
// FIXME(davidtwco): Support Split DWARF on Cygwin - may require LLVM changes to | ||
// output DWO, despite using DWARF, doesn't use ELF.. | ||
debuginfo_kind: DebuginfoKind::Pdb, |
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.
It was fixed recently: #135790
Please incorporate it here.
Also, please note #134999 (comment) that is collapsed right now. |
Co-authored-by: Ookiineko <[email protected]> Co-authored-by: nora <[email protected]> Co-authored-by: Jubilee <[email protected]>
eb4efcd
to
402a52e
Compare
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.