Replace ant build with cmake #14
Conversation
|
I ❤️ PRs that have huge negative line counts like this! I'm not super familiar with the tooling in use, though I'll try this out over the weekend. |
There was a problem hiding this comment.
I tried this out on macOS. After installing cmake 3.16.3, I was able to use mkdir build && cd build && cmake .. && make && make install and it seemed to build and install properly. Only tested using Makefiles so far, though it looks like Xcode is also supported in theory?
| $ cd apache-log4cxx-x.x.x | ||
| $ mkdir build | ||
| $ cd build | ||
| $ ccmake .. |
There was a problem hiding this comment.
Missing cmake .. step? I'm not very familiar with the tool, but ccmake seems to be the GUI to configure some options?
There was a problem hiding this comment.
Do you think I should duplicate the Windows instructions for building from source into a unix/macOS section (just need to change \ to / actually)?
There was a problem hiding this comment.
If there's any differences in the commands, go ahead.
There was a problem hiding this comment.
Unfortunately, the Windows technique will not work on unix/macOS as APR and APR-Util CMakeLists.txt file are Windows-only implementations.
|
|
||
| ** Mac OS/X: | ||
|
|
||
| APR and APR-Util are provided by the platform in Mac OS/X 10.5 and iODBC in 10.4. |
There was a problem hiding this comment.
This might be obvious to you, but cmake does not come with macOS by default. It might help users to add the cmake dependencies explicit in the install commands given here for various operating systems.
There was a problem hiding this comment.
Could you tell what to add as I do not have macOS
There was a problem hiding this comment.
The most common installer is via brew install cmake using homebrew or it can always be downloaded from their binaries page.
…phen-webb/logging-log4cxx into replace-ant-build-with-cmake
|
Hi All,
I have done everything I can think of prepare
#14 for inclusion in the
repository.
Please review.
…On Sat, Feb 8, 2020 at 3:33 AM Matt Sicker ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/site/apt/building/cmake.apt
<#14 (comment)>:
> + ------
+ ------
+
+Building Apache log4cxx with cmake
+
+* Quick start:
+
+ Make sure cmake 3.13+, g++ and make are available, install or
+ build apr 1.x, apr-util 1.x, gzip and zip.
+
++------------+
+$ apt-get install build-essential libapr1-dev libaprutil1-dev gzip zip
+$ cd apache-log4cxx-x.x.x
+$ mkdir build
+$ cd build
+$ ccmake ..
If there's any differences in the commands, go ahead.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14?email_source=notifications&email_token=ANLOSDVTHN7UFW2RM7WPM7LRBWENXA5CNFSM4KOCPPYKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUWSZOA#discussion_r376487230>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANLOSDQ522H2MTOKR75YCW3RBWENXANCNFSM4KOCPPYA>
.
|
|
So let's close #12 and really focus on this one, replacing things in favour of CMAKE instead of adding it? I don't use the ANT-build myself anyway, so would focus on that site generation etc. still works. It's just that things might break for other users. |
| @@ -478,4 +477,4 @@ LOGUNIT_CLASS(SocketServerTestCase) | |||
| const File SocketServerTestCase::TEMP("output/temp"); | |||
| const File SocketServerTestCase::FILTERED("output/filtered"); | |||
|
|
|||
| LOGUNIT_TEST_SUITE_REGISTRATION_DISABLED(SocketServerTestCase) | |||
| LOGUNIT_TEST_SUITE_REGISTRATION(SocketServerTestCase) | |||
There was a problem hiding this comment.
Should this test really be enabled? While your CMAKE-build might handle things automatically, it might be more difficult for users without CMAKE. Took me a while to figure things out based on this commit myself and the test still fails currently...
set PATH=C:\Program Files\Java\jdk-8\bin;C:\Program Files (x86)\UnxUtils\usr\local\wbin;%PATH%
set TEST_PROJ=%cd%
set LOG4CXX_VER_BASE=..\..\..\..
set TEST_SRC=%LOG4CXX_VER_BASE%\src\src\test
set TEST_RES=%TEST_SRC%\resources
set TOTO=wonderful
set key1=value
set key2=value2
set log4j_CLASSPATH=%LOG4CXX_VER_BASE%\..\..\..\..\Java\log4j-1.2.17.jar
set SOCKET_SERVER_SOURCES=%TEST_SRC%\java\org\apache\log4j\net\ShortSocketServer.java
set SOCKET_SERVER_CLASSPATH=%TEST_RES%;%log4j_CLASSPATH%
set CLASSPATH=%SOCKET_SERVER_CLASSPATH%
set SOCKET_SERVER_COMMAND=C:\Program Files\Java\jdk-8\bin\java.exe;org.apache.log4j.net.ShortSocketServer;8;input/socketServer
pushd "%TEST_RES%"
javac -d "." -classpath "%log4j_CLASSPATH%" "%SOCKET_SERVER_SOURCES%"
"%TEST_PROJ%\Win32\Debug\out\%~n0.exe" %*
popd
goto :EOF
There was a problem hiding this comment.
Disabling a testcase is needed when building multiple tests into a single executable. To run a disabled test case you then need to add it as a parameter.
The socketservertestcase excutable has only one test case so I do not think it is appropriate to disable it.
There was a problem hiding this comment.
The point is not about different executables, but that this one test case only requires additional Java and is difficult to setup. It has been disabled in the past for a long time already:
|
@stephen-webb Does I have no idea where that |
I think the log settings and witnesses were simply wrong, because the test has been disabled so long. I've additionally applied reformatting to the code, because non-tests have been reformatted in the past already and the tests were pretty inconsistent with indentation and stuff. Things look good to merge for me now. I won't have the time to test the CMAKE-stuff myself, but problems can be solved in master in future as well. Note to self: Try not to forget closing the following issue when this is merged. |
|
Using Targetting older versions of CMAKE has been asked before as well and 3.7 was the preferred target then as well. So, is it really necessary to use these current versions instead of older ones? Are the Maven plugin and the version of CMAKE bound at each other? Simply downgrading the MVN-plugin doesn't work: https://github.com/cmake-maven-project/cmake-maven-project/blame/release-3.7.2-b1/README.md#L30 |
|
The previous version of cmake.maven.plugin did not work on Windows.
The maintainer is active so may be receptive if you raise an issue.
…On Tue, Feb 11, 2020, 6:54 PM Thorsten Schöning ***@***.***> wrote:
Using com.googlecode.cmake-maven-project with OpenJDK 8 fails, because
they require JDK 11
<cmake-maven-project/cmake-maven-project@715e2c8>
for around a year now. Version 3.7.2
<https://github.com/cmake-maven-project/cmake-maven-project/blob/release-3.7.2-b1/.travis.yml>
seems to be the last building with JDK 8.
Targetting older versions of CMAKE has been asked before
<https://github.com/apache/logging-log4cxx/pull/14/files/af0134afaf2cd8d85d60aca3bae4b627d57b8c48#r373735482>
as well and 3.7 was the preferred target then as well.
So, is it really necessary to use these current versions instead of older
ones? Are the Maven plugin and the version of CMAKE bound at each other?
Simply downgrading the MVN-plugin doesn't work:
[ERROR] Failed to execute goal com.googlecode.cmake-maven-project:cmake-maven-plugin:3.7.2-b1:generate (cmake-generate) on project apache-log4cxx: The parameters 'generator' for goal com.googlecode.cmake-maven-project:cmake-maven-plugin:3.7.2-b1:generate are missing or invalid -> [Help 1]
https://github.com/cmake-maven-project/cmake-maven-project/blame/release-3.7.2-b1/README.md#L30
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14?email_source=notifications&email_token=ANLOSDXI2GWPYMAEIJXM5RDRCJKTVA5CNFSM4KOCPPYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELLPV2Y#issuecomment-584514283>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANLOSDWLMQTDOJUO6K6K4D3RCJKTVANCNFSM4KOCPPYA>
.
|
|
I'm trying to test CMAKE and MVN on Sunday myself, just out of interest to see what happens, but nevertheless, as long as nobody objects, going to merge So until Sunday, people should base their work on |
No description provided.