Conversion to the cmake building system. #2

Merged
SlavekB merged 5 commits from feat/cmakeConv into master 1 year ago
cethyel commented 1 year ago
Collaborator

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:
cethyel added the
PR/wip
label 1 year ago
Poster
Collaborator

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 1 year 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:
Poster
Collaborator

@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?
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 */ ```
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 ```
Poster
Collaborator

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.
cethyel removed the
PR/wip
label 1 year ago
cethyel changed title from WIP:Conversion to the cmake building system. to Conversion to the cmake building system. 1 year ago
Poster
Collaborator

@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 1 year ago
SlavekB left a comment

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

CMakeLists.txt Outdated
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 1 year ago
Poster
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.
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 1 year ago
Poster
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.
##### check for gpsim version
check_cxx_source_compiles( "
#include <gpsim/interface.h>
SlavekB commented 1 year ago
Poster
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?
tde_auto_add_subdirectories( )
SlavekB commented 1 year ago
Poster
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.
)
install(
FILES x-circuit.desktop x-flowcode.desktop
SlavekB commented 1 year ago
Poster
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.
}
void exit_gpsim(int ret)
#ifdef HAVE_CONFIG_H
SlavekB commented 1 year ago
Poster
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…
cethyel changed title from Conversion to the cmake building system. to WIP:Conversion to the cmake building system. 1 year ago
cethyel added the
PR/wip
label 1 year ago
cethyel removed the
PR/wip
label 1 year ago
cethyel changed title from WIP:Conversion to the cmake building system. to Conversion to the cmake building system. 1 year 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.
Poster
Collaborator
  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.
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 1 year ago
SlavekB left a comment

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 1 year ago
SlavekB deleted branch feat/cmakeConv 1 year ago
SlavekB added this to the R14.0.10 release milestone 1 year ago

Reviewers

SlavekB approved these changes 1 year ago
The pull request has been merged as 82104ef5be.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.