Skip to content

Conversation

@jjerphan
Copy link
Collaborator

@jjerphan jjerphan commented Mar 19, 2025

Reference Issues/PRs

Fix #2604.

What does this implement or fix?

Add setup for building conda packages for Windows before they are published on conda-forge.

Any other comments?

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@jjerphan jjerphan added the patch Small change, should increase patch version label Mar 19, 2025
@jjerphan jjerphan force-pushed the build/windows-conda-build branch 7 times, most recently from 9e226d0 to 0520427 Compare March 20, 2025 10:09
@jjerphan jjerphan changed the title ci: Add workflow for Windows conda-build: Add workflow for Windows Mar 21, 2025
@jjerphan jjerphan force-pushed the build/windows-conda-build branch 13 times, most recently from 34410d5 to 9323534 Compare March 24, 2025 15:59
@jjerphan jjerphan force-pushed the build/windows-conda-build branch 7 times, most recently from f9ddf0d to 68c4487 Compare March 25, 2025 17:23
- libprotobuf <7
# We have to use the build of libprotobuf starting from this PR:
# https://github.com/conda-forge/libprotobuf-feedstock/pull/277
- sel(win): libprotobuf==5.29.3=h*_3
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a known installable and patched version of libprotobuf 5.

libprotobuf 6 is not as installable for Windows in my experience, and also would need to be patched (see conda-forge/libprotobuf-feedstock#275 (comment)).

@jjerphan jjerphan added the minor Feature change, should increase minor version label Dec 5, 2025
@jjerphan jjerphan force-pushed the build/windows-conda-build branch 2 times, most recently from 8fc8184 to dee474d Compare December 5, 2025 10:36
@jjerphan jjerphan force-pushed the build/windows-conda-build branch from dee474d to 10b2105 Compare December 5, 2025 11:04
Comment on lines +209 to +322
set CMAKE_GENERATOR_PLATFORM=
set CMAKE_GENERATOR_TOOLSET=
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it odd that this is needed only for the windows conda build. The other builds don't set this and still use ninja. How is conda different? What's the error that's raised if these are not added?

Comment on lines +303 to +301
# DONT use recursive submodules checkout to simulate conda feedstock build
# with:
# submodules: recursive
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Why not just remove these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mainly to keep the style of this workflow file (this comment exists for Linux and Mac and is informative I think).

This file needs to be reformated and improved, and this better be done in another PR IMO.

Comment on lines +92 to +131
if(${ARCTICDB_USING_CONDA})
add_compile_definitions(
# Required to link against zstd.dll
# See: https://github.com/facebook/zstd/blob/dev/lib/zstd.h#L47-L53
ZSTD_DLL_IMPORT=1

# Required to link against lz4.dll
# See: https://github.com/lz4/lz4/blob/dev/lib/lz4frame.h#L79-L85
LZ4_DLL_IMPORT=1

# Required to link against protobuf.dll
# See: https://github.com/protocolbuffers/protobuf/blob/384fabf35a9b5d1f1502edaf9a139b1f51551a01/cmake/README.md?plain=1#L296-L319
PROTOBUF_USE_DLLS=1

# Required to link against fmt.dll
FMT_SHARED=1

# Required to link against spdlog.dll
# See: https://github.com/gabime/spdlog/blob/faa0a7a9c5a3550ed5461fab7d8e31c37fd1a2ef/include/spdlog/common.h#L28-L48
SPDLOG_COMPILED_LIB=1
SPDLOG_SHARED_LIB=1
SPDLOG_HEADER_ONLY=0

# Indicate that we are building using fmt's external DLL instead of spdlog's internal fmt
SPDLOG_FMT_EXTERNAL=1

# Required to link against azure-core.dll
# See: https://github.com/Azure/azure-sdk-for-cpp/blob/c914c0933fa4835c6618a42bb276d6077423b170/sdk/storage/azure-storage-common/inc/azure/storage/common/dll_import_export.hpp#L19
AZ_CORE_DLL=1

# Required to link against azure-identity.dll
# See: https://github.com/Azure/azure-sdk-for-cpp/blob/bf36db827f5ea1ef2e38953875c6c335b4242b2e/sdk/identity/azure-identity/inc/azure/identity/dll_import_export.hpp
AZ_IDENTITY_DLL=1

# Required to link against azure-storage-common.dll
# See: https://github.com/Azure/azure-sdk-for-cpp/blob/c914c0933fa4835c6618a42bb276d6077423b170/sdk/storage/azure-storage-common/inc/azure/storage/common/dll_import_export.hpp#L19
AZ_STORAGE_COMMON_DLL=1

# Required to link against azure-storage-blobs.dll
# See: https://github.com/Azure/azure-sdk-for-cpp/blob/bf36db827f5ea1ef2e38953875c6c335b4242b2e/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs/dll_import_export.hpp
AZ_STORAGE_BLOBS_DLL=1
)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree this doesn't look right. We can merge it like this, though I'd appreciate if you can investigate it at some point.

Copy link
Collaborator Author

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Replying to Vasil's review.

@jjerphan jjerphan force-pushed the build/windows-conda-build branch from 38bbf3f to 975dc61 Compare December 5, 2025 17:35
@jjerphan
Copy link
Collaborator Author

jjerphan commented Dec 5, 2025

@vasil-pashov: one of the thread disappeared with e89bc80 which gives the errors in the logs.

Here is the error for posterity:

[vcvarsall.bat] Environment initialized for: 'x64'
CMake Error at CMakeLists.txt:13 (project):
  Generator

    Ninja

  does not support platform specification, but platform

    x64

  was specified.


-- Configuring incomplete, errors occurred!
CMake Error:
  Generator

    Ninja

  does not support platform specification, but platform

    x64

  was specified.

Full logs

This thread was a hint to unset CMAKE_GENERATOR_{PLATFORM,TOOLSET}.

@jjerphan jjerphan force-pushed the build/windows-conda-build branch from 7a91a93 to 88fac60 Compare December 8, 2025 08:19
Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan jjerphan force-pushed the build/windows-conda-build branch from 0917fb4 to aa3f369 Compare December 8, 2025 08:52
Signed-off-by: Julien Jerphanion <[email protected]>
"[...] By contrast, PRE_TEST delays test discovery until just prior to
test execution. This way test discovery occurs in the target environment
where the test has a better chance at finding appropriate runtime
dependencies."

See: https://cmake.org/cmake/help/latest/module/GoogleTest.html

Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan jjerphan force-pushed the build/windows-conda-build branch from 522d8e0 to 758ba2a Compare December 8, 2025 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request minor Feature change, should increase minor version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants