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 option to build DXTex as shared lib to cmake #573

Closed
wants to merge 0 commits into from

Conversation

bmarques1995
Copy link

This pull request solves the #572, only for CMake, and doesn't modify the behavior of the previous build, tested, compiles for all utilities (ddsview, texconv, texassemble and texdiag)

@bmarques1995 bmarques1995 force-pushed the main branch 3 times, most recently from 52ebf80 to dcea118 Compare January 29, 2025 14:28
@walbourn walbourn added the cmake Related to CMake build label Jan 29, 2025
@walbourn
Copy link
Member

I've always had these libraries focused on static C++ lib scenarios and not built as DLLs since there's no consistent ABI design (COM or C flat).

What's your scenario that a DLL is required?

Also, you could move all the declspec annotations to the class level instead of the individual methods for the non-free functions which would at least minimize the code-lines changed.

@bmarques1995
Copy link
Author

I've always had these libraries focused on static C++ lib scenarios and not built as DLLs since there's no consistent ABI design (COM or C flat).

What's your scenario that a DLL is required?

Also, you could move all the declspec annotations to the class level instead of the individual methods for the non-free functions which would at least minimize the code-lines changed.

Hi Walbourn, my point to let the user decide if it want to use static or shared libs, taking my example, I'm developing a game engine, and, often i request new features and updates of packages, and since the DirectXTex is only static, at every change I need to redistribute the entire binary.

I believe u have seen that the default behavior was mantained (static linkage). Also, this change can turn the lib closer to KTX (OpenGL and Vulkan texture handler).

About your request to reduce the number of declspec, this is fine for me.

CMakeLists.txt Outdated Show resolved Hide resolved
DirectXTex/DirectXTex.h Outdated Show resolved Hide resolved
@walbourn
Copy link
Member

Required some test updates to get this to build.

walbourn/directxtextest#58

@bmarques1995
Copy link
Author

Required some test updates to get this to build.

walbourn/directxtextest#58

Walbourn, about the failed tests, do u have any idea about what is causing the run-vcpkg error? I'm failing in reproduce the error

@bmarques1995 bmarques1995 force-pushed the main branch 18 times, most recently from d29aa34 to 07d80c5 Compare January 30, 2025 23:15
@bmarques1995 bmarques1995 force-pushed the main branch 15 times, most recently from b05cbd8 to fabee0f Compare January 31, 2025 01:28
@bmarques1995
Copy link
Author

bmarques1995 commented Jan 31, 2025

Required some test updates to get this to build.
walbourn/directxtextest#58

Walbourn, about the failed tests, do u have any idea about what is causing the run-vcpkg error? I'm failing in reproduce the error

Forget this Walbourn, I've fixed the workflows, now they are handling the latest vcpkg version

@bmarques1995
Copy link
Author

bmarques1995 commented Jan 31, 2025

Required some test updates to get this to build.

walbourn/directxtextest#58

About the previous reply, I saw that the modified workflows were not getting the correct vcpkg commit hash, so I added the step to get the hash and saved in the env, to later get it in ${{ env.VCPKG_COMMIT_ID}}

@walbourn
Copy link
Member

About the previous reply, I saw that the modified workflows were not getting the correct vcpkg commit hash, so I added the step to get the hash and saved in the env, to later get it in ${{ env.VCPKG_COMMIT_ID}}

I had the VCPKG-based workflows using a commitId value from the 'repository variables' and set them to the 'formal releases' rather than using 'latest HEAD', but workflows run by non-owners of the repo are restricted as secrets so that's why they were failing.

I wonder if I can set it up to use the var if accessible and fallback to "HEAD" otherwise.

@walbourn
Copy link
Member

I'm working on some additional commits here to add another build pipeline to validate this model and add VERSIONINFO... I should have it ready this weekend and we can wrap this PR up.

Thanks!

@bmarques1995
Copy link
Author

About the previous reply, I saw that the modified workflows were not getting the correct vcpkg commit hash, so I added the step to get the hash and saved in the env, to later get it in ${{ env.VCPKG_COMMIT_ID}}

I had the VCPKG-based workflows using a commitId value from the 'repository variables' and set them to the 'formal releases' rather than using 'latest HEAD', but workflows run by non-owners of the repo are restricted as secrets so that's why they were failing.

I wonder if I can set it up to use the var if accessible and fallback to "HEAD" otherwise.

So, in this case, I only

I'm working on some additional commits here to add another build pipeline to validate this model and add VERSIONINFO... I should have it ready this weekend and we can wrap this PR up.

Thanks!

You're welcome.

.github/workflows/bvt.yml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Related to CMake build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants