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.

Sign up for GitHub

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

Jump to bottom

Merge: Always initialize LibGit #3221

Merged
merged 2 commits into from
Nov 16, 2019
Merged

Merge: Always initialize LibGit #3221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7d6779
Merge: Always initialize LibGit
dominicjaeger Nov 15, 2019
b7cd476
Add release note
dominicjaeger Nov 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/news/_preparation_next_release.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ you up to date with the multi-language support provided by Elektra.
- Improved `range` plugin error message. _(Michael Zronek)_
- Improved error codes documentation to clarify the hierarchy for developers. _(Michael Zronek)_
- Release notes now use git's union merge driver. _(Dominic Jäger)_
- Please remove me. I'm only here for the build server. _(Dominic Jäger)_

## Infrastructure

Expand Down
3 changes: 0 additions & 3 deletions src/libs/merge/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ if (LibGit2_FOUND)
set (CMERGE_INCLUDE_DIRS ${LibGit2_INCLUDE_DIRS})
set (CMERGE_LIBRARY_DIRS ${LibGit2_LIBRARIES})
add_definitions (-DLIBGITFOUND)
if (${CMAKE_SYSTEM_NAME} MATCHES "Linux")
add_definitions (-DCMERGE_ON_LINUX)
endif ()
endif ()

# This is derived from LibAddPlugin. LibAddLib has no such functionality yet. This is required so that libelektra-full.so has defined
Expand Down
8 changes: 0 additions & 8 deletions src/libs/merge/kdbmerge.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,21 +988,13 @@ KeySet * elektraMerge (KeySet * our, Key * ourRoot, KeySet * their, Key * theirR
}

#ifdef LIBGITFOUND
#ifndef CMERGE_ON_LINUX
git_libgit2_init ();
ELEKTRA_LOG ("Initializing LibGit2");
#else
ELEKTRA_LOG ("Not initializing LibGit2, but using it all the same.");
#endif
ELEKTRA_LOG ("cmerge can use libgit2 to handle arrays");
if (handleArrays (ourCropped, theirCropped, baseCropped, result, informationKey, strategy) > 0)
{
ksDel (result);
return NULL;
}
#ifndef CMERGE_ON_LINUX
git_libgit2_shutdown ();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we do the init but no shutdown here?
Btw if it is definitely an upstream problem/leak we can make a valgrind suppression for it and report the issue there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some libraries are not safe to be initialized again after shutdown. Furthermore we do not really know if anybody else is using libgit2 in our process. So it can make sense to not call shutdown (if it is not working properly).

The whole global init and shutdown is calling for a lot of trouble. Unfortunately, many libraries do not use proper solutions (like handles).

In theory (with mutex protection and ref counting) multiple init/shutdown could be supported but it is rare that this is working properly.

and report the issue there

I agree.

Copy link
Member

Choose a reason for hiding this comment

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

Some libraries are not safe to be initialized again after shutdown.

Ok, but the shutdown was clearly called before, and now we only call init. I assumed it was removed by mistake. If there is a reason not to call it, I'm fine with that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we do the init but no shutdown here?

That was a mistake :/ Nonetheless, it is actually optional

Usually you don’t need to call the shutdown function as the operating system will take care of reclaiming resources, but if your application uses libgit2 in some areas which are not usually active, you can use git_libgit2_shutdown(); to ask the library to clean up the global state. (source)

Btw if it is definitely an upstream problem/leak we can make a valgrind suppression for it

I added memleak labels for both tests in this PR.

and report the issue there.

The person who answered my stackoverflow question also appears in the LibGit2 contributor list, so I assume they are aware.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added memleak labels for both tests in this PR.

It is better if we suppress everything from libgit. This way we still check if our code has memleaks.

The person who answered my stackoverflow question also appears in the LibGit2 contributor list, so I assume they are aware.

You never know. Better we report it. But they are obviously only interested in the latest versions, maybe they already fixed it?

#endif
#else
ELEKTRA_LOG ("cmerge can NOT use libgit2 to handle arrays");
#endif
Expand Down
5 changes: 2 additions & 3 deletions tests/ctest/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,5 @@ target_link_elektra (test_opts elektra-opts)

target_link_elektra (test_cmerge elektra-merge)

if (NOT ${CMAKE_SYSTEM_NAME} MATCHES "Linux")
set_property (TEST test_cmerge PROPERTY LABELS memleak)
endif ()
# LibGit leaks memory
set_property (TEST test_cmerge PROPERTY LABELS memleak)
16 changes: 6 additions & 10 deletions tests/shell/shell_recorder/tutorial_wrapper/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,12 @@ if (ENABLE_ASAN AND APPLE AND DEFINED ENV{CIRRUS_CI})
else (ENABLE_ASAN AND APPLE AND DEFINED ENV{CIRRUS_CI})
add_msr_test (tutorial_storage_plugins "${CMAKE_SOURCE_DIR}/doc/tutorials/storage-plugins.md" REQUIRED_PLUGINS type yamlcpp)
endif (ENABLE_ASAN AND APPLE AND DEFINED ENV{CIRRUS_CI})
if (${CMAKE_SYSTEM_NAME} MATCHES "Linux")
add_msr_test (cmerge "${CMAKE_SOURCE_DIR}/doc/tutorials/cmerge.md" REQUIRED_PLUGINS hosts line)
else ()
add_msr_test (cmerge "${CMAKE_SOURCE_DIR}/doc/tutorials/cmerge.md"
REQUIRED_PLUGINS hosts
line
PROPERTY
LABELS
memleak)
endif ()
add_msr_test (cmerge "${CMAKE_SOURCE_DIR}/doc/tutorials/cmerge.md"
REQUIRED_PLUGINS hosts
line
PROPERTY
LABELS
memleak)

if (ENABLE_ASAN)
message (STATUS "Excluding Markdown Shell Recorder test for `validation`, as it leaks memory and fails with ASAN enabled")
Expand Down