Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

About window shows version 0.83 instead of 1.0.1 #4

Closed
ssuominengentoo opened this issue Mar 15, 2012 · 26 comments
Closed

About window shows version 0.83 instead of 1.0.1 #4

ssuominengentoo opened this issue Mar 15, 2012 · 26 comments

Comments

@ssuominengentoo
Copy link
Contributor

Fabian Köster reported here http://bugs.gentoo.org/show_bug.cgi?id=408321 that Pragha's About Window is showing version
0.83 instead of 1.0.1.

0.83 is the version number of libcdio...

@matiasdelellis
Copy link
Contributor

You can verify this?
Is very strange. Always worked well, and here it continues working properly

@ssuominengentoo
Copy link
Contributor Author

I looked into RPM from Fedora and Gentoo's libcdio package version 0.83 and both have this include file setting this:

dev-libs/libcdio-0.83 (/usr/include/cdio/cdio_config.h)

cdio_config.h:#define PACKAGE_VERSION "0.83"

Which might be the culprit? I looked config.log, build output with --disable-silent-rules, and a lot more and couldn't find any other explanation. Not sure how it gets picked up from there yet.

http://dev.gentoo.org/~ssuominen/pragha-1.0.1.png

@ssuominengentoo
Copy link
Contributor Author

Please see /usr/include/cdio/cdio_unconfig.h which should get added somewhere in cdda.h or pragha.h or ... I'm not sure which is the correct sequence. Please read some 10 first lines of this header file:

You can include this to remove any C preprocessor symbols created in cdio_config.h.
For example, if your project has your own config.h which sets symbols do this:

(Not pasting the rest here because github for some reason thinks I'm adding invisible text or something funny.)

(src/cdda.h includes 2 libcdio headers which then include the cdio_config.h and sets PACKAGE_VERSION from there... that needs later get wiped by cdio_unconfig.h which is now missing)

@matiasdelellis
Copy link
Contributor

Wow..
Thanks you Pavel and ssuominengentoo. ;)

You are genius!.
One by looking at the headers!, And the other by looking at the documentation!. =)

P.s: Sorry. I have not much time lately.

matiasdelellis added a commit that referenced this issue Mar 20, 2012
Fix issue #4
workaround for mess after including cdio
@matiasdelellis
Copy link
Contributor

:(

In fedora:
make[2]: se ingresa al directorio `/home/matias/Proyectos/Linux/pragha/src'
CC pragha-cdda.o
In file included from pragha.h:22:0,
from cdda.c:19:
cdda.h:26:32: error fatal: cdio/cdio_unconfig.h: No existe el fichero o el directorio compilación terminada.

See here: http://lists.gnu.org/archive/html/libcdio-devel/2012-01/msg00077.html

Any idea?
So.. Cosmetic issue vs not compilation? For now, I will undo the merge..

@ssuominengentoo
Copy link
Contributor Author

Fedora is still at 0.83 which is the last released version, as per:

http://pkgs.fedoraproject.org/gitweb/?p=libcdio.git;a=blob_plain;f=libcdio.spec;hb=HEAD

I guess add AC_CHECK_HEADER([cdio/cdio_unconfig.h]) and #ifdef it in the code to be compatible with the libcdio from version control? Just like the config.h is #ifdef'd already.

@matiasdelellis
Copy link
Contributor

Yes. In fedora 16 is still V0.82.
But in /usr/include/cdio/cdio_config.h no have any PACKAGE_VERSION define..

Maybe for this?
##another multilib fix; remove the architecture information from version.h
sed -i -e "s,%{version}.*$,%{version}\",g" include/cdio/version.h

@ssuominengentoo
Copy link
Contributor Author

rawhide is still at 0.83... not newer...
I looked into that sed and it doesn't remove the PACKAGE_VERSION. In fact, I extracted the SRPM from Fedora and the PACKAGE_VERSION is still there. I'm inclined to believe your libcdio is installed from some unofficial repository.
Not that any of this really matters, should be made to work with any :)

@ghost
Copy link

ghost commented Mar 20, 2012

We might remove line #include <cdio/cdio_unconfig.h>. Now our config.h included after cdio's and overrides PACKAGE_VERSION if any

@ssuominengentoo
Copy link
Contributor Author

Then it would fail to compile with an error like 'PACKAGE_VERSION already declared here.' if not unset. Can't #define twice. No?

@ghost
Copy link

ghost commented Mar 20, 2012

Maybe warning, but not error

@ssuominengentoo
Copy link
Contributor Author

With current git, and cdio_unconfig.h removed from cdda.h. Didn't fail, but looks pretty ugly:

./config.h:81:0: warning: "PACKAGE" redefined [enabled by default]
/usr/include/cdio/cdio_config.h:306:0: note: this is the location of the previous definition
../config.h:84:0: warning: "PACKAGE_BUGREPORT" redefined [enabled by default]
/usr/include/cdio/cdio_config.h:309:0: note: this is the location of the previous definition
../config.h:87:0: warning: "PACKAGE_NAME" redefined [enabled by default]
/usr/include/cdio/cdio_config.h:312:0: note: this is the location of the previous definition
../config.h:90:0: warning: "PACKAGE_STRING" redefined [enabled by default]
/usr/include/cdio/cdio_config.h:315:0: note: this is the location of the previous definition
../config.h:93:0: warning: "PACKAGE_TARNAME" redefined [enabled by default]
/usr/include/cdio/cdio_config.h:318:0: note: this is the location of the previous definition
../config.h:99:0: warning: "PACKAGE_VERSION" redefined [enabled by default]
/usr/include/cdio/cdio_config.h:324:0: note: this is the location of the previous definition
../config.h:105:0: warning: "VERSION" redefined [enabled by default]
/usr/include/cdio/cdio_config.h:352:0: note: this is the location of the previous definition

(And these same messages few hundred lines more... Didn't want to paste everything...)

@ssuominengentoo
Copy link
Contributor Author

Something like this could work but then cdda.h would need to include config.h heh :-P

--- pragha.orig/configure.ac 2012-03-20 16:07:46.314132265 +0200
+++ pragha/configure.ac 2012-03-20 16:07:55.830131497 +0200
@@ -73,6 +73,11 @@
XDT_CHECK_PACKAGE([LIBX11], [x11], 1.0.0)
XDT_CHECK_PACKAGE([TAGLIB], [taglib], [1.4])

+AC_CHECK_HEADER([cdio/cdio_unconfig.h],
+[

  • AC_DEFINE([HAVE_CDIO_UNCONFIG_H], [1], [Define to 1 if cdio_unconfig.h is found])

  • ], [])
    +
    dnl Check for taglib_c headers.
    TAGLIBC_LIBS=""
    TAGLIBC_CFLAGS=""
    diff -ur pragha.orig/src/cdda.h pragha/src/cdda.h
    --- pragha.orig/src/cdda.h 2012-03-20 16:07:46.318132264 +0200
    +++ pragha/src/cdda.h 2012-03-20 16:08:02.646130962 +0200
    @@ -23,7 +23,9 @@

    #include <cdio/cdda.h>
    #include <cdio/cd_types.h>
    +#ifdef HAVE_CDIO_UNCONFIG_H
    #include <cdio/cdio_unconfig.h>
    +#endif

    struct con_cdda_decoder {
    gchar cdda_buf[CDIO_CD_FRAMESIZE_RAW];

@matiasdelellis
Copy link
Contributor

Nop.
Rawhide is the future fedora 18 and fedora 17 is still alpha. So, fedora 16 is stable, and use libcdio 0.82

Well. Also extracted both SRPM, and seems that this issue are olny in cdio 0.83.
In cdio 0.83 tarball have the cdio_unconfig.h, that remove the PACKAGE_VERSION, but not have any cdio_config.h
In cdio 0.82 tarball have the cdio_config.h, but without any reference to PACKAGE_VERSION, and not have cdio_unconfig.h

  • We might remove line #include <cdio/cdio_unconfig.h>. Now our config.h included after cdio's and overrides PACKAGE_VERSION if any
    mmm.. With this, work in fedora 16 and cdio 0.82. The about dialog is correct, and still work cd playback.
  • Then it would fail to compile with an error like 'PACKAGE_VERSION already declared here.' if not unset. Can't #define twice. No?
    Do not know. : S

You can test it with 0.83?

@matiasdelellis
Copy link
Contributor

Yes.
I'd rather not have any waring, and I like your idea.

@ghost
Copy link

ghost commented Mar 20, 2012

@ssuominengentoo for #ifdef HAVE_CDIO_UNCONFIG_H we need include our config.h...

@ssuominengentoo
Copy link
Contributor Author

OK, because there is a #define in cdio_config.h that gets set if it's included we can do this:

http://dev.gentoo.org/~ssuominen/pragra-cdio-compat.patch

That should work for everyone. And without warnings.

@matiasdelellis
Copy link
Contributor

@ssuominengentoo for #ifdef HAVE_CDIO_UNCONFIG_H we need include our config.h... Yes.

You patch work here..
Will work on the version of git?

@ssuominengentoo
Copy link
Contributor Author

looks a lot has changed in git, for example there is no longer cdda.h
and once you switch that to cdio.h or sector.h, other errors keep popping up

so no, pragha won't compile with libcdio git with or without the patch

@ghost
Copy link

ghost commented Mar 20, 2012

cdda.h is part of pragha, isn't it?

@ssuominengentoo
Copy link
Contributor Author

pragha has src/cdda.h which includes cdio/cdda.h and that cdio/cdda.h is no longer part of libcdio in git

and it isn't that simple as more has changed too in libcdio git, I won't be doing the port to it. sorry :(

@ghost
Copy link

ghost commented Mar 20, 2012

Pardon :-)

@matiasdelellis
Copy link
Contributor

Well.. But with 0.83 and you patch work?

In the list of libcdio are talking about making two parallel versions, by the amount of changes that are adding. We should wait the next version, but for now we focus at 0.82 and 0.83.

Agree? when release the next version, we'll see.

EDIT: 0.84 to 0.83. haha

@ghost
Copy link

ghost commented Mar 22, 2012

So commit it.

@matiasdelellis
Copy link
Contributor

ok. ;)
Regards..

@ssuominengentoo
Copy link
Contributor Author

I don't see any problems with the About window version anymore, using pragha 1.1.1 and libcdio-paranoia-0.90 (Issue 46)

matiasdelellis added a commit that referenced this issue Jan 31, 2013
Looking examples seems that  <cdio/cd_types.h> no change.
And  <cdio/paranoia/cdio_unconfig.h> no exist.. No?????
Also test if issue #4 was fixed with libcdio 0.90
matiasdelellis added a commit that referenced this issue Jun 20, 2014
Fix issue #4
workaround for mess after including cdio
matiasdelellis added a commit that referenced this issue Jun 20, 2014
Looking examples seems that  <cdio/cd_types.h> no change.
And  <cdio/paranoia/cdio_unconfig.h> no exist.. No?????
Also test if issue #4 was fixed with libcdio 0.90
matiasdelellis added a commit that referenced this issue Jun 20, 2014
Fix issue #4
workaround for mess after including cdio
matiasdelellis added a commit that referenced this issue Jun 20, 2014
Looking examples seems that  <cdio/cd_types.h> no change.
And  <cdio/paranoia/cdio_unconfig.h> no exist.. No?????
Also test if issue #4 was fixed with libcdio 0.90
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants