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

Add backward compatibility workflow and tests #1063

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bratpiorka
Copy link
Contributor

@bratpiorka bratpiorka commented Jan 28, 2025

Add backward compatibility workflow and tests.

Suppose we have umfFunc_0_11 and umfFunc_0_10. With this PR:

  • "umfFunc" == "umfFunc_0_11" in compile time (using #define)
  • UMF exports 2 symbols: "umfFunc@@UMF_0.10" and "umfFunc_0_11@@UMF_0.11"
  • dlsym("umfFunc") == "umfFunc_0_10"
  • dlsym("umfFunc_0_11") == "umfFunc_0_11"

Major changes:

  • use UMF_PROVIDER_OPS_VERSION_CURRENT and UMF_POOL_OPS_VERSION_CURRENT for versioning memory provider and pools struct instead of UMF_VERSION_CURRENT that would be used only for versioning the lib
  • add new implementation for umfDevDaxMemoryProviderOps and umfFileMemoryProviderOps ver 0.11
  • handle old (0.10) provider ops in umfMemoryProviderCreate
  • do not assert when the user pass an old provider/pool version of ops to umfMemoryProvider/PoolCreate and print a warning instead
  • add tests for multiple symbols (file and DevDax providers ops) - assume that calling dlsym without a specified version returns the oldest implementation

fixes #908

@bratpiorka bratpiorka force-pushed the rrudnick_compat_wflow branch 3 times, most recently from b4226c3 to a341c50 Compare January 29, 2025 11:25
src/memory_provider.c Outdated Show resolved Hide resolved
src/memory_provider.c Outdated Show resolved Hide resolved
@bratpiorka bratpiorka force-pushed the rrudnick_compat_wflow branch 10 times, most recently from 2a23695 to be7dbb6 Compare January 31, 2025 12:45
@bratpiorka bratpiorka changed the title [DRAFT] compat Add backward compatibility workflow and test Jan 31, 2025
@bratpiorka bratpiorka changed the title Add backward compatibility workflow and test Add backward compatibility workflow and tests Jan 31, 2025
@bratpiorka bratpiorka force-pushed the rrudnick_compat_wflow branch from be7dbb6 to 51391d7 Compare January 31, 2025 17:32
@oneapi-src oneapi-src deleted a comment from github-actions bot Jan 31, 2025
@oneapi-src oneapi-src deleted a comment from github-actions bot Jan 31, 2025
@bratpiorka bratpiorka force-pushed the rrudnick_compat_wflow branch 3 times, most recently from 143d33a to 965b918 Compare January 31, 2025 17:57
@bratpiorka bratpiorka force-pushed the rrudnick_compat_wflow branch 10 times, most recently from 49a57f5 to 3ecbe58 Compare February 2, 2025 15:49
@bratpiorka bratpiorka marked this pull request as ready for review February 3, 2025 07:45
@bratpiorka bratpiorka requested a review from a team as a code owner February 3, 2025 07:45
tag: ["v0.10.1"]
with:
tag: ${{matrix.tag}}

Copy link
Contributor

Choose a reason for hiding this comment

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

missing EOF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

working-directory: ${{github.workspace}}/tag_version
run: |
VERSION=$(git describe --tags --abbrev=0 | grep -oP '\d+\.\d+\.\d+')
echo "tag version: $VERSION"
Copy link
Contributor

Choose a reason for hiding this comment

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

why "cut it"? Just running git describe --tags should suffice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

run: >
cmake
-B ${{github.workspace}}/tag_version/build
-DCMAKE_INSTALL_PREFIX="${{github.workspace}}/tag_version/build/install"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe drop "build" in the path here, so the installation will be sep. from build?

also, do we even use this install_prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

working-directory: ${{github.workspace}}/latest_version
run: |
VERSION=$(git describe --tags --abbrev=0 | grep -oP '\d+\.\d+\.\d+')
echo "checked version: $VERSION"
Copy link
Contributor

Choose a reason for hiding this comment

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

.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

run: >
cmake
-B ${{github.workspace}}/latest_version/build
-DCMAKE_INSTALL_PREFIX="${{github.workspace}}/latest_version/build/install"
Copy link
Contributor

Choose a reason for hiding this comment

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

.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

-DCMAKE_C_COMPILER=cl
-DCMAKE_CXX_COMPILER=cl
-DUMF_BUILD_SHARED_LIBRARY=ON
-DUMF_BUILD_TESTS=ON
Copy link
Contributor

Choose a reason for hiding this comment

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

.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


# NOTE: exclude umf-provider_coarse, umf-disjointCoarseMallocPool,
# umf-jemalloc_coarse_file, umf-scalable_coarse_file as they use Coarse
# NOTE2: on Windows we simply overwrite the umf.dll
Copy link
Contributor

Choose a reason for hiding this comment

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

heh, this NOTE2 should be moved a little down, I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -38,7 +38,11 @@ static umf_result_t umfPoolCreateInternal(const umf_memory_pool_ops_t *ops,
return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
}

assert(ops->version == UMF_VERSION_CURRENT);
if (ops->version != UMF_POOL_OPS_VERSION_CURRENT) {
LOG_WARN("memory pool ops version \"%d\" is different than current "
Copy link
Contributor

Choose a reason for hiding this comment

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

...then the current..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

umf_memory_provider_ops_t ops_current_ver;
if (ops->version != UMF_PROVIDER_OPS_VERSION_CURRENT) {
LOG_WARN("Memory Provider ops version \"%d\" is different than current "
Copy link
Contributor

Choose a reason for hiding this comment

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

. (pls apply in all messages)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// @brief Version of the Memory Pool ops structure.
/// NOTE: This is equal to the latest UMF version, in which the ops structure
/// has been modified.
#define UMF_POOL_OPS_VERSION_CURRENT UMF_MAKE_VERSION(0, 11)
Copy link
Contributor

Choose a reason for hiding this comment

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

should RELEASE_STEPS include this info..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

all that code in 1 commit? ;-) Consider splitting or, at least, describing it a little more in the commit msg, pls.

@bratpiorka bratpiorka force-pushed the rrudnick_compat_wflow branch 4 times, most recently from 5aafd9a to 5fa5a8b Compare February 4, 2025 13:33
@bratpiorka
Copy link
Contributor Author

all that code in 1 commit? ;-) Consider splitting or, at least, describing it a little more in the commit msg, pls.

done

@bratpiorka bratpiorka marked this pull request as draft February 6, 2025 12:59
@bratpiorka bratpiorka added this to the v0.11.x milestone Feb 10, 2025
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.

Implement backward compatibility support in UMF
4 participants