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

Fix static blosc2 build #4093

Merged
merged 4 commits into from
Mar 25, 2024

Conversation

vicentebolea
Copy link
Collaborator

@vicentebolea vicentebolea commented Mar 12, 2024

Here, we give up supporting older c-blosc2, I set the dependency of blosc2 to 2.10.1 so that it always includes the blosc2config.cmake file (that you made). This simplify things a lot.

@vicentebolea
Copy link
Collaborator Author

@ax3l For some strange reason #4092 broke.

@vicentebolea
Copy link
Collaborator Author

@ax3l I give up supporting older c-blosc2, I set the dependecy to 2.10.1 that includes the blosc2config.cmake file (that you made). This simplify things a lot (as you suggested months ago).

@vicentebolea vicentebolea force-pushed the fix-static-blosc2-build branch 3 times, most recently from 6905e07 to 3a12b46 Compare March 13, 2024 16:54
@vicentebolea vicentebolea changed the base branch from release_210 to master March 13, 2024 16:54
@vicentebolea vicentebolea requested review from eisenhauer and removed request for caitlinross March 13, 2024 16:56
eisenhauer
eisenhauer previously approved these changes Mar 13, 2024
Copy link
Member

@eisenhauer eisenhauer left a comment

Choose a reason for hiding this comment

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

Looks reasonable...

@vicentebolea vicentebolea force-pushed the fix-static-blosc2-build branch 18 times, most recently from 7804372 to f996b35 Compare March 14, 2024 23:34
@vicentebolea
Copy link
Collaborator Author

@eisenhauer is it possible that the test errors are related to enabling blosc2 in the windows build?

@vicentebolea vicentebolea force-pushed the fix-static-blosc2-build branch from 2db4b4f to be4a8c3 Compare March 15, 2024 17:01
@vicentebolea vicentebolea force-pushed the fix-static-blosc2-build branch 7 times, most recently from 49f7d86 to 9575324 Compare March 18, 2024 17:57
@vicentebolea
Copy link
Collaborator Author

I think that this is ready to merge @eisenhauer. This should allow @ax3l to build with adios2 static in windows and we added blosc2 in the windows static build.

@vicentebolea vicentebolea requested a review from eisenhauer March 18, 2024 19:49
@ax3l
Copy link
Contributor

ax3l commented Mar 19, 2024

Thanks a lot! I just boarded a plane to Europe, but will try this during the week 👍

@vicentebolea
Copy link
Collaborator Author

Thanks a lot! I just boarded a plane to Europe, but will try this during the week 👍

Safe trip!

@vicentebolea vicentebolea force-pushed the fix-static-blosc2-build branch from 39ebb26 to 284db89 Compare March 19, 2024 20:50
@ax3l
Copy link
Contributor

ax3l commented Mar 25, 2024

32bit windows: this fails due to another reason:

2024-03-25T06:36:25.2420671Z D:\a\openPMD-api\openPMD-api\src\dep-adios2\ADIOS2-fix-static-blosc2-build\source\adios2\toolkit\format\bp5\BP5Serializer.cpp(1600,68): error C2397: conversion from 'uint64_t' to 'size_t' requires a narrowing conversion [D:\a\openPMD-api\openPMD-api\src\build-adios2\source\adios2\adios2_core.vcxproj]
2024-03-25T06:36:25.2425984Z D:\a\openPMD-api\openPMD-api\src\dep-adios2\ADIOS2-fix-static-blosc2-build\source\adios2\toolkit\format\bp5\BP5Serializer.cpp(1600,68): warning C4244: 'initializing': conversion from 'uint64_t' to 'size_t', possible loss of data [D:\a\openPMD-api\openPMD-api\src\build-adios2\source\adios2\adios2_core.vcxproj]
2024-03-25T06:36:25.2429288Z D:\a\openPMD-api\openPMD-api\src\dep-adios2\ADIOS2-fix-static-blosc2-build\source\adios2\toolkit\format\bp5\BP5Serializer.cpp(1601,32): warning C4244: '+=': conversion from 'uint64_t' to 'size_t', possible loss of data [D:\a\openPMD-api\openPMD-api\src\build-adios2\source\adios2\adios2_core.vcxproj]
2024-03-25T06:36:25.2431866Z D:\a\openPMD-api\openPMD-api\src\dep-adios2\ADIOS2-fix-static-blosc2-build\source\adios2\toolkit\format\bp5\BP5Serializer.cpp(1608,69): error C2397: conversion from 'uint64_t' to 'size_t' requires a narrowing conversion [D:\a\openPMD-api\openPMD-api\src\build-adios2\source\adios2\adios2_core.vcxproj]

#4105

@ax3l
Copy link
Contributor

ax3l commented Mar 25, 2024

64bit windows:

  • builds ADIOS2 🎉
  • configures openPMD-api 🎉
  • links openPMD-api 🎉

The patch here is ready to go! 🚀 ✨

Copy link
Contributor

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you, @vicentebolea 🎉

Copy link
Member

@eisenhauer eisenhauer left a comment

Choose a reason for hiding this comment

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

Glad it works!

@eisenhauer eisenhauer enabled auto-merge (squash) March 25, 2024 12:27
@eisenhauer eisenhauer disabled auto-merge March 25, 2024 12:44
@eisenhauer
Copy link
Member

MSAN barely ran out of time. (It was doing the last installation check. Probably we should disable installation checks for the sanitizers as that doesn't exercise any code that we should be sanitizing... Going to bypass branch protections and merge this anyway.

@eisenhauer eisenhauer merged commit 44e4478 into ornladios:master Mar 25, 2024
38 of 39 checks passed
@vicentebolea vicentebolea deleted the fix-static-blosc2-build branch March 25, 2024 16:30
pnorbert added a commit to pnorbert/ADIOS2 that referenced this pull request Mar 26, 2024
* master:
  - Only add campaign store to file name if that is not absolute path - list command supports second argument as path
  flake8 fixes
  Update campaign manager script to handle config file, time in nanosecond format, and avoiding conflict when updating database
  dill 2024-03-12 (ebc98c4d) (ornladios#4091)
  Don't run derived test in MPI mode, it's not written for that (ornladios#4104)
  Fix static blosc2 build (ornladios#4093)
  ci: add ccache job summary (ornladios#4101)
  Fix typo in fortran.rst (ornladios#4102)
  WIP: Make Fortran tests fail with a non-zero exit code (ornladios#4097)
  Bison 3.8 Parser (ornladios#4062)
@vicentebolea vicentebolea mentioned this pull request Apr 2, 2024
vicentebolea added a commit to vicentebolea/ADIOS2 that referenced this pull request Apr 3, 2024
* blosc2: require >=2.10.1
This is needed to simplify our dependency to Blosc2, supporting
prior to 2.10.1 requires us to support to types of blosc2 cmake
dependencies (CONFIG and MODULE) and code this per each version.
* compress: Address blosc2 compress warnings in Windows
* ci: use blosc2 in builds
dmitry-ganyushin added a commit to dmitry-ganyushin/ADIOS2 that referenced this pull request Apr 16, 2024
* origin/adios-xrootd: (164 commits)
  Fixes for FreeBSD, including upstream (ornladios#4138)
  Bump version to v2.10.0
  Setting the derived variable support OFF by default
  Add the CURL function to derived variables (ornladios#4114)
  Add -f file1 [file2...] option to process adios files from a list instead of a campaign recording
  DataPlane Configuration changes (ornladios#4121)
  update doc
  Add attribute support to campaign
  Add a random string to each database name to avoid collision when running multiple applications in the same directory at the same time. Fixes issues with CI that runs ctest in parallel
  Initialize ADIOS::m_UserOption before tentaively calling ProcessUserConfig()
  - set ACA version to  0.1 - remove debug prints - add doc on Campaign Management in Advanced section - change static struct of UserOptions to class member of ADIOS class to make it work with gcc 4.8
  used size_t not int for map indexing to avoid type conversion
  fix remote server test, use the new binary name
  clang-format
  Use the name of the campaign in the cache path to avoid name collision
  rename remote_server to adios2_remote_server
  bug fix: the order of entries in bpdataset table is undefined but the campaign data reader relied on calculating the index as if it was sorted by the insertion order. Use a map instead to store the rowid and use that as index for the bpfile elements.
  Use yaml parser in campaign manager python script
  change a long variable to int64_t to avoid size confusion on windows
  do not include unistd.h
  Fix compiler error Remove extra file not needed
  Add support for user options in ~/.config/adios2/adios2.yaml Currently supported options:
  cmake: add sqlite3 and zlib dep in adios2 cmake pkg
  Different names for MPI and Serial tests (ornladios#4118)
  EVpath upstream to make NO_RDMA more robust (ornladios#4116)
  Warnings (ornladios#4113)
  Don't use assert() in tests (ornladios#4108)
  - Only add campaign store to file name if that is not absolute path - list command supports second argument as path
  flake8 fixes
  Update campaign manager script to handle config file, time in nanosecond format, and avoiding conflict when updating database
  dill 2024-03-12 (ebc98c4d) (ornladios#4091)
  Don't run derived test in MPI mode, it's not written for that (ornladios#4104)
  Fix static blosc2 build (ornladios#4093)
  ci: add ccache job summary (ornladios#4101)
  Fix typo in fortran.rst (ornladios#4102)
  WIP: Make Fortran tests fail with a non-zero exit code (ornladios#4097)
  Bison 3.8 Parser (ornladios#4062)
  Do not create adios-campaign/ unless there is something to record
  Add setup for Aurora (load adios2 as e4s package)
  Completely hide derived variables in C API if not enabled. Print warning inside Fortran F2C function.
  adios2_define_derived_variable C/Fortran API. C is compiled conditionally, like the C++ API. The Fortran function is always available, it will print a WARNING if derived variable is not supported. Added Fortran test for magnitude().
  Fix links to tutorial materials (ornladios#4086)
  BlockIndex.Evaluate() made branch based on BP5 vs BP4. To support CampaignReader engine, decision is made based on whether MinBlocksInfo is supported by engine.
  Update documentation for 2.10 changes to the GPU-backend (ornladios#4083)
  Add test for single string attribute vs string array attribute with a single element
  - Python: fix for scalar reading. If a global value has 1 step (i.e. always in streaming), read returns a 0-dim numpy array (single value). If the variable has multiple steps (only in ReadRandomAccess mode), read returns a 1-dim numpy array even if the step selection is a single step. This way, read of a certain variable always results in the same type of array no matter the number of steps selected. - Python: fix for string attributes: return a string, not a list of one element which is a string, to be consistent with string global values and with other APIs.
  format more
  format
  Python: add the same treatment to attributes as to variables before: return scalars (0-dim ndarray) for single value attributes.
  Raise an exception if remote open fails (ornladios#4069)
  Fortran bindings for memory space related functions (ornladios#4077)
  consolidate (ornladios#4078)
  Making the Detect memory space available regardless of the backend used
  Testing code for the C bindings with memory space API
  Adding c bindings for getting the shape of a variable based on a memory space
  Adding c bindings for setting and getting the memory space
  Small typo fixes
  - Restructure python API doc in separate main topic, add working example to it. - What's new for 2.10 - Usage on DOE machines
  Fix Reord to use MinBlocksInfo where appropriate (ornladios#4071)
  Using the correct flag to detect the CUDA backend in ZFP
  clang-format fix
  fixed warning
  added support to read back from H5T_STRING VARIABLES it turns out strings written out through h5py are all variable strings
  format
  fixes to still be able to build with gcc 4.8.2. Needed for OLCF DTN nodes
  Add minmax and shape functions to CampaignReader, so that per-block info is complete when listing campaign archives
  do not flush io and adios in read mode. BP5 reader does not like it.
  Campaign engine is recognized by file extension.
  WIP. Changed the name of the campaign config file.
  Added reading of configuration parametyers from ./config/adios2
  Use GetEstimatedSize in encryption operator plugin
  Allow plugin operators to take advantage of the estimated size API
  Setting the derived variable support OFF by default
  Add the CURL function to derived variables (ornladios#4114)
  Add -f file1 [file2...] option to process adios files from a list instead of a campaign recording
  DataPlane Configuration changes (ornladios#4121)
  update doc
  Add attribute support to campaign
  Add a random string to each database name to avoid collision when running multiple applications in the same directory at the same time. Fixes issues with CI that runs ctest in parallel
  Initialize ADIOS::m_UserOption before tentaively calling ProcessUserConfig()
  - set ACA version to  0.1 - remove debug prints - add doc on Campaign Management in Advanced section - change static struct of UserOptions to class member of ADIOS class to make it work with gcc 4.8
  used size_t not int for map indexing to avoid type conversion
  fix remote server test, use the new binary name
  clang-format
  Use the name of the campaign in the cache path to avoid name collision
  rename remote_server to adios2_remote_server
  bug fix: the order of entries in bpdataset table is undefined but the campaign data reader relied on calculating the index as if it was sorted by the insertion order. Use a map instead to store the rowid and use that as index for the bpfile elements.
  Use yaml parser in campaign manager python script
  change a long variable to int64_t to avoid size confusion on windows
  do not include unistd.h
  Fix compiler error Remove extra file not needed
  Add support for user options in ~/.config/adios2/adios2.yaml Currently supported options:
  cmake: add sqlite3 and zlib dep in adios2 cmake pkg
  Different names for MPI and Serial tests (ornladios#4118)
  EVpath upstream to make NO_RDMA more robust (ornladios#4116)
  Warnings (ornladios#4113)
  Don't use assert() in tests (ornladios#4108)
  - Only add campaign store to file name if that is not absolute path - list command supports second argument as path
  flake8 fixes
  Update campaign manager script to handle config file, time in nanosecond format, and avoiding conflict when updating database
  ...
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.

3 participants