TDE
/
ktechlab
6
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Labels Milestones

Conversion to the cmake building system. #2

Merged
SlavekB merged 5 commits from feat/cmakeConv into master 4 years ago
Conversation 10 Commits 5 Files Changed 75
Ghost commented 4 years ago

Guys, the sooner I get to know if the build tests over the gpsim version are corrects (for all supported Debian...or FreeBSD), the better. 😁

Guys, the sooner I get to know if the build tests over the gpsim version are corrects (for all supported Debian...or FreeBSD), the better. :grin:
Ghost added the
PR/wip
 label 4 years ago
Ghost commented 4 years ago
Poster

Rem:

-std=c++11 when building with gpsim-0.31 (if the compiler does not build c++11 turned on by default)

Rem: -std=c++11 when building with gpsim-0.31 (if the compiler does not build c++11 turned on by default)
MicheleC requested review from SlavekB 4 years ago
MicheleC commented 4 years ago
Owner

Guys, the sooner I get to know if the build tests over the gpsim version are corrects (for all supported Debian...or FreeBSD), the better. 😁

Looks good in bullseyes, but for all other distros we need Slavek's help 😄

> Guys, the sooner I get to know if the build tests over the gpsim version are corrects (for all supported Debian...or FreeBSD), the better. :grin: Looks good in bullseyes, but for all other distros we need Slavek's help :smile:
Ghost commented 4 years ago
Poster

@MicheleC, Hello Michele, if you've got some time for that (you seem awfully busy this week), can you send me over the resulting config.h file as well as making a short compile test?

@MicheleC, Hello Michele, if you've got some time for that (you seem awfully busy this week), can you send me over the resulting **config.h** file as well as making a short compile test?
MicheleC commented 4 years ago
Owner

Here is is (and yes, very busy for a few weeks more):

#define VERSION "R14.1.0"

// Defined if you have fvisibility and fvisibility-inlines-hidden support.
#define __KDE_HAVE_GCC_VISIBILITY 1

/* Define WORDS_BIGENDIAN to 1 if your processor stores words with the most
   significant byte first (like Motorola and SPARC, unlike Intel). */
/* #undef WORDS_BIGENDIAN */

/* #undef GPSIM_0_21_4 */
/* #undef GPSIM_0_21_11 */
#define GPSIM_0_21_12 1
#define GPSIM_0_27_0 1
#define GPSIM_0_31_0 1
/* #undef NO_GPSIM */
Here is is (and yes, very busy for a few weeks more): ``` #define VERSION "R14.1.0" // Defined if you have fvisibility and fvisibility-inlines-hidden support. #define __KDE_HAVE_GCC_VISIBILITY 1 /* Define WORDS_BIGENDIAN to 1 if your processor stores words with the most significant byte first (like Motorola and SPARC, unlike Intel). */ /* #undef WORDS_BIGENDIAN */ /* #undef GPSIM_0_21_4 */ /* #undef GPSIM_0_21_11 */ #define GPSIM_0_21_12 1 #define GPSIM_0_27_0 1 #define GPSIM_0_31_0 1 /* #undef NO_GPSIM */ ```
MicheleC commented 4 years ago
Owner

Looks good in bullseyes, but for all other distros we need Slavek's help 😄

I mistakenly build master branch instead of the cmake conversion :-(

EDIT: I get this error on bullseye

/usr/bin/ld: ktechlab: hidden symbol `_Z10exit_gpsimi' in core/libcore.a(main.cpp.o) is referenced by DSO
> Looks good in bullseyes, but for all other distros we need Slavek's help :smile: I mistakenly build master branch instead of the cmake conversion :-( EDIT: I get this error on bullseye ``` /usr/bin/ld: ktechlab: hidden symbol `_Z10exit_gpsimi' in core/libcore.a(main.cpp.o) is referenced by DSO ```
Ghost commented 4 years ago
Poster

EDIT: I get this error on bullseye

/usr/bin/ld: ktechlab: hidden symbol `_Z10exit_gpsimi' in core/libcore.a(main.cpp.o) is referenced by DSO

Symbols visibility should have been fixed.

> EDIT: I get this error on bullseye > ``` > /usr/bin/ld: ktechlab: hidden symbol `_Z10exit_gpsimi' in core/libcore.a(main.cpp.o) is referenced by DSO > ``` > Symbols visibility should have been fixed.
Ghost removed the
PR/wip
 label 4 years ago
Ghost changed title from WIP:Conversion to the cmake building system. to Conversion to the cmake building system. 4 years ago
Ghost commented 4 years ago
Poster

@SlavekB, I have removed the WIP status of this PR. I'd like you to test it against all the Debian distros you package, once this is done we shall see what to do with the gpsim-0.31 test build and c++11. 😐

@SlavekB, I have removed the WIP status of this PR. I'd like you to test it against all the Debian distros you package, once this is done we shall see what to do with the gpsim-0.31 test build and c++11. :neutral_face:
SlavekB reviewed 4 years ago
SlavekB left a comment
Owner

It looks good. However, there are a few comments.

It looks good. However, there are a few comments.
CMakeLists.txt Outdated
@ -0,0 +45,4 @@
option( WITH_ALL_OPTIONS "Enable all optional support" OFF )
option( WITH_GCC_VISIBILITY "Enable fvisibility and fvisibility-inlines-hidden" ${WITH_ALL_OPTIONS} )
option( WITH_GPSIM "Enable gpsim support" ${WITH_ALL_OPTIONS} )
option( WITH_GLIB_1_2 "Enforce glib-1.2 support" OFF )
SlavekB commented 4 years ago
Owner

I'm still not very happy with this option. You notice that this is not used for ktechlab, but for gpsim as such. And here it is not possible to enforce the use of a different glib than the one with which gpsim was built. That is why enforcement looks strange in the ktechlab options, when ktechlab cannot enforce it. The problem is that there is probably no reasonable way to reliably determine which version of glib gpsim is built with.

I'm still not very happy with this option. You notice that this is not used for ktechlab, but for gpsim as such. And here it is not possible to enforce the use of a different glib than the one with which gpsim was built. That is why enforcement looks strange in the ktechlab options, when ktechlab cannot enforce it. The problem is that there is probably no reasonable way to reliably determine which version of glib gpsim is built with.
ConfigureChecks.cmake Outdated
@ -0,0 +47,4 @@
set( CMAKE_REQUIRED_INCLUDES ${GLIB_INCLUDE_DIRS} ${GPSIM_INCLUDE_DIRS})
else()
pkg_search_module( GLIB glib-2.0 )
tde_save( CMAKE_REQUIRED_INCLUDES )
SlavekB commented 4 years ago
Owner

These two rows can be pulled down bellow the condition because they are identical in both cases.

At the same time, there is missing verification that the glib search was successful.

These two rows can be pulled down bellow the condition because they are identical in both cases. At the same time, there is missing verification that the glib search was successful.
ConfigureChecks.cmake Outdated
@ -0,0 +54,4 @@
##### check for gpsim version
check_cxx_source_compiles( "
#include <gpsim/interface.h>
SlavekB commented 4 years ago
Owner

Please, can you indent the test code to make it easier to read, what is the test program code?

Please, can you indent the test code to make it easier to read, what is the test program code?
doc/CMakeLists.txt Outdated
@ -0,0 +1 @@
tde_auto_add_subdirectories( )
SlavekB commented 4 years ago
Owner

By the way, here it would be possible to use common cmake rules for documentation – such as in the abakus.

By the way, here it would be possible to use common cmake rules for documentation – such as in the abakus.
src/CMakeLists.txt Outdated
@ -0,0 +92,4 @@
)
install(
FILES x-circuit.desktop x-flowcode.desktop
SlavekB commented 4 years ago
Owner

Note that there is a Comment in these desktop files, which could also be translated. So it also seems appropriate to use tde_create_translated_desktop here.

Note that there is a `Comment` in these desktop files, which could also be translated. So it also seems appropriate to use `tde_create_translated_desktop` here.
src/core/main.cpp Outdated
@ -58,3 +58,3 @@
}
void exit_gpsim(int ret)
#ifdef HAVE_CONFIG_H
SlavekB commented 4 years ago
Owner

It is unusual for an include file to be hidden deep in the code instead of being at the beginning of the source file…

It is unusual for an include file to be hidden deep in the code instead of being at the beginning of the source file…
Ghost changed title from Conversion to the cmake building system. to WIP:Conversion to the cmake building system. 4 years ago
Ghost added the
PR/wip
 label 4 years ago
Ghost removed the
PR/wip
 label 4 years ago
Ghost changed title from WIP:Conversion to the cmake building system. to Conversion to the cmake building system. 4 years ago
SlavekB commented 4 years ago
Owner

I did a test on FreeBSD and it's not easy.

  1. There is an error with include "../config.h" in the gpsim include file.
  2. In src/electronics/port.cpp, there are Linux-dependent things that cause FTBFS on non-Linux systems.
I did a test on FreeBSD and it's not easy. 1. There is an error with `include "../config.h"` in the gpsim include file. 2. In `src/electronics/port.cpp`, there are Linux-dependent things that cause FTBFS on non-Linux systems.
Ghost commented 4 years ago
Poster
  1. There is an error with include "../config.h" in the gpsim include file.

Either the trouble is brought to mainstream gpsim Devs that they fix It or It is brought to the FreeBSD gpsim packager for him to patch.
Bothering ourself with this config file is not really an option and/or our very concern.

  1. In src/electronics/port.cpp, there are Linux-dependent things that cause FTBFS on non-Linux systems.

Out of curiosity, can you display the error? Maybe Akio can help too.

> 1. There is an error with `include "../config.h"` in the gpsim include file. Either the trouble is brought to mainstream gpsim Devs that they fix It or It is brought to the FreeBSD gpsim packager for him to patch. Bothering ourself with this config file is not really an option and/or our very concern. > 2. In `src/electronics/port.cpp`, there are Linux-dependent things that cause FTBFS on non-Linux systems. Out of curiosity, can you display the error? Maybe Akio can help too.
SlavekB commented 4 years ago
Owner
  1. There is an error with include "../config.h" in the gpsim include file.

Either the trouble is brought to mainstream gpsim Devs that they fix It or It is brought to the FreeBSD gpsim packager for him to patch.
Bothering ourself with this config file is not really an option and/or our very concern.

I did a workaround for it. At first I was afraid it would be ugly, but it looks acceptable. See recent commit.

  1. In src/electronics/port.cpp, there are Linux-dependent things that cause FTBFS on non-Linux systems.

Out of curiosity, can you display the error? Maybe Akio can help too.

First thing – Linux specific include: src/electronics/port.cpp#L17
Other resulting – linux ppdev specific ioctl codes, for example PPCLAIM: src/electronics/port.cpp#L438

> > 1. There is an error with `include "../config.h"` in the gpsim include file. > > Either the trouble is brought to mainstream gpsim Devs that they fix It or It is brought to the FreeBSD gpsim packager for him to patch. > Bothering ourself with this config file is not really an option and/or our very concern. > I did a workaround for it. At first I was afraid it would be ugly, but it looks acceptable. See recent commit. > > 2. In `src/electronics/port.cpp`, there are Linux-dependent things that cause FTBFS on non-Linux systems. > > Out of curiosity, can you display the error? Maybe Akio can help too. > > First thing – Linux specific include: [src/electronics/port.cpp#L17](src/branch/master/src/electronics/port.cpp#L17) Other resulting – linux ppdev specific ioctl codes, for example `PPCLAIM`: [src/electronics/port.cpp#L438](src/branch/master/src/electronics/port.cpp#L438)
SlavekB approved these changes 4 years ago
SlavekB left a comment
Owner

It looks good. The problem with Linux-dependent code goes beyond the scope of this PR, so it is not blocking for merging.

It looks good. The problem with Linux-dependent code goes beyond the scope of this PR, so it is not blocking for merging.
SlavekB merged commit 82104ef5be into master 4 years ago
SlavekB deleted branch feat/cmakeConv 4 years ago
SlavekB added this to the R14.0.10 release milestone 4 years ago

Reviewers

SlavekB approved these changes 4 years ago
The pull request has been merged as 82104ef5be.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
SlavekB
Labels
Apply labels
Clear labels   
GE/need-info

General - need additional info from contributor   
PR/keep-branch

Pull request - do not delete branch after merging   
PR/not-ok

Pull request - need fixing   
PR/rfc

Pull request - request for comments   
PR/update-trans

Pull request - update to translation files needed   
PR/wip

Pull request - work in progress   
RS/R14.0.x

Related to R14.0.x series   
RS/R14.1.x

Related to R14.1.x series   
SL/critical

Severity level - critical   
SL/major

Severity level - major   
SL/minor

Severity level - minor   
SL/normal

Severity level - normal   
SL/regression

Severity level - regression from previous version   
SL/trivial

Severity level - trivial   
SL/wishlist

Severity level - wishlist request   
ST/duplicate

Status - duplicate of another issue   
ST/invalid

Status - invalid report   
ST/notourproblem

Status - not our problem   
ST/rejected

Status - rejected   
ST/wontfix

Status - won't fix   
ST/worksforme

Status - works for me, unable to reproduce
No Label
GE/need-info
PR/keep-branch
PR/not-ok
PR/rfc
PR/update-trans
PR/wip
RS/R14.0.x
RS/R14.1.x
SL/critical
SL/major
SL/minor
SL/normal
SL/regression
SL/trivial
SL/wishlist
ST/duplicate
ST/invalid
ST/notourproblem
ST/rejected
ST/wontfix
ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.0.10 release
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: TDE/ktechlab#2
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'feat/cmakeConv'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes