Skip to content

Windows shared builds #25

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

Merged
xhochy merged 9 commits into from
Feb 28, 2022
Merged

Windows shared builds #25

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ca05db8
BUILD_SHARED_LIBS also on windows
h-vetinari Feb 21, 2022
5fd94d5
check for failures in bld.bat
h-vetinari Feb 21, 2022
e6f11f9
bump build number
h-vetinari Feb 21, 2022
0ace809
MNT: Re-rendered with conda-build 3.21.8, conda-smithy 3.17.1, and co…
h-vetinari Feb 21, 2022
cb48ebf
update test section
h-vetinari Feb 21, 2022
eed7a97
use echo
h-vetinari Feb 21, 2022
23f8279
absl_libs on windows are still static
h-vetinari Feb 21, 2022
87b4dc6
remove internal libraries
h-vetinari Feb 21, 2022
dc32fd6
fixes
h-vetinari Feb 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .azure-pipelines/azure-pipelines-win.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion .ci_support/README
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .ci_support/linux_64_.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
c_compiler:
- gcc
c_compiler_version:
- '9'
- '10'
cdt_name:
- cos6
channel_sources:
Expand All @@ -11,7 +11,7 @@ channel_targets:
cxx_compiler:
- gxx
cxx_compiler_version:
- '9'
- '10'
docker_image:
- quay.io/condaforge/linux-anvil-cos7-x86_64
target_platform:
Expand Down
4 changes: 2 additions & 2 deletions .ci_support/linux_aarch64_.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ BUILD:
c_compiler:
- gcc
c_compiler_version:
- '9'
- '10'
cdt_arch:
- aarch64
cdt_name:
Expand All @@ -15,7 +15,7 @@ channel_targets:
cxx_compiler:
- gxx
cxx_compiler_version:
- '9'
- '10'
docker_image:
- quay.io/condaforge/linux-anvil-cos7-x86_64
target_platform:
Expand Down
4 changes: 2 additions & 2 deletions .ci_support/linux_ppc64le_.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
c_compiler:
- gcc
c_compiler_version:
- '9'
- '10'
cdt_name:
- cos7
channel_sources:
Expand All @@ -11,7 +11,7 @@ channel_targets:
cxx_compiler:
- gxx
cxx_compiler_version:
- '9'
- '10'
docker_image:
- quay.io/condaforge/linux-anvil-cos7-x86_64
target_platform:
Expand Down
4 changes: 2 additions & 2 deletions .ci_support/osx_64_.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ MACOSX_DEPLOYMENT_TARGET:
c_compiler:
- clang
c_compiler_version:
- '11'
- '12'
channel_sources:
- conda-forge
channel_targets:
- conda-forge main
cxx_compiler:
- clangxx
cxx_compiler_version:
- '11'
- '12'
macos_machine:
- x86_64-apple-darwin13.4.0
target_platform:
Expand Down
4 changes: 2 additions & 2 deletions .ci_support/osx_arm64_.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ MACOSX_DEPLOYMENT_TARGET:
c_compiler:
- clang
c_compiler_version:
- '11'
- '12'
channel_sources:
- conda-forge
channel_targets:
- conda-forge main
cxx_compiler:
- clangxx
cxx_compiler_version:
- '11'
- '12'
macos_machine:
- arm64-apple-darwin20.0.0
target_platform:
Expand Down
1 change: 1 addition & 0 deletions .gitattributes
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions .scripts/build_steps.sh
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .scripts/run_docker_build.sh
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 29 additions & 13 deletions .scripts/run_osx_build.sh
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 4 additions & 7 deletions build-locally.py
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 10 additions & 4 deletions recipe/bld.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
@echo on

mkdir build
cd build
cmake -GNinja ^
-DCMAKE_INSTALL_PREFIX=%LIBRARY_PREFIX% ^
-DCMAKE_PREFIX_PATH=%LIBRARY_PREFIX% ^
-DCMAKE_BUILD_TYPE=Release ^
..
-DCMAKE_INSTALL_PREFIX=%LIBRARY_PREFIX% ^
-DCMAKE_PREFIX_PATH=%LIBRARY_PREFIX% ^
-DCMAKE_BUILD_TYPE=Release ^
-DBUILD_SHARED_LIBS=ON ^
..
if %ERRORLEVEL% neq 0 exit 1

cmake --build .
if %ERRORLEVEL% neq 0 exit 1

cmake --build . --target install
if %ERRORLEVEL% neq 0 exit 1
28 changes: 25 additions & 3 deletions recipe/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ source:
- remove-4221.patch # [win]

build:
number: 0
number: 1
run_exports:
- {{ pin_subpackage("abseil-cpp", max_pin="x.x") }}

Expand All @@ -26,10 +26,32 @@ requirements:
- cmake
- ninja

{% set absl_libs = [
Copy link
Member

@xhochy xhochy Feb 21, 2022

Choose a reason for hiding this comment

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

The bot will probably choke on this, I've seen several feedstock stop autoupdate because they had such a pattern. Could you put this into a test script?

Copy link
Member Author

@h-vetinari h-vetinari Feb 21, 2022

Choose a reason for hiding this comment

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

Hm, I'd almost be tempted to rather fix the bot. I like this idiom for testing, keeps things nicely visible in meta.yaml.

Copy link
Member Author

@h-vetinari h-vetinari Feb 22, 2022

Choose a reason for hiding this comment

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

The bot will probably choke on this, [...]

So it's mostly about the version updates? I've looked here (and other places in that repo), and it sounds like it'd just need some extra handling for {% ... %} cases.

"base", "civil_time", "cord", "cordz_functions", "cordz_handle", "cordz_info",
"cordz_sample_token", "examine_stack", "exponential_biased", "failure_signal_handler",
"flags", "flags_commandlineflag", "flags_config", "flags_marshalling", "flags_parse",
"flags_private_handle_accessor", "flags_program_name", "flags_reflection", "flags_usage",
"hash", "hashtablez_sampler", "int128", "log_severity", "low_level_hash", "periodic_sampler",
"random_distributions", "random_seed_gen_exception", "random_seed_sequences", "raw_hash_set",
"scoped_set_env", "spinlock_wait", "stacktrace", "status", "statusor", "strerror", "strings",
"symbolize", "synchronization", "time", "time_zone"
] %}
test:
commands:
- test -f $PREFIX/lib/libabsl_base${SHLIB_EXT} # [not win]
- if not exist %LIBRARY_LIB%\\absl_base.lib exit 1 # [win]
# windows
- if not exist %LIBRARY_LIB%\\abseil_dll.lib exit 1 # [win]
- if not exist %LIBRARY_BIN%\\abseil_dll.dll exit 1 # [win]

# absl_libs
{% for each_lib in absl_libs %}
# presence of shared libs
- test -f $PREFIX/lib/libabsl_{{ each_lib }}${SHLIB_EXT} # [unix]
- if not exist %LIBRARY_LIB%\\absl_{{ each_lib }}.lib exit 1 # [win]
# absence of static libs
- test ! -f $PREFIX/lib/libabsl_{{ each_lib }}.a # [unix]
# TODO: do we want to get rid of the static windows libs?
# - if exist %LIBRARY_LIB%\\absl_{{ each_lib }}.lib exit 1 # [win]
Copy link
Member

@xhochy xhochy Feb 21, 2022

Choose a reason for hiding this comment

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

On Windows, you will always have .lib files. In the case of shared DLLs, these are small import libraries needed that only contain "glue" to lazy load the function definitions from the DLLs at runtime.

Copy link
Member Author

@h-vetinari h-vetinari Feb 21, 2022

Choose a reason for hiding this comment

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

I had to learn already that .lib is both for static libs as well as the "import libraries" that wrap DLLs, but the logs here look pretty unambiguous to me:

[142/154] Linking CXX static library absl\flags\absl_flags_parse.lib

Copy link
Member Author

@h-vetinari h-vetinari Feb 21, 2022

Choose a reason for hiding this comment

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

It seems there's only one DLL (plus its import lib) being created on windows with BUILD_SHARED_LIBS

Copy link
Contributor

@bluescarni bluescarni Feb 22, 2022

Choose a reason for hiding this comment

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

It's been some time since I last looked at the abseil build system, but my recollection was that on Windows only static builds were supported. Has this changed in the last year or so?

Copy link
Member Author

@h-vetinari h-vetinari Feb 22, 2022

Choose a reason for hiding this comment

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

Seems so. abseil_dll.dll is pretty unambiguous as well IMO. ;-)

Copy link
Contributor

@bluescarni bluescarni Feb 22, 2022

Choose a reason for hiding this comment

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

I am probably being paranoid, and I understand that this would be additional work for you, but would it be possible to have a small tests that checks (on Windows) that the DLL actually works (i.e., compile and run a small executable which calls functions that have been compiled in the DLL)?

I am asking because I am a bit scared by some of the statements in the abseil docs regarding dynamics libraries:

https://abseil.io/about/compatibility

But in any case, thanks a lot for working on this! Dynamic libraries are definitely the way to go for a distribution like conda, it would be great to have this PR merged.

Copy link
Member Author

@h-vetinari h-vetinari Feb 22, 2022

Choose a reason for hiding this comment

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

Yeah, AFAICT those warnings are for systems that cannot track ABI as comprehensively as conda(-forge) can.

I have an open PR that directly inspired this need. Not sure if a 5-line #include "absl/xxx" / int main() {} would help much to determine the proper operations, to be honest. What I could think of is publishing this PR to a separate channel (say absl_dev) first, and then I can test it in the sentencepiece PR (though, admittedly, a negative outcome can also just mean that the sentencepiece side isn't working yet).

How does that sound?

Copy link
Contributor

@bluescarni bluescarni Feb 22, 2022

Choose a reason for hiding this comment

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

I would be personally more in favour of pulling the trigger and merge this PR directly, and perhaps roll it back later in case it turns out it breaks existing packages.

But certainly @xhochy has a better feeling than I do about whether this course of action is appropriate for conda-forge as a whole, so I'll leave the decision to him.

{% endfor %}

about:
home: https://github.com/abseil/abseil-cpp
Expand Down