Skip to content

Adding vcpkg integration#825

Merged
mjp41 merged 8 commits intomicrosoft:mainfrom
matajoh:vcpkg
Mar 13, 2026
Merged

Adding vcpkg integration#825
mjp41 merged 8 commits intomicrosoft:mainfrom
matajoh:vcpkg

Conversation

@matajoh
Copy link
Member

@matajoh matajoh commented Mar 10, 2026

This PR proposes a vcpkg integration for snmalloc in both header-only library mode and static-shim configurations. Once accepted, it will be possible to add snmalloc to vcpkg repositories.

Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

Is there a way this can be tested in CI? I just want to add something so this can't be broken in the future?

Comment on lines +36 to +37
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/include")
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/share")
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what these lines are doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

vcpkg only allows you to have one copy of the headers etc. under the install directory. If it didn't remove the debug copies, then the post-validation step would fail.

matajoh added 5 commits March 11, 2026 10:31
Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
@matajoh
Copy link
Member Author

matajoh commented Mar 11, 2026

Is there a way this can be tested in CI? I just want to add something so this can't be broken in the future?

Great suggestion, I've added CI tests for this which simulate using vcpkg to install the library.

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

LGTM, just wondering if the test is actually testing that snmalloc is there in the static case.

Comment on lines +10 to +14
void* p = malloc(64);
if (p == nullptr)
return 1;
free(p);
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to check if actually uses snmalloc. I think something like:

Suggested change
void* p = malloc(64);
if (p == nullptr)
return 1;
free(p);
return 0;
char* p = (char*)malloc(64);
if (p == nullptr)
return 1;
if ((p + 64) != __malloc_endpointer(p))
return 1;
free(p);
return 0;

where you add a prototype of

void* __malloc_end_pointer(void*);

This will then call an snmalloc specific feature.

Not sure if what you are building will have a prefix of sn_ for all the methods in the static shim.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point.

target_include_directories(snmalloc
INTERFACE
$<INSTALL_INTERFACE:include/snmalloc>
$<INSTALL_INTERFACE:include>
Copy link
Member

Choose a reason for hiding this comment

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

Is this the bit that has been wrong as an install target.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
Signed-off-by: Matthew A Johnson <matjoh@microsoft.com>
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

LGTM

@mjp41 mjp41 merged commit 139180c into microsoft:main Mar 13, 2026
191 checks passed
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.

2 participants