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

Lowercase lib name in Windows link attribute #355

Merged
merged 1 commit into from
Oct 21, 2021
Merged

Conversation

Skirmisher
Copy link
Contributor

Cross-building for Windows on (for example) Linux exposes some fun case-sensitivity issues, since mingw libraries are in all lowercase. This just changes title-case library names to lowercase, and shouldn't affect native Windows builds at all (I hope).

@jhpratt
Copy link
Member

jhpratt commented Oct 3, 2021

Is there a setup that could be used in CI to ensure this doesn't regress?

CI isn't running because you're a first-time contributor here, but I should have a button that runs it manually. For whatever reason that isn't the case, which is why CI appears as though it had stalled.

@jhpratt jhpratt self-assigned this Oct 3, 2021
@jhpratt jhpratt added the A-local-offset Area: local offset label Oct 3, 2021
@Skirmisher
Copy link
Contributor Author

Skirmisher commented Oct 3, 2021

I have approximately zero experience with Github Actions... but, it should be pretty straightforward to install the mingw-w64 toolchain on Ubuntu and run something like cargo build --tests --target x86_64-pc-windows-gnu (might need some RUSTFLAGS to pass the mingw library path, not sure since I don't run Ubuntu). (Though I haven't been able to build the tests myself for some reason...)

@jhpratt
Copy link
Member

jhpratt commented Oct 5, 2021

For status I'm just waiting on a response from GitHub on why I can't run CI here. I can add a regression check in CI myself.

@jhpratt jhpratt added the C-keep-open Category: should not be closed due to inactivity label Oct 5, 2021
@jhpratt
Copy link
Member

jhpratt commented Oct 11, 2021

Still waiting on a response from GitHub. No idea why it's taking so long; they've responded to me within a day in the past and it's been eight days at this point.

@jhpratt
Copy link
Member

jhpratt commented Oct 18, 2021

I'm on Linux, so I'm able to test this manually. Would you mind providing steps that fail on main but succeed with this patch?

For compatibility with cross-compiling for Windows in case-sensitive
environments.
@Skirmisher
Copy link
Contributor Author

Skirmisher commented Oct 18, 2021

The steps are roughly "on Linux, cross-compile for Windows anything that depends on time with the feature local-offset and links an executable". The tests are of course the most convenient thing that accomplishes that, but I was thrown for a loop for a while because (as I mentioned before) I couldn't get them to build, as they would swamp me with thousands of errors about unresolved names. I realized after peeking at your .github/workflows that you pass --all-features when testing. Could you perhaps document that in the README or such?

Prerequisites

  • Install the MinGW libraries + toolchain
    • On Ubuntu, apt install gcc-mingw-w64, or gcc-mingw-w64-x86-64 to only install the 64-bit toolchain
    • On Fedora, dnf install mingw{32,64}-gcc
  • Install a Windows target for your current Rust toolchain: rustup target add x86_64-pc-windows-gnu

Steps to reproduce

cargo build --tests --all-features --target x86_64-pc-windows-gnu

Output on main

error: linking with `x86_64-w64-mingw32-gcc` failed: exit status: 1
  |
  = note: "x86_64-w64-mingw32-gcc" <very long list of arguments>
  = note: /usr/lib/gcc/x86_64-w64-mingw32/10.3.1/../../../../x86_64-w64-mingw32/bin/ld: cannot find -lKernel32
          collect2: error: ld returned 1 exit status

Output after patch

Finished dev [unoptimized] target(s) in 3.35s

@jhpratt
Copy link
Member

jhpratt commented Oct 19, 2021

After doing that and applying this patch, I still get an error.

   Compiling time v0.3.3 (/home/jhpratt/code/time)
error: linking with `x86_64-w64-mingw32-gcc` failed: exit status: 1
  |
  = note: "x86_64-w64-mingw32-gcc" <omitted>
  = note: /usr/lib/gcc/x86_64-w64-mingw32/10.3.1/../../../../x86_64-w64-mingw32/bin/ld: cannot find -l:libpthread.a
          collect2: error: ld returned 1 exit status

@Skirmisher
Copy link
Contributor Author

Skirmisher commented Oct 19, 2021

That's strange—pthreads should be pulled in when you install the MinGW toolchain (on Fedora the package is mingw64-winpthreads, on Ubuntu it's part of mingw-w64-x86-64-dev, both of which are depended on by the respective mingw-gcc packages). You may need to set RUSTFLAGS=-L/path/to/mingw/lib (on Ubuntu that's /usr/x86_64-w64-mingw32/lib, on Fedora it's /usr/x86_64-w64-mingw32/sys-root/mingw/lib).

I remember running into this before, and finding it strange that the argument is -l:libpthread.a instead of something like -lpthread, and I couldn't find any references to the full filename in the Rust dependencies. Turns out it's passed by rustc itself... (I also remember having to make a symlink or something, but that doesn't seem to be the case...?)

@jhpratt
Copy link
Member

jhpratt commented Oct 19, 2021

I'm on Fedora for what it's worth. I can't get it to work locally even with RUSTFLAGS set, but I'll give setting up CI a shot to see if I can get it working there.

@Skirmisher
Copy link
Contributor Author

Oops, I missed a package on Fedora—the static winpthreads library lives in mingw64-winpthreads-static. Sorry about that! For Ubuntu (i.e. for CI purposes), the static lib does actually exist in mingw-w64-x86-64-dev. (Also I just realized their packages spell it x86-64 with a dash and not an underscore, woops...)

@jhpratt
Copy link
Member

jhpratt commented Oct 21, 2021

I'll get CI set up at some point, but I've just confirmed that this does fix a link error that would otherwise occur. Merging as such.

@jhpratt jhpratt merged commit a28507f into time-rs:main Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-local-offset Area: local offset C-keep-open Category: should not be closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants