On 10/12/2012 02:47 AM, Joel Holdsworth wrote:
> Ok that looks good, but I think it needs splitting up a bit.
>
> For starters, your patch seems to remove quite a lot of code that isn't
> in the file to begin with?!? Is there another couple of patches on your
> branch that you need to submit as well?
Maybe there was a formatting issue.
>
> If you have time, here's what I'd like:
>
> * Please do add a header to CMakeLists.txt, and if you could add my
> details to it as well, I would appreciate it.
done
> * set(VERSION 0 1 0), would be very helpful. Is there a way we can hook
> that up to the About box?
It is already hooked in, but with a compile-time define. Not the
friendliest way. The best way that I see is to add a config.h file.
Patch 2 does just that.
> * Adding global sections would be helpful, but I'd prefer more concise
> captions. "Global Defines" and "Global Include Directories" is fine.
> "Back to our pulseview binaries **" is a bit ugly when you could just
> put "Binaries" or something like that. Also minor point: Is there a way
> you could delimit sections which is less heavy? That's an awful lot of
> "#" characters. Even using "-" would be lighter on they eye, and/or just
> one row, would look a lot less heavy.
Let's see. You might like this better.
Alex
> * Shortening the WIN32 stuff down: Yes! Absolutely!
>
> If you can do each of those bullet points as a single patch, that would
> be ideal.
>
> I don't know if anyone else has any comments, but thanks for
> contributing. It's really nice to have people joining in on this!
>
> Thanks
> Joel
>
> On 11 October 2012 at 23:41 "Alex G." <[email protected]> wrote:
>> Minor CMakeLists.txt cleanup
>>
>> First of all, specify the build directory as an include directory.
>> This enables us to build out of source, for example:
>> mkdir build
>> cd build
>> cmake ..
>>
>> Remove all the if(WIN32) clauses, and use an option to specify wheter we
>> want to link to the static versions of libsigrok*. That's basically what
>> the WIN32 clauses were doing anyway. This makes the "what is going on"
>> more readeable.
>> On WIN32, automatically set this option to link to the static libraries.
>>
>> Add a copyright header to CMakeLists.txt
>>
>> Signed-off-by: Alexandru Gagniuc <[email protected]>
>> ---
>>
> ------------------------------------------------------------------------------
>
>> Don't let slow site performance ruin your business. Deploy New Relic APM
>> Deploy New Relic app performance management and know exactly
>> what is happening inside your Ruby, Python, PHP, Java, and .NET app
>> Try New Relic at no cost today and get our sweet Data Nerd shirt too!
>>
> http://p.sf.net/sfu/newrelic-dev2dev_______________________________________________
>
>> sigrok-devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/sigrok-devel
>From bafe0e602b73a0006c25d332dadc443df0a6339e Mon Sep 17 00:00:00 2001
From: Alexandru Gagniuc <[email protected]>
Date: Thu, 11 Oct 2012 17:25:18 -0500
Subject: [PATCH 1/2] Minor CMakeLists.txt cleanup
First of all, specify the build directory as an include directory.
This enables us to build out of source, for example:
mkdir build
cd build
cmake ..
Remove all the if(WIN32) clauses, and use an option to specify whether
we want to link to the static versions of libsigrok*. That's basically
what the WIN32 clauses were doing anyway. This makes the "what is
going on" more readeable.
On WIN32, automatically set this option to link to the static libraries.
Add a copyright header to CMakeLists.txt
Signed-off-by: Alexandru Gagniuc <[email protected]>
---
CMakeLists.txt | 134 +++++++++++++++++++++++++++++++++------------------------
1 file changed, 77 insertions(+), 57 deletions(-)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index afbe0df..78642c3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,8 +1,37 @@
+##
+## This file is part of pulseview
+##
+## Copyright (C) 2012 Joel Holdsworth <[email protected]>
+## Copyright (C) 2012 Alexandru Gagniuc <[email protected]>
+##
+## This program is free software: you can redistribute it and/or modify
+## it under the terms of the GNU General Public License as published by
+## the Free Software Foundation, either version 2 of the License, or
+## (at your option) any later version.
+##
+## This program is distributed in the hope that it will be useful,
+## but WITHOUT ANY WARRANTY; without even the implied warranty of
+## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+## GNU General Public License for more details.
+##
+## You should have received a copy of the GNU General Public License
+## along with this program. If not, see <http://www.gnu.org/licenses/>.
+##
+
cmake_minimum_required(VERSION 2.6)
include(FindPkgConfig)
project(pulseview)
+#===============================================================================
+#= PulseView configuration and user-configurable options
+#-------------------------------------------------------------------------------
+set(VERSION 0.1.0)
+
+option(STATIC_PKGDEPS_LIBS "Statically link to sigrok libraries" OFF)
+#===============================================================================
+#= Dependencies tracking
+#-------------------------------------------------------------------------------
find_package(PkgConfig)
pkg_check_modules(PKGDEPS REQUIRED
libsigrokdecode>=0.1.0
@@ -11,14 +40,16 @@ pkg_check_modules(PKGDEPS REQUIRED
# On Windows/MinGW we explicitly point cmake to the Boost directory.
if(WIN32)
-set(BOOST_ROOT /usr/local)
-endif(WIN32)
+ # FIXME: Is BOOST always packaged in /usr/local on win32 ?
+ set(BOOST_ROOT /usr/local)
+endif()
find_package(Qt4 REQUIRED)
find_package(Boost 1.46 COMPONENTS unit_test_framework REQUIRED)
-set(VERSION 0.1.0)
-
+#===============================================================================
+#= Sources and resources
+#-------------------------------------------------------------------------------
set(pulseview_SOURCES
about.cpp
datasnapshot.cpp
@@ -69,79 +100,68 @@ qt4_add_resources(pulseview_RESOURCES_RCC ${pulseview_RESOURCES})
include(${QT_USE_FILE})
+#===============================================================================
+#= Global defines
+#-------------------------------------------------------------------------------
add_definitions(${QT_DEFINITIONS})
add_definitions(-DAPP_VERSION="${VERSION}")
+# TODO: Explain the reason for this define
+add_definitions(-DBOOST_TEST_DYN_LINK)
-# On Windows/MinGW we need PKGDEPS_STATIC_INCLUDE_DIRS.
-if(WIN32)
-include_directories(
- ${include_directories}
- ${Boost_INCLUDE_DIRS}
- ${PKGDEPS_STATIC_INCLUDE_DIRS}
-)
-else(WIN32)
-include_directories(
- ${include_directories}
- ${Boost_INCLUDE_DIRS}
- ${PKGDEPS_INCLUDE_DIRS}
-)
-endif(WIN32)
+#===============================================================================
+#= Global include directories
+#-------------------------------------------------------------------------------
+include_directories(${Boost_INCLUDE_DIRS})
+# We generate a few files, so we also need to look in the build directory when
+# building out of tree
+include_directories(${CMAKE_CURRENT_BINARY_DIR})
-# On Windows/MinGW we need PKGDEPS_STATIC_LIBRARY_DIRS.
-if(WIN32)
-link_directories(
- ${Boost_LIBRARY_DIRS}
- ${PKGDEPS_STATIC_LIBRARY_DIRS}
-)
-else(WIN32)
-link_directories(
- ${Boost_LIBRARY_DIRS}
- ${PKGDEPS_LIBRARY_DIRS}
+#===============================================================================
+#= Link libraries configuration
+#-------------------------------------------------------------------------------
+link_directories(${Boost_LIBRARY_DIRS})
+
+set(PULSEVIEW_LINK_LIBS
+ ${Boost_LIBRARIES}
+ ${QT_LIBRARIES}
)
-endif(WIN32)
+if(WIN32)
+ # On Windows/MinGW we need to statically link to libraries
+ # This option is user configurable, but enable it by default on win32
+ set(STATIC_PKGDEPS_LIBS ON)
+endif()
+
+if(STATIC_PKGDEPS_LIBS)
+ include_directories(${PKGDEPS_STATIC_INCLUDE_DIRS})
+ link_directories(${PKGDEPS_STATIC_LIBRARY_DIRS})
+ list(APPEND PULSEVIEW_LINK_LIBS ${PKGDEPS_STATIC_LIBRARIES})
+else()
+ include_directories(${PKGDEPS_INCLUDE_DIRS})
+ link_directories(${PKGDEPS_LIBRARY_DIRS})
+ list(APPEND PULSEVIEW_LINK_LIBS ${PKGDEPS_LIBRARIES})
+endif()
+
+#===============================================================================
+#= Binaries
+#-------------------------------------------------------------------------------
add_executable(pulseview
${pulseview_SOURCES}
${pulseview_HEADERS_MOC}
${pulseview_FORMS_HEADERS}
${pulseview_RESOURCES_RCC}
)
-
-# On Windows/MinGW we need PKGDEPS_STATIC_LIBRARIES.
-if(WIN32)
target_link_libraries(pulseview
- ${Boost_LIBRARIES}
- ${PKGDEPS_STATIC_LIBRARIES}
- ${QT_LIBRARIES}
-)
-else(WIN32)
-target_link_libraries(pulseview
- ${Boost_LIBRARIES}
- ${PKGDEPS_LIBRARIES}
- ${QT_LIBRARIES}
+ ${PULSEVIEW_LINK_LIBS}
)
-endif(WIN32)
-
-add_definitions(-DBOOST_TEST_DYN_LINK)
add_executable(pulseview-test
${pulseview_TEST_SOURCES}
)
-
-# On Windows/MinGW we need PKGDEPS_STATIC_LIBRARIES.
-if(WIN32)
-target_link_libraries(pulseview-test
- ${Boost_LIBRARIES}
- ${PKGDEPS_STATIC_LIBRARIES}
- ${QT_LIBRARIES}
-)
-else(WIN32)
target_link_libraries(pulseview-test
- ${Boost_LIBRARIES}
- ${PKGDEPS_LIBRARIES}
- ${QT_LIBRARIES}
+ ${PULSEVIEW_LINK_LIBS}
)
-endif(WIN32)
+# TODO: Explain what we use this testing for
enable_testing()
add_test(test ${CMAKE_CURRENT_BINARY_DIR}/pulseview-test)
--
1.7.11.7
>From aedc2a617cf3393d01ee55a514754f266641c5a0 Mon Sep 17 00:00:00 2001
From: Alexandru Gagniuc <[email protected]>
Date: Fri, 12 Oct 2012 03:35:39 -0500
Subject: [PATCH 2/2] Generate a config.h file for versioning
Versioning was already handled in CMakeLists.txt, but it was handled
in a non-standard and unclear manner. We used
set(VERSION
for starters. While this is valid CMake syntax, VERSION is also a
reserved word in cmake, depending on the context. We were
communicating the version information by a compile-time define, again,
not a recommended practice
Change versioning to the more standard way of
set(*VERSION_MAJOR
set(*VERSION_MINOR
set(*VERSION_MICRO
set(*VERSION_STRING
Instead of defining the version at compile time, generate a config.h
file with the version information.
Signed-off-by: Alexandru Gagniuc <[email protected]>
---
CMakeLists.txt | 13 +++++++++++--
config.h.in | 28 ++++++++++++++++++++++++++++
main.cpp | 3 ++-
3 files changed, 41 insertions(+), 3 deletions(-)
create mode 100644 config.h.in
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 78642c3..fa962f7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -25,7 +25,17 @@ project(pulseview)
#===============================================================================
#= PulseView configuration and user-configurable options
#-------------------------------------------------------------------------------
-set(VERSION 0.1.0)
+set(PV_VERSION_MAJOR 0)
+set(PV_VERSION_MINOR 1)
+set(PV_VERSION_MICRO 0)
+set(PV_VERSION_STRING
+ ${PV_VERSION_MAJOR}.${PV_VERSION_MINOR}.${PV_VERSION_MICRO}
+)
+
+configure_file (
+ ${PROJECT_SOURCE_DIR}/config.h.in
+ ${PROJECT_BINARY_DIR}/config.h
+)
option(STATIC_PKGDEPS_LIBS "Statically link to sigrok libraries" OFF)
@@ -104,7 +114,6 @@ include(${QT_USE_FILE})
#= Global defines
#-------------------------------------------------------------------------------
add_definitions(${QT_DEFINITIONS})
-add_definitions(-DAPP_VERSION="${VERSION}")
# TODO: Explain the reason for this define
add_definitions(-DBOOST_TEST_DYN_LINK)
diff --git a/config.h.in b/config.h.in
new file mode 100644
index 0000000..7efc9e3
--- /dev/null
+++ b/config.h.in
@@ -0,0 +1,28 @@
+/*
+ * This file is part of the sigrok project.
+ *
+ * Copyright (C) 2012 Alexandru Gagniuc <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef _PULSEVIEW_CONFIG_H
+#define _PULSEVIEW_CONFIG_H
+
+/* Pulseview version information */
+#define PV_VERSION_MAJOR @PV_VERSION_MAJOR@
+#define PV_VERSION_MINOR @PV_VERSION_MINOR@
+#define PV_VERSION_MICRO @PV_VERSION_MICRO@
+#define PV_VERSION_STRING "@PV_VERSION_STRING@"
+
+#endif /* _PULSEVIEW_CONFIG_H */
\ No newline at end of file
diff --git a/main.cpp b/main.cpp
index e63c87d..09ad719 100644
--- a/main.cpp
+++ b/main.cpp
@@ -27,13 +27,14 @@ extern "C" {
#include <QtGui/QApplication>
#include <QDebug>
#include "mainwindow.h"
+#include "config.h"
int main(int argc, char *argv[])
{
QApplication a(argc, argv);
/* Set some application metadata. */
- QApplication::setApplicationVersion(APP_VERSION);
+ QApplication::setApplicationVersion(PV_VERSION_STRING);
QApplication::setApplicationName("PulseView");
QApplication::setOrganizationDomain("http://www.sigrok.org";);
--
1.7.11.7
------------------------------------------------------------------------------
Don't let slow site performance ruin your business. Deploy New Relic APM
Deploy New Relic app performance management and know exactly
what is happening inside your Ruby, Python, PHP, Java, and .NET app
Try New Relic at no cost today and get our sweet Data Nerd shirt too!
http://p.sf.net/sfu/newrelic-dev2dev
_______________________________________________
sigrok-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
