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

Ensure different Windows flavors build their own abseil library #81

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

stanhu
Copy link
Collaborator

@stanhu stanhu commented Jul 27, 2023

Previously if you ran:

bundle exec rake gem:x64-mingw-ucrt
bundle exec rake gem:x64-mingw32

The latter would fail with undefined symbols:

linking shared-object re2.so
/usr/bin/x86_64-w64-mingw32-ld: /tmp/re2/ports/x86_64-w64-mingw32/abseil/20230125.3/lib/libabsl_time_zone.a(time_zone_libc.cc.obj):time_zone_libc.cc:(.text+0x97): undefined reference to `__imp___timezone'
/usr/bin/x86_64-w64-mingw32-ld: /tmp/re2/ports/x86_64-w64-mingw32/abseil/20230125.3/lib/libabsl_time_zone.a(time_zone_libc.cc.obj):time_zone_libc.cc:(.text+0xa8): undefined reference to `__imp___dstbias'
/usr/bin/x86_64-w64-mingw32-ld: /tmp/re2/ports/x86_64-w64-mingw32/abseil/20230125.3/lib/libabsl_time_zone.a(time_zone_libc.cc.obj):time_zone_libc.cc:(.text+0xde): undefined reference to `__imp___tzname'
collect2: error: ld returned 1 exit status
make: *** [Makefile:262: re2.so] Error 1

The timezone, dstbias, and tzname symbols come from the Microsoft Runtime library.

This happened because the abseil C++ library would be installed for x64-mingw-ucrt and x64-mingw32 in the same directory based on the host value (x86_64-w64-mingw32). As a result, if x64-mingw-ucrt were built first, then the abseil libraries will link against the Universal C Runtime (UCRT). When rake gem:x64-mingw32 is attempted after rake gem:x64-mingw-ucrt, then it will fail to link those symbols because the abseil library in ports/x86_64-w64-mingw32 directory expects UCRT to be used.

To avoid this mess, append the RbConfig::CONFIG['arch'] to the mini_portile recipe to ensure x64-mingw-ucrt and x64-mingw32 builds use different library paths.

@stanhu stanhu mentioned this pull request Jul 27, 2023
@stanhu stanhu changed the title Ensure Windows builds use the correctly built abseil library Ensure different Windows flavors build their own abseil library Jul 27, 2023
@stanhu stanhu force-pushed the sh-isolate-windows-ucrt-builds branch 3 times, most recently from 6a7154b to d515c4b Compare July 27, 2023 22:21
Previously if you ran:

```
bundle exec rake gem:x64-mingw-ucrt
bundle exec rake gem:x64-mingw32
```

The latter would fail with undefined symbols:

```
linking shared-object re2.so
/usr/bin/x86_64-w64-mingw32-ld: /tmp/re2/ports/x86_64-w64-mingw32/abseil/20230125.3/lib/libabsl_time_zone.a(time_zone_libc.cc.obj):time_zone_libc.cc:(.text+0x97): undefined reference to `__imp___timezone'
/usr/bin/x86_64-w64-mingw32-ld: /tmp/re2/ports/x86_64-w64-mingw32/abseil/20230125.3/lib/libabsl_time_zone.a(time_zone_libc.cc.obj):time_zone_libc.cc:(.text+0xa8): undefined reference to `__imp___dstbias'
/usr/bin/x86_64-w64-mingw32-ld: /tmp/re2/ports/x86_64-w64-mingw32/abseil/20230125.3/lib/libabsl_time_zone.a(time_zone_libc.cc.obj):time_zone_libc.cc:(.text+0xde): undefined reference to `__imp___tzname'
collect2: error: ld returned 1 exit status
make: *** [Makefile:262: re2.so] Error 1
```

The `timezone`, `dstbias`, and `tzname` symbols come from the
Microsoft Runtime library.

This happened because the abseil C++ library would be installed for
`x64-mingw-ucrt` and `x64-mingw32` in the same directory based on the
`host` value (`x86_64-w64-mingw32`). As a result, if `x64-mingw-ucrt`
were built first, then the abseil libraries will link against the
Universal C Runtime (UCRT). When `rake gem:x64-mingw32` is attempted
after `rake gem:x64-mingw-ucrt`, then it will fail to link those
symbols because the abseil library in `ports/x86_64-w64-mingw32`
directory expects UCRT to be used.

To avoid this mess, append the `RbConfig::CONFIG['arch']` to the
mini_portile recipe to ensure `x64-mingw-ucrt` and `x64-mingw32`
builds use different library paths.
@stanhu stanhu force-pushed the sh-isolate-windows-ucrt-builds branch from d515c4b to 71f3bd5 Compare July 27, 2023 22:31
target_dir = File.join(PACKAGE_ROOT_DIR, "ports")
# Ensure x64-mingw-ucrt and x64-mingw32 use different library paths since the host
# is the same (x86_64-w64-mingw32).
target_dir = File.join(target_dir, target_arch) if windows? && !target_arch.empty?
Copy link
Owner

Choose a reason for hiding this comment

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

Is it worth doing this for all builds rather than special casing it only for Windows? (I’m not sure what a sensible fallback value is if target_arch is nil or empty.)

Copy link
Collaborator Author

@stanhu stanhu Jul 28, 2023

Choose a reason for hiding this comment

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

I originally had that, but I ran into an issue where the vendored libraries got moved from ports/archives to ports/<arch>/archives, so the Rakefile couldn't find the tarballs to include:

re2/Rakefile

Lines 111 to 112 in 1379bd2

abseil_archive = File.join('ports', 'archives', "#{dependencies['abseil']['version']}.tar.gz")
libre2_archive = File.join('ports', 'archives', "re2-#{dependencies['libre2']['version']}.tar.gz")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could do what Nokogiri does and have an --enable-cross-build flag to indicate whether cross-compiling is happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a much better solution: cbe6dc6. We already use --enable-cross-build in the Rakefile even though we didn't actually use it extconf.rb. 😄

Using this flag will enable us to keep the vendored libraries in
`ports/archives` rather than `ports/<arch>/archives`.
@stanhu stanhu requested a review from mudge July 28, 2023 16:38
@stanhu
Copy link
Collaborator Author

stanhu commented Aug 1, 2023

@mudge Anything else I can do to get this merged? Would love to see 2.0 out the door: #78 (comment)

@mudge mudge merged commit 0619315 into mudge:v2.0.x Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants