You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Sahlberg <da...@gmail.com> on 2024/07/16 15:40:31 UTC
Building with CMake on Linux
Hi,
Inspired by Timofei's work on the cmake branch, I checked it out and tried
to build on Linux.
It worked, almost.
[[[
[ 17%] Building C object
CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/sysinfo.c.o
/home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c: In function
‘linux_release_name’:
/home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c:649:31: warning:
implicit declaration of function ‘release_name_from_uname’
[-Wimplicit-function-declaration]
649 | const char *uname_release = release_name_from_uname(pool);
| ^~~~~~~~~~~~~~~~~~~~~~~
/home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c:649:31: warning:
initialization of ‘const char *’ from ‘int’ makes pointer from integer
without a cast [-Wint-conversion]
/home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c: In function
‘linux_shared_libs’:
/home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c:750:65: warning:
implicit declaration of function ‘getpid’ [-Wimplicit-function-declaration]
750 | const char *maps = apr_psprintf(pool, "/proc/%ld/maps",
(long)getpid());
|
^~~~~~
]]]
The above warnings were caused by missing defines HAVE_SYS_UTSNAME_H,
HAVE_UNAME and HAVE_UNISTD_H.
I also got the following error:
[[[
[ 64%] Linking C shared library liblibsvn_ra-1.so
/usr/bin/ld: liblibsvn_ra_svn.a(marshal.c.o): warning: relocation against
`svn_ctype_table' in read-only section `.text'
/usr/bin/ld: liblibsvn_ra_svn.a(client.c.o): relocation R_X86_64_PC32
against symbol `svn_ctype_table' can not be used when making a shared
object; recompile with -fPIC
/usr/bin/ld: final link failed: bad value
collect2: error: ld returned 1 exit status
gmake[2]: *** [CMakeFiles/libsvn_ra.dir/build.make:186: liblibsvn_ra-1.so]
Error 1
gmake[1]: *** [CMakeFiles/Makefile2:330: CMakeFiles/libsvn_ra.dir/all]
Error 2
gmake: *** [Makefile:136: all] Error 2
]]]
I made some effort trying to figure out the proper way to fix this and I
came up with the following changes. After this the build succeeds and I get
a working svn binary.
I'm SURE this doesn't follow CMake best-practice but I'd be happy if it
could be a starting point in getting CMake to work under Linux. I'd also be
happy to see how it SHOULD be done :-)
To get this to work under Macos, someone should look at the
SVN_HAVE_MACOS_PLIST define.
[[[
Index: CMakeLists.txt
===================================================================
--- CMakeLists.txt (revision 1919275)
+++ CMakeLists.txt (working copy)
@@ -125,8 +125,29 @@
enable_testing()
endif()
+include(CheckSymbolExists)
+include(CheckIncludeFiles)
+check_include_files("sys/utsname.h" HAVE_SYS_UTSNAME_H)
+if (HAVE_SYS_UTSNAME_H)
+ add_compile_definitions(HAVE_SYS_UTSNAME_H)
+
+ check_symbol_exists(uname sys/utsname.h HAVE_UNAME)
+ if (HAVE_UNAME)
+ add_compile_definitions(HAVE_UNAME)
+ endif()
+endif()
+check_include_files("unistd.h" HAVE_UNISTD_H)
+if (HAVE_UNISTD_H)
+ add_compile_definitions(HAVE_UNISTD_H)
+endif()
+
+if (BUILD_SHARED_LIBS)
+ set(CMAKE_POSITION_INDEPENDENT_CODE ON)
+endif()
+
if(SVN_BUILD_SHARED_FS)
set(SVN_FS_BUILD_TYPE SHARED)
+ set(CMAKE_POSITION_INDEPENDENT_CODE ON)
else()
set(SVN_FS_BUILD_TYPE STATIC)
endif()
@@ -133,6 +154,7 @@
if(SVN_BUILD_SHARED_RA)
set(SVN_RA_BUILD_TYPE SHARED)
+ set(CMAKE_POSITION_INDEPENDENT_CODE ON)
else()
set(SVN_RA_BUILD_TYPE STATIC)
endif()
]]]
Re: Building with CMake on Linux
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Mon, Jul 29, 2024 at 10:55 PM Daniel Sahlberg
<da...@gmail.com> wrote:
>
> Den sön 28 juli 2024 kl 20:00 skrev Stefan Sperling <st...@apache.org>:
>>
>> On Fri, Jul 26, 2024 at 10:41:14PM +0200, Daniel Sahlberg wrote:
>> > I also tried OpenBSD but failed on missing Expat ("Could not find a package
>> > configuration file provided by "expat" with any of the following names:
>> > expatConfig.cmake expat-config.cmake [....]". I could not figure out if
>> > this is a missing package in OpenBSD or that Expat on OpenBSD doesn't
>> > provide the required cmake file. I hope @Stefan Sperling <st...@apache.org>
>> > can chime in on this issue!
>>
>> The expat library is part of openbsd base + comp installation sets.
>> Any default install will have it.
>>
>> There is a pkg-config file: /usr/lib/pkgconfig/expat.pc
>>
>> cmake needs to be installed with pkg_add, and a FindEXPAT.cmake module
>> is part of the cmake package. Does this help?
>>
>> Contents of FindEXPAT.cmake listed below:
>
>
> [cut]
>
> Thank you Stefan, that helped a lot.
>
> CMake is far from my comfort zone but I've tested the following change and it seems to work on both OpenBSD and Ubuntu 24.04:
>
> [[[
> Index: CMakeLists.txt
> ===================================================================
> --- CMakeLists.txt (revision 1919555)
> +++ CMakeLists.txt (working copy)
> @@ -174,8 +174,8 @@
>
> ### EXPAT
>
> -find_package(expat CONFIG REQUIRED)
> -add_library(external-xml ALIAS expat::expat)
> +find_package(EXPAT REQUIRED)
> +add_library(external-xml ALIAS EXPAT::EXPAT)
>
> ### LZ4
>
> @@ -481,7 +481,7 @@
> message(STATUS " Dependecies:")
> message(STATUS " APR ........................... : ${APR_VERSION}")
> message(STATUS " APR-Util ...................... : ${APRUTIL_VERSION}")
> -message(STATUS " EXPAT ......................... : ${expat_VERSION}")
> +message(STATUS " EXPAT ......................... : ${EXPAT_VERSION_STRING}")
> message(STATUS " ZLIB .......................... : ${ZLIB_VERSION}")
> message(STATUS " LZ4 ........................... : ${lz4_VERSION}")
> message(STATUS " UTF8PROC ...................... : ${UTF8PROC_VERSION}")
> ]]]
>
> @Timofei, does this make sense? Notably, does it work on Windows?
>
> Kind regards,
> Daniel
>
Hi,
Thank you!
There are two different standard ways for finding dependencies in
CMake; in Config mode or using modules. There is an explanation about
them in the find_package documentation [1]. The advantage of using
config is that it is generated during the build of the library, so it
could handle the library configuration more correctly. Initially, I
wanted to use config for finding all libraries, but most of the
libraries just don't provide it, so I decided to use it only for the
libraries which do (these libraries are Expat and LZ4). Also, see
change from r1918954 where this behaviour has changed.
The original problem is that the Expat config file wasn't generated
correctly for that system or it is not included into a package (in the
current case, I think that it is just excluded from the package).
So, there are lots of problems with using Config mode; it is so
challenging to generate a config file, the configs could be just
excluded from the package that is used for build, and sometimes they
might be unstable. The resolution that Daniel suggested seems to be
nice, since we're migrating from configs. Additionally, if so, there
could be a similar problem with LZ4, when SVN_USE_INTERNAL_LZ4 is
disabled. If there is, we should also do something similar for
external LZ4.
Hope this explains well, because it is so complicated :)
[1] https://cmake.org/cmake/help/latest/command/find_package.html
--
Timofei Zhakov
Re: Building with CMake on Linux
Posted by Daniel Sahlberg <da...@gmail.com>.
Den sön 28 juli 2024 kl 20:00 skrev Stefan Sperling <st...@apache.org>:
> On Fri, Jul 26, 2024 at 10:41:14PM +0200, Daniel Sahlberg wrote:
> > I also tried OpenBSD but failed on missing Expat ("Could not find a
> package
> > configuration file provided by "expat" with any of the following names:
> > expatConfig.cmake expat-config.cmake [....]". I could not figure out if
> > this is a missing package in OpenBSD or that Expat on OpenBSD doesn't
> > provide the required cmake file. I hope @Stefan Sperling <
> stsp@apache.org>
> > can chime in on this issue!
>
> The expat library is part of openbsd base + comp installation sets.
> Any default install will have it.
>
> There is a pkg-config file: /usr/lib/pkgconfig/expat.pc
>
> cmake needs to be installed with pkg_add, and a FindEXPAT.cmake module
> is part of the cmake package. Does this help?
>
> Contents of FindEXPAT.cmake listed below:
>
[cut]
Thank you Stefan, that helped a lot.
CMake is far from my comfort zone but I've tested the following change and
it seems to work on both OpenBSD and Ubuntu 24.04:
[[[
Index: CMakeLists.txt
===================================================================
--- CMakeLists.txt (revision 1919555)
+++ CMakeLists.txt (working copy)
@@ -174,8 +174,8 @@
### EXPAT
-find_package(expat CONFIG REQUIRED)
-add_library(external-xml ALIAS expat::expat)
+find_package(EXPAT REQUIRED)
+add_library(external-xml ALIAS EXPAT::EXPAT)
### LZ4
@@ -481,7 +481,7 @@
message(STATUS " Dependecies:")
message(STATUS " APR ........................... : ${APR_VERSION}")
message(STATUS " APR-Util ...................... : ${APRUTIL_VERSION}")
-message(STATUS " EXPAT ......................... : ${expat_VERSION}")
+message(STATUS " EXPAT ......................... :
${EXPAT_VERSION_STRING}")
message(STATUS " ZLIB .......................... : ${ZLIB_VERSION}")
message(STATUS " LZ4 ........................... : ${lz4_VERSION}")
message(STATUS " UTF8PROC ...................... : ${UTF8PROC_VERSION}")
]]]
@Timofei, does this make sense? Notably, does it work on Windows?
Kind regards,
Daniel
Re: Building with CMake on Linux
Posted by Stefan Sperling <st...@apache.org>.
On Fri, Jul 26, 2024 at 10:41:14PM +0200, Daniel Sahlberg wrote:
> I also tried OpenBSD but failed on missing Expat ("Could not find a package
> configuration file provided by "expat" with any of the following names:
> expatConfig.cmake expat-config.cmake [....]". I could not figure out if
> this is a missing package in OpenBSD or that Expat on OpenBSD doesn't
> provide the required cmake file. I hope @Stefan Sperling <st...@apache.org>
> can chime in on this issue!
The expat library is part of openbsd base + comp installation sets.
Any default install will have it.
There is a pkg-config file: /usr/lib/pkgconfig/expat.pc
cmake needs to be installed with pkg_add, and a FindEXPAT.cmake module
is part of the cmake package. Does this help?
Contents of FindEXPAT.cmake listed below:
# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
# file Copyright.txt or https://cmake.org/licensing for details.
#[=======================================================================[.rst:
FindEXPAT
---------
Find the native Expat headers and library.
Expat is a stream-oriented XML parser library written in C.
Imported Targets
^^^^^^^^^^^^^^^^
.. versionadded:: 3.10
This module defines the following :prop_tgt:`IMPORTED` targets:
``EXPAT::EXPAT``
The Expat ``expat`` library, if found.
Result Variables
^^^^^^^^^^^^^^^^
This module will set the following variables in your project:
``EXPAT_INCLUDE_DIRS``
where to find expat.h, etc.
``EXPAT_LIBRARIES``
the libraries to link against to use Expat.
``EXPAT_FOUND``
true if the Expat headers and libraries were found.
Hints
^^^^^
``EXPAT_USE_STATIC_LIBS``
.. versionadded:: 3.28
Set to ``TRUE`` to use static libraries.
#]=======================================================================]
find_package(PkgConfig QUIET)
pkg_check_modules(PC_EXPAT QUIET expat)
# Look for the header file.
find_path(EXPAT_INCLUDE_DIR NAMES expat.h HINTS ${PC_EXPAT_INCLUDE_DIRS})
set(EXPAT_NAMES expat expatw)
set(EXPAT_NAMES_DEBUG expatd expatwd)
if(WIN32)
if(EXPAT_USE_STATIC_LIBS)
list(APPEND EXPAT_NAMES expatMT expatwMT)
list(APPEND EXPAT_NAMES_DEBUG expatdMT expatwdMT)
else()
list(APPEND EXPAT_NAMES expatMT expatMD expatwMT expatwMD)
list(APPEND EXPAT_NAMES_DEBUG expatdMT expatdMD expatwdMT expatwdMD)
endif()
endif()
# Allow EXPAT_LIBRARY to be set manually, as the location of the expat library
if(NOT EXPAT_LIBRARY)
if(DEFINED CMAKE_FIND_LIBRARY_PREFIXES)
set(_expat_ORIG_CMAKE_FIND_LIBRARY_PREFIXES "${CMAKE_FIND_LIBRARY_PREFIXES}")
else()
set(_expat_ORIG_CMAKE_FIND_LIBRARY_PREFIXES)
endif()
if(WIN32)
list(APPEND CMAKE_FIND_LIBRARY_PREFIXES "" "lib")
endif()
# Look for the library.
find_library(EXPAT_LIBRARY_RELEASE NAMES ${EXPAT_NAMES} NAMES_PER_DIR HINTS ${PC_EXPAT_LIBRARY_DIRS} PATH_SUFFIXES lib)
find_library(EXPAT_LIBRARY_DEBUG NAMES ${EXPAT_NAMES_DEBUG} NAMES_PER_DIR HINTS ${PC_EXPAT_LIBRARY_DIRS} PATH_SUFFIXES lib)
# Restore the original find library ordering
if(DEFINED _expat_ORIG_CMAKE_FIND_LIBRARY_PREFIXES)
set(CMAKE_FIND_LIBRARY_PREFIXES "${_expat_ORIG_CMAKE_FIND_LIBRARY_PREFIXES}")
else()
set(CMAKE_FIND_LIBRARY_PREFIXES)
endif()
include(${CMAKE_CURRENT_LIST_DIR}/SelectLibraryConfigurations.cmake)
select_library_configurations(EXPAT)
endif()
unset(EXPAT_NAMES)
unset(EXPAT_NAMES_DEBUG)
if(EXPAT_INCLUDE_DIR AND EXISTS "${EXPAT_INCLUDE_DIR}/expat.h")
file(STRINGS "${EXPAT_INCLUDE_DIR}/expat.h" expat_version_str
REGEX "^#[\t ]*define[\t ]+XML_(MAJOR|MINOR|MICRO)_VERSION[\t ]+[0-9]+$")
unset(EXPAT_VERSION_STRING)
foreach(VPART MAJOR MINOR MICRO)
foreach(VLINE ${expat_version_str})
if(VLINE MATCHES "^#[\t ]*define[\t ]+XML_${VPART}_VERSION[\t ]+([0-9]+)$")
set(EXPAT_VERSION_PART "${CMAKE_MATCH_1}")
if(EXPAT_VERSION_STRING)
string(APPEND EXPAT_VERSION_STRING ".${EXPAT_VERSION_PART}")
else()
set(EXPAT_VERSION_STRING "${EXPAT_VERSION_PART}")
endif()
endif()
endforeach()
endforeach()
endif()
include(${CMAKE_CURRENT_LIST_DIR}/FindPackageHandleStandardArgs.cmake)
FIND_PACKAGE_HANDLE_STANDARD_ARGS(EXPAT
REQUIRED_VARS EXPAT_LIBRARY EXPAT_INCLUDE_DIR
VERSION_VAR EXPAT_VERSION_STRING)
# Copy the results to the output variables and target.
if(EXPAT_FOUND)
set(EXPAT_INCLUDE_DIRS ${EXPAT_INCLUDE_DIR})
if(NOT EXPAT_LIBRARIES)
set(EXPAT_LIBRARIES ${EXPAT_LIBRARY})
endif()
if(NOT TARGET EXPAT::EXPAT)
add_library(EXPAT::EXPAT UNKNOWN IMPORTED)
set_target_properties(EXPAT::EXPAT PROPERTIES
IMPORTED_LINK_INTERFACE_LANGUAGES "C"
INTERFACE_INCLUDE_DIRECTORIES "${EXPAT_INCLUDE_DIRS}")
if(EXPAT_USE_STATIC_LIBS)
set_property(TARGET EXPAT::EXPAT APPEND PROPERTY
INTERFACE_COMPILE_DEFINITIONS "XML_STATIC")
endif()
if(EXPAT_LIBRARY_RELEASE)
set_property(TARGET EXPAT::EXPAT APPEND PROPERTY
IMPORTED_CONFIGURATIONS RELEASE)
set_target_properties(EXPAT::EXPAT PROPERTIES
IMPORTED_LOCATION_RELEASE "${EXPAT_LIBRARY_RELEASE}")
endif()
if(EXPAT_LIBRARY_DEBUG)
set_property(TARGET EXPAT::EXPAT APPEND PROPERTY
IMPORTED_CONFIGURATIONS DEBUG)
set_target_properties(EXPAT::EXPAT PROPERTIES
IMPORTED_LOCATION_DEBUG "${EXPAT_LIBRARY_DEBUG}")
endif()
if(NOT EXPAT_LIBRARY_RELEASE AND NOT EXPAT_LIBRARY_DEBUG)
set_property(TARGET EXPAT::EXPAT APPEND PROPERTY
IMPORTED_LOCATION "${EXPAT_LIBRARY}")
endif()
endif()
endif()
mark_as_advanced(EXPAT_INCLUDE_DIR EXPAT_LIBRARY)
Re: Building with CMake on Linux
Posted by Daniel Sahlberg <da...@gmail.com>.
Den sön 21 juli 2024 kl 18:29 skrev Timofei Zhakov <ti...@chemodax.net>:
> >> > (snipping most content)
> Hi,
>
> I made the most fixes suggested from this thread. Now the project
> compiles correctly on Linux and also the tests are passing (except one
> which doesn't work due to different build layouts and the Python tests
> are not yet running). It still needs some testing and improvement. I'd
> appreciate any feedback.
>
> Additionally, I tried to compile with CMake on Mac OS and it worked
> correctly without any modifications!! I was so excited that it
> succeeded!
>
I can add FreeBSD as "working without modifications".
I also tried OpenBSD but failed on missing Expat ("Could not find a package
configuration file provided by "expat" with any of the following names:
expatConfig.cmake expat-config.cmake [....]". I could not figure out if
this is a missing package in OpenBSD or that Expat on OpenBSD doesn't
provide the required cmake file. I hope @Stefan Sperling <st...@apache.org>
can chime in on this issue!
Finally, after more exploring the build system on Unix platforms and
> trying the CMake on them, I thought that the CMake build system could
> be also recommended for Unix (of course when it is ready for this).
>
> The main reason for this is that we will only need to support a single
> build system across all possible platforms. This means we will only
> have to implement features and utilities, such as dependency
> searching, once, while ensuring cross-platform support. For example,
> this approach could prevent scenarios where a feature is implemented
> for Linux, but then requires fixes for the Windows build system.
>
> Additionally, nowadays CMake is considered more modern than autoconf,
> offering numerous advanced features for developers, packagers, and
> users. It could sometimes work faster and be more configurable than
> autoconf. However, this is just my personal opinion, and some
> individuals may still prefer autoconf instead, so...
>
> What do you think?
>
I have already expressed my opinion in previous e-mails, but Timofei
articulate it much better than I could ever do. I agree with his analysis
above and give it my support. I previously suggested to have CMake as an
"alternative" build system for 1.15 and make it the default build system
for 1.16.
Kind regards,
Daniel
Re: Building with CMake on Linux
Posted by Timofei Zhakov <ti...@chemodax.net>.
>> > (snipping most content)
>> > > When I was was doing that, I solved the problem with uname with a
>> > > little patch by adding ifdef over release_name_from_uname() invocation
>> > > in sysinfo.c:
>> > >
>> > > [[[
>> > > Index: subversion/libsvn_subr/sysinfo.c
>> > > ===================================================================
>> > > --- subversion/libsvn_subr/sysinfo.c (revision 1919186)
>> > > +++ subversion/libsvn_subr/sysinfo.c (working copy)
>> > > @@ -646,8 +646,12 @@
>> > > static const char *
>> > > linux_release_name(apr_pool_t *pool)
>> > > {
>> > > - const char *uname_release = release_name_from_uname(pool);
>> > > + const char *uname_release;
>> > >
>> > > +#if HAVE_UNAME
>> > > + uname_release = release_name_from_uname(pool);
>> > > +#endif
>> > (snipping the rest...)
>> >
>> > If HAVE_UNAME is not defined, then uname_release will point to
>> > undefined address? I think we need some default string. Perhaps
>> > "unknown"?
>>
>> Hi,
>>
>> Yes it will. However, as I understand the implementation of
>> release_name_from_uname(), it returns NULL if the uname cannot be
>> recognized (see sysinfo.c:312).
>>
>> Additionally, the svn_sysinfo__release_name function (which will
>> return that NULL) could do that, because (sysinfo.h:41):
>>
>> [[[
>> Return the release name (i.e., marketing name) of the running
>> system, or NULL if it's not available.
>>
>> All allocations are done in POOL.
>> ]]]
>>
>> However, "unknown" might be better than NULL...
>>
> Actually I just looked at the code (I probably should have looked before my "unknown" reply!!) and if uname_release is NULL then near the end of the function we return release_name instead. If we assign "unknown" to uname_release then we will mess up this logic.
>
> Therefore IMO it would be a more correct fix to wrap most of the body of release_name_from_uname() in #if HAVE_UNAME so that that function will return NULL if uname() is not available, then leave the call site function as is.
Hi,
I made the most fixes suggested from this thread. Now the project
compiles correctly on Linux and also the tests are passing (except one
which doesn't work due to different build layouts and the Python tests
are not yet running). It still needs some testing and improvement. I'd
appreciate any feedback.
Additionally, I tried to compile with CMake on Mac OS and it worked
correctly without any modifications!! I was so excited that it
succeeded!
Regarding the "unknown" problem: I checked what the svn --version
shows when there is no uname; it will be "unknown-unknown-unknown" if
the uname is NULL. I tried to implement the #if HAVE_UNAME check
inside the body, but other similar checks are done similarly to the
code I suggested, for example, the svn_sysinfo__canonical_host
function. Additionally, I svn-diff'd the svn --version commands from
Subversion installed from apt-get and compiled with CMake -- they
are almost the same (excepting minor differences between different
versions).
(Little fix to the code I initially suggested: I forgot to add
initialization of uname_release to NULL, so it should be there, I
forgot to add it):
Finally, after more exploring the build system on Unix platforms and
trying the CMake on them, I thought that the CMake build system could
be also recommended for Unix (of course when it is ready for this).
The main reason for this is that we will only need to support a single
build system across all possible platforms. This means we will only
have to implement features and utilities, such as dependency
searching, once, while ensuring cross-platform support. For example,
this approach could prevent scenarios where a feature is implemented
for Linux, but then requires fixes for the Windows build system.
Additionally, nowadays CMake is considered more modern than autoconf,
offering numerous advanced features for developers, packagers, and
users. It could sometimes work faster and be more configurable than
autoconf. However, this is just my personal opinion, and some
individuals may still prefer autoconf instead, so...
What do you think?
Thanks!
--
Timofei Zhakov
Re: Building with CMake on Linux
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Jul 17, 2024 at 2:35 PM Timofei Zhakov <ti...@chemodax.net> wrote:
> On Wed, Jul 17, 2024 at 8:21 PM Nathan Hartman <ha...@gmail.com>
> wrote:
> >
> > On Wed, Jul 17, 2024 at 10:31 AM Timofei Zhakov <ti...@chemodax.net>
> wrote:
> > (snipping most content)
> > > When I was was doing that, I solved the problem with uname with a
> > > little patch by adding ifdef over release_name_from_uname() invocation
> > > in sysinfo.c:
> > >
> > > [[[
> > > Index: subversion/libsvn_subr/sysinfo.c
> > > ===================================================================
> > > --- subversion/libsvn_subr/sysinfo.c (revision 1919186)
> > > +++ subversion/libsvn_subr/sysinfo.c (working copy)
> > > @@ -646,8 +646,12 @@
> > > static const char *
> > > linux_release_name(apr_pool_t *pool)
> > > {
> > > - const char *uname_release = release_name_from_uname(pool);
> > > + const char *uname_release;
> > >
> > > +#if HAVE_UNAME
> > > + uname_release = release_name_from_uname(pool);
> > > +#endif
> > (snipping the rest...)
> >
> > If HAVE_UNAME is not defined, then uname_release will point to
> > undefined address? I think we need some default string. Perhaps
> > "unknown"?
>
> Hi,
>
> Yes it will. However, as I understand the implementation of
> release_name_from_uname(), it returns NULL if the uname cannot be
> recognized (see sysinfo.c:312).
>
> Additionally, the svn_sysinfo__release_name function (which will
> return that NULL) could do that, because (sysinfo.h:41):
>
> [[[
> Return the release name (i.e., marketing name) of the running
> system, or NULL if it's not available.
>
> All allocations are done in POOL.
> ]]]
>
> However, "unknown" might be better than NULL...
>
> --
> Timofei Zhakov
Actually I just looked at the code (I probably should have looked before my
"unknown" reply!!) and if uname_release is NULL then near the end of the
function we return release_name instead. If we assign "unknown" to
uname_release then we will mess up this logic.
Therefore IMO it would be a more correct fix to wrap most of the body
of release_name_from_uname()
in #if HAVE_UNAME so that that function will return NULL if uname() is not
available, then leave the call site function as is.
Cheers,
Nathan
Re: Building with CMake on Linux
Posted by Daniel Sahlberg <da...@gmail.com>.
Den ons 17 juli 2024 kl 20:35 skrev Timofei Zhakov <ti...@chemodax.net>:
> On Wed, Jul 17, 2024 at 8:21 PM Nathan Hartman <ha...@gmail.com>
> wrote:
> >
> > On Wed, Jul 17, 2024 at 10:31 AM Timofei Zhakov <ti...@chemodax.net>
> wrote:
> > (snipping most content)
> > > When I was was doing that, I solved the problem with uname with a
> > > little patch by adding ifdef over release_name_from_uname() invocation
> > > in sysinfo.c:
> > >
> > > [[[
> > > Index: subversion/libsvn_subr/sysinfo.c
> > > ===================================================================
> > > --- subversion/libsvn_subr/sysinfo.c (revision 1919186)
> > > +++ subversion/libsvn_subr/sysinfo.c (working copy)
> > > @@ -646,8 +646,12 @@
> > > static const char *
> > > linux_release_name(apr_pool_t *pool)
> > > {
> > > - const char *uname_release = release_name_from_uname(pool);
> > > + const char *uname_release;
> > >
> > > +#if HAVE_UNAME
> > > + uname_release = release_name_from_uname(pool);
> > > +#endif
> > (snipping the rest...)
> >
> > If HAVE_UNAME is not defined, then uname_release will point to
> > undefined address? I think we need some default string. Perhaps
> > "unknown"?
>
> Hi,
>
> Yes it will. However, as I understand the implementation of
> release_name_from_uname(), it returns NULL if the uname cannot be
> recognized (see sysinfo.c:312).
>
> Additionally, the svn_sysinfo__release_name function (which will
> return that NULL) could do that, because (sysinfo.h:41):
>
> [[[
> Return the release name (i.e., marketing name) of the running
> system, or NULL if it's not available.
>
> All allocations are done in POOL.
> ]]]
>
> However, "unknown" might be better than NULL...
>
All this is protected by an outer #ifdef __linux__ and I think all
Linux:es should have uname(). This came up because the cmake build didn't
check / define HAVE_UNAME. Now it does so I don't think we should run into
it.7
But if we want to support __linux__ without HAVE_UNAME, the patch above
looks good to me, with the addition to initialize uname_release to NULL.
Kind regards,
Daniel
Re: Building with CMake on Linux
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Wed, Jul 17, 2024 at 8:21 PM Nathan Hartman <ha...@gmail.com> wrote:
>
> On Wed, Jul 17, 2024 at 10:31 AM Timofei Zhakov <ti...@chemodax.net> wrote:
> (snipping most content)
> > When I was was doing that, I solved the problem with uname with a
> > little patch by adding ifdef over release_name_from_uname() invocation
> > in sysinfo.c:
> >
> > [[[
> > Index: subversion/libsvn_subr/sysinfo.c
> > ===================================================================
> > --- subversion/libsvn_subr/sysinfo.c (revision 1919186)
> > +++ subversion/libsvn_subr/sysinfo.c (working copy)
> > @@ -646,8 +646,12 @@
> > static const char *
> > linux_release_name(apr_pool_t *pool)
> > {
> > - const char *uname_release = release_name_from_uname(pool);
> > + const char *uname_release;
> >
> > +#if HAVE_UNAME
> > + uname_release = release_name_from_uname(pool);
> > +#endif
> (snipping the rest...)
>
> If HAVE_UNAME is not defined, then uname_release will point to
> undefined address? I think we need some default string. Perhaps
> "unknown"?
Hi,
Yes it will. However, as I understand the implementation of
release_name_from_uname(), it returns NULL if the uname cannot be
recognized (see sysinfo.c:312).
Additionally, the svn_sysinfo__release_name function (which will
return that NULL) could do that, because (sysinfo.h:41):
[[[
Return the release name (i.e., marketing name) of the running
system, or NULL if it's not available.
All allocations are done in POOL.
]]]
However, "unknown" might be better than NULL...
--
Timofei Zhakov
Re: Building with CMake on Linux
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Jul 17, 2024 at 10:31 AM Timofei Zhakov <ti...@chemodax.net> wrote:
(snipping most content)
> When I was was doing that, I solved the problem with uname with a
> little patch by adding ifdef over release_name_from_uname() invocation
> in sysinfo.c:
>
> [[[
> Index: subversion/libsvn_subr/sysinfo.c
> ===================================================================
> --- subversion/libsvn_subr/sysinfo.c (revision 1919186)
> +++ subversion/libsvn_subr/sysinfo.c (working copy)
> @@ -646,8 +646,12 @@
> static const char *
> linux_release_name(apr_pool_t *pool)
> {
> - const char *uname_release = release_name_from_uname(pool);
> + const char *uname_release;
>
> +#if HAVE_UNAME
> + uname_release = release_name_from_uname(pool);
> +#endif
(snipping the rest...)
If HAVE_UNAME is not defined, then uname_release will point to
undefined address? I think we need some default string. Perhaps
"unknown"?
Cheers,
Nathan
Re: Building with CMake on Linux
Posted by Daniel Sahlberg <da...@gmail.com>.
Den ons 17 juli 2024 kl 16:31 skrev Timofei Zhakov <ti...@chemodax.net>:
> On Tue, Jul 16, 2024 at 5:41 PM Daniel Sahlberg
> <da...@gmail.com> wrote:
> >
> > Hi,
> >
> > Inspired by Timofei's work on the cmake branch, I checked it out and
> tried to build on Linux.
> >
> > It worked, almost.
> >
> > [[[
> > [ 17%] Building C object
> CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/sysinfo.c.o
> > /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c: In function
> ‘linux_release_name’:
> > /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c:649:31: warning:
> implicit declaration of function ‘release_name_from_uname’
> [-Wimplicit-function-declaration]
> > 649 | const char *uname_release = release_name_from_uname(pool);
> > | ^~~~~~~~~~~~~~~~~~~~~~~
> > /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c:649:31: warning:
> initialization of ‘const char *’ from ‘int’ makes pointer from integer
> without a cast [-Wint-conversion]
> > /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c: In function
> ‘linux_shared_libs’:
> > /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c:750:65: warning:
> implicit declaration of function ‘getpid’ [-Wimplicit-function-declaration]
> > 750 | const char *maps = apr_psprintf(pool, "/proc/%ld/maps",
> (long)getpid());
> > |
> ^~~~~~
> > ]]]
> >
> > The above warnings were caused by missing defines HAVE_SYS_UTSNAME_H,
> HAVE_UNAME and HAVE_UNISTD_H.
> >
> > I also got the following error:
> > [[[
> > [ 64%] Linking C shared library liblibsvn_ra-1.so
> > /usr/bin/ld: liblibsvn_ra_svn.a(marshal.c.o): warning: relocation
> against `svn_ctype_table' in read-only section `.text'
> > /usr/bin/ld: liblibsvn_ra_svn.a(client.c.o): relocation R_X86_64_PC32
> against symbol `svn_ctype_table' can not be used when making a shared
> object; recompile with -fPIC
> > /usr/bin/ld: final link failed: bad value
> > collect2: error: ld returned 1 exit status
> > gmake[2]: *** [CMakeFiles/libsvn_ra.dir/build.make:186:
> liblibsvn_ra-1.so] Error 1
> > gmake[1]: *** [CMakeFiles/Makefile2:330: CMakeFiles/libsvn_ra.dir/all]
> Error 2
> > gmake: *** [Makefile:136: all] Error 2
> > ]]]
> >
> > I made some effort trying to figure out the proper way to fix this and I
> came up with the following changes. After this the build succeeds and I get
> a working svn binary.
> >
> > I'm SURE this doesn't follow CMake best-practice but I'd be happy if it
> could be a starting point in getting CMake to work under Linux. I'd also be
> happy to see how it SHOULD be done :-)
> >
> > To get this to work under Macos, someone should look at the
> SVN_HAVE_MACOS_PLIST define.
> >
> > [[[
> > Index: CMakeLists.txt
> > ===================================================================
> > --- CMakeLists.txt (revision 1919275)
> > +++ CMakeLists.txt (working copy)
> > @@ -125,8 +125,29 @@
> > enable_testing()
> > endif()
> >
> > +include(CheckSymbolExists)
> > +include(CheckIncludeFiles)
> > +check_include_files("sys/utsname.h" HAVE_SYS_UTSNAME_H)
> > +if (HAVE_SYS_UTSNAME_H)
> > + add_compile_definitions(HAVE_SYS_UTSNAME_H)
> > +
> > + check_symbol_exists(uname sys/utsname.h HAVE_UNAME)
> > + if (HAVE_UNAME)
> > + add_compile_definitions(HAVE_UNAME)
> > + endif()
> > +endif()
> > +check_include_files("unistd.h" HAVE_UNISTD_H)
> > +if (HAVE_UNISTD_H)
> > + add_compile_definitions(HAVE_UNISTD_H)
> > +endif()
> > +
> > +if (BUILD_SHARED_LIBS)
> > + set(CMAKE_POSITION_INDEPENDENT_CODE ON)
> > +endif()
> > +
> > if(SVN_BUILD_SHARED_FS)
> > set(SVN_FS_BUILD_TYPE SHARED)
> > + set(CMAKE_POSITION_INDEPENDENT_CODE ON)
> > else()
> > set(SVN_FS_BUILD_TYPE STATIC)
> > endif()
> > @@ -133,6 +154,7 @@
> >
> > if(SVN_BUILD_SHARED_RA)
> > set(SVN_RA_BUILD_TYPE SHARED)
> > + set(CMAKE_POSITION_INDEPENDENT_CODE ON)
> > else()
> > set(SVN_RA_BUILD_TYPE STATIC)
> > endif()
> > ]]]
>
> Hi,
>
> Thanks for your attention to the branch and trying to build with
> CMake. I appreciate it.
>
> I was also trying it on Linux and got the same problems, but didn't
> find a nice resolution.
>
> When I was was doing that, I solved the problem with uname with a
> little patch by adding ifdef over release_name_from_uname() invocation
> in sysinfo.c:
>
> [[[
> Index: subversion/libsvn_subr/sysinfo.c
> ===================================================================
> --- subversion/libsvn_subr/sysinfo.c (revision 1919186)
> +++ subversion/libsvn_subr/sysinfo.c (working copy)
> @@ -646,8 +646,12 @@
> static const char *
> linux_release_name(apr_pool_t *pool)
> {
> - const char *uname_release = release_name_from_uname(pool);
> + const char *uname_release;
>
> +#if HAVE_UNAME
> + uname_release = release_name_from_uname(pool);
> +#endif
> +
> /* Try anything that has /usr/bin/lsb_release.
> Covers, for example, Debian, Ubuntu and SuSE. */
> const char *release_name = lsb_release(pool);
> ]]]
>
> This fixed the problem, but the resolution is not nice, because the
> version in svn --version becomes weird. I think I will look at the
> checks using CheckIncludeFiles that you suggested, but they should be
> added for every HAVE_* or SVN_HAVE_* constants, so...
>
Yup, that would fix the problem but change behaviour between CMake and
autconf builds - I don't think that is acceptable.
As you say, all the HAVE_* and SVN_HAVE_* currently set by ./configure
should also be checked and set by CMake.
>
> The problem with svn_ctype_table in that time I fixed by simply
> disabling ra-svn or disabling shared libraries, also adding -fpic to
> compile options resolved the problem. However, I didn't notice the
> CMAKE_POSITION_INDEPENDENT_CODE option and I think I will utilize it.
>
*thumbs up*
>
> Additionally, there are two other problems that I noticed when trying
> to build on Linux and didn't fix yet:
>
> 1. The libraries compiled to the files with double 'lib' prefix and
> there might be files like liblibsvn_subr.so. I'm going to fix it by
> making some modifications in the CMAKE_SHARED_LIBRARY_PREFIX variable
> and also get libraries without the 'lib' prefix in static
> configuration.
>
Oh, yeah, that should be fixed. set(CMAKE_SHARED_LIBRARY_PREFIX "") seems
to do the trick for me but I don't know if there are any downsides.
For the static libraries: set(CMAKE_STATIC_LIBRARY_PREFIX "")
>
> 2. Path separator (SVN_PATH_LOCAL_SEPARATOR) is not currently
> configured correctly, so there might be some runtime problems when
> converting the path from local style and to local style.
>
I saw that one when running the tests. How about:
[[[
Index: subversion/svn_private_config.hc
===================================================================
--- subversion/svn_private_config.hc (revision 1919275)
+++ subversion/svn_private_config.hc (working copy)
@@ -39,7 +39,11 @@
#define SVN_FS_WANT_DB_PATCH 14
/* Path separator for local filesystem */
+#if defined(_WIN32) || defined(_WIN64)
#define SVN_PATH_LOCAL_SEPARATOR '\\'
+#else
+#define SVN_PATH_LOCAL_SEPARATOR '/'
+#endif
/* Name of system's null device */
#define SVN_NULL_DEVICE_NAME "nul"
]]]
(Thinking of it, it should probably be enough to test for _WIN32)
> By the way, I think that Linux support of the CMake is not very
> important due to the great and useful current build system which is
> quite useful. I think it should work, but the basic target should be
> Windows or unusual platforms that autoconf doesn't have a great
> support for. However, as it is requested, I will try to improve
> support for non-Windows platforms as well...
>
Well... the autoconf build system is good and well tested but I still
believe it would be good to have one single build system for all platforms,
at least in the long run. It simplifies instructions (see the new III.F
chapter in INSTALL - we'd "only" have to provide instructions on how to get
dependencies per platform) and less dividing efforts to multiple code
bases. Maybe this discussion should be held in my thread about INSTALL,
where I floated the idea of a new unified build system.
Kind regards,
Daniel
Re: Building with CMake on Linux
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Tue, Jul 16, 2024 at 5:41 PM Daniel Sahlberg
<da...@gmail.com> wrote:
>
> Hi,
>
> Inspired by Timofei's work on the cmake branch, I checked it out and tried to build on Linux.
>
> It worked, almost.
>
> [[[
> [ 17%] Building C object CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/sysinfo.c.o
> /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c: In function ‘linux_release_name’:
> /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c:649:31: warning: implicit declaration of function ‘release_name_from_uname’ [-Wimplicit-function-declaration]
> 649 | const char *uname_release = release_name_from_uname(pool);
> | ^~~~~~~~~~~~~~~~~~~~~~~
> /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c:649:31: warning: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
> /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c: In function ‘linux_shared_libs’:
> /home/dsg/svn_cmake/subversion/libsvn_subr/sysinfo.c:750:65: warning: implicit declaration of function ‘getpid’ [-Wimplicit-function-declaration]
> 750 | const char *maps = apr_psprintf(pool, "/proc/%ld/maps", (long)getpid());
> | ^~~~~~
> ]]]
>
> The above warnings were caused by missing defines HAVE_SYS_UTSNAME_H, HAVE_UNAME and HAVE_UNISTD_H.
>
> I also got the following error:
> [[[
> [ 64%] Linking C shared library liblibsvn_ra-1.so
> /usr/bin/ld: liblibsvn_ra_svn.a(marshal.c.o): warning: relocation against `svn_ctype_table' in read-only section `.text'
> /usr/bin/ld: liblibsvn_ra_svn.a(client.c.o): relocation R_X86_64_PC32 against symbol `svn_ctype_table' can not be used when making a shared object; recompile with -fPIC
> /usr/bin/ld: final link failed: bad value
> collect2: error: ld returned 1 exit status
> gmake[2]: *** [CMakeFiles/libsvn_ra.dir/build.make:186: liblibsvn_ra-1.so] Error 1
> gmake[1]: *** [CMakeFiles/Makefile2:330: CMakeFiles/libsvn_ra.dir/all] Error 2
> gmake: *** [Makefile:136: all] Error 2
> ]]]
>
> I made some effort trying to figure out the proper way to fix this and I came up with the following changes. After this the build succeeds and I get a working svn binary.
>
> I'm SURE this doesn't follow CMake best-practice but I'd be happy if it could be a starting point in getting CMake to work under Linux. I'd also be happy to see how it SHOULD be done :-)
>
> To get this to work under Macos, someone should look at the SVN_HAVE_MACOS_PLIST define.
>
> [[[
> Index: CMakeLists.txt
> ===================================================================
> --- CMakeLists.txt (revision 1919275)
> +++ CMakeLists.txt (working copy)
> @@ -125,8 +125,29 @@
> enable_testing()
> endif()
>
> +include(CheckSymbolExists)
> +include(CheckIncludeFiles)
> +check_include_files("sys/utsname.h" HAVE_SYS_UTSNAME_H)
> +if (HAVE_SYS_UTSNAME_H)
> + add_compile_definitions(HAVE_SYS_UTSNAME_H)
> +
> + check_symbol_exists(uname sys/utsname.h HAVE_UNAME)
> + if (HAVE_UNAME)
> + add_compile_definitions(HAVE_UNAME)
> + endif()
> +endif()
> +check_include_files("unistd.h" HAVE_UNISTD_H)
> +if (HAVE_UNISTD_H)
> + add_compile_definitions(HAVE_UNISTD_H)
> +endif()
> +
> +if (BUILD_SHARED_LIBS)
> + set(CMAKE_POSITION_INDEPENDENT_CODE ON)
> +endif()
> +
> if(SVN_BUILD_SHARED_FS)
> set(SVN_FS_BUILD_TYPE SHARED)
> + set(CMAKE_POSITION_INDEPENDENT_CODE ON)
> else()
> set(SVN_FS_BUILD_TYPE STATIC)
> endif()
> @@ -133,6 +154,7 @@
>
> if(SVN_BUILD_SHARED_RA)
> set(SVN_RA_BUILD_TYPE SHARED)
> + set(CMAKE_POSITION_INDEPENDENT_CODE ON)
> else()
> set(SVN_RA_BUILD_TYPE STATIC)
> endif()
> ]]]
Hi,
Thanks for your attention to the branch and trying to build with
CMake. I appreciate it.
I was also trying it on Linux and got the same problems, but didn't
find a nice resolution.
When I was was doing that, I solved the problem with uname with a
little patch by adding ifdef over release_name_from_uname() invocation
in sysinfo.c:
[[[
Index: subversion/libsvn_subr/sysinfo.c
===================================================================
--- subversion/libsvn_subr/sysinfo.c (revision 1919186)
+++ subversion/libsvn_subr/sysinfo.c (working copy)
@@ -646,8 +646,12 @@
static const char *
linux_release_name(apr_pool_t *pool)
{
- const char *uname_release = release_name_from_uname(pool);
+ const char *uname_release;
+#if HAVE_UNAME
+ uname_release = release_name_from_uname(pool);
+#endif
+
/* Try anything that has /usr/bin/lsb_release.
Covers, for example, Debian, Ubuntu and SuSE. */
const char *release_name = lsb_release(pool);
]]]
This fixed the problem, but the resolution is not nice, because the
version in svn --version becomes weird. I think I will look at the
checks using CheckIncludeFiles that you suggested, but they should be
added for every HAVE_* or SVN_HAVE_* constants, so...
The problem with svn_ctype_table in that time I fixed by simply
disabling ra-svn or disabling shared libraries, also adding -fpic to
compile options resolved the problem. However, I didn't notice the
CMAKE_POSITION_INDEPENDENT_CODE option and I think I will utilize it.
Additionally, there are two other problems that I noticed when trying
to build on Linux and didn't fix yet:
1. The libraries compiled to the files with double 'lib' prefix and
there might be files like liblibsvn_subr.so. I'm going to fix it by
making some modifications in the CMAKE_SHARED_LIBRARY_PREFIX variable
and also get libraries without the 'lib' prefix in static
configuration.
2. Path separator (SVN_PATH_LOCAL_SEPARATOR) is not currently
configured correctly, so there might be some runtime problems when
converting the path from local style and to local style.
By the way, I think that Linux support of the CMake is not very
important due to the great and useful current build system which is
quite useful. I think it should work, but the basic target should be
Windows or unusual platforms that autoconf doesn't have a great
support for. However, as it is requested, I will try to improve
support for non-Windows platforms as well...
--
Timofei Zhakov