Rockbox

  • Status Unconfirmed   Reopened
  • Percent Complete
    0%
  • Task Type Patches
  • Category Utils
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Uchida - 2008-09-06
Last edited by zagor - 2008-12-04

FS#9371 - create database application

Create database application.

1) At first FS9349 patch file (http://www.rockbox.org/tracker/task/9349?getfile=17401) apply.

2) After this task’s patch apply.

Because appropriating the patch to the source file of the latest version (svn. 18806)
patch file updates.

For the latest version (svn.18814), songdb_2.patch cannot applied, the patch file modified.

songdb was able to be made by using MinGW.

It is necessary to install MSYS.

sync for svn. 19026

update patch file.

changes:

  1. sync svn.19360.
  2. add -d (–rockbox-directory) option.

set Rockbox directory option.

  1. add -V (–version).

show version information option.

  1. -m (–mode) option add “append”.

append non registered files only (not checked deleted file) mode.

Because the patch of FS9349 was committed, the patch of FS9349 need not be appld.

P.S. Should I add to the patch the addition of my name to docs/CREDITS ?

Project Manager
zagor commented on 2008-12-16 10:34

Generally I think this is the right approach. utils/songdb is a better place than tools/database, and your songdb.c is more evolved than the database.c skeleton.

But there are a number of lines in this patch that I don’t see why you have changed. Could you please try stripping the patch to only include the changes which are needed?

Also, feel free to copy parts of tools/database/Makefile so it includes support for all codecs.

As for docs/CREDITS, normally patches don’t include this since it is a frequent cause of patch conflict. The committer adds it when he commits the patch.

Thank you for the comment.

Also, feel free to copy parts of tools/database/Makefile so it includes support for all codecs.

utils/songdb/Makefile has contained all supported codecs. (see line
if new codec increases, it is automatically
codec increases when it is a method of A, it is necessary to correct Makefile. Therefore, I did not do the correction Makefile.

As for docs/CREDITS, normally patches don’t include this since it is a frequent cause of patch conflict. The committer adds it when he commits the
understood.


sync svn.
The correction to erase warning when making songdb was excluded. About patch file  Correction for songdb to make it compile on PC   - apps/metadata/asf.c   - apps/apps/settings.h   - apps/metadata.c   - apps/tagcache.c   - apps/misc.c   - firmware/export/logf.h   - uisimulator/common/io.c

 Correction to make it compile even if “SIMULATOR” is not defined   - firmware/export/system.h   - firmware/export/thread.h   - firmware/include/file.h

 Correction to use language such as SJIS etc.   - firmware/include/rbunicode.h   - firmware/common/unicode.c

 Correction to have to change “Rockbox directory” dynamically   - apps/main.c   - apps/tree.c   - apps/tagcache.h   - apps/tagcache.c  In MinGW or Cygwin with -mno-cygwin option, if ch is signed char and ch < 0 (ch = 0×80 - 0xff) , isspace(ch) is doesn’t operate normally.   - apps/metadata/mp3.c   - apps/metadata/metadata_common.c   - apps/replaygain.c

Please comment when there is a correction that seems to be unnecessary it.

sync svn.
year change: 2008 → 2009.

Project Manager
zagor commented on 2009-01-12 19:21

Yoshihisa, are you building this for win32 in some other environment than cygwin? That would explain some of the changes I find “unnecessary”.

I am using Cygwin and MinGW (+msys) for Win32.

sync svn. 20227.

sync r20281

I admit I’d like to see this patch in, but it has quite some necessary and unrelated changes, it seems to me.

For example, the chanes to uisimulator/common/io.c, may you can explain why this file is needed at all for the database tool? The renaming of the DATABASE_FILE etc things make the patch bigger and harder to read, without serving a purpose (they still point to the database *file*, right? so why call it _PATH_ all the way?).

I don’t quite understand the “#ifdef WIN32” to “#ifdef _WIN32” too, but I haven’t really looked into this generally.

I checked needlessness corrections.

I left corrections that changed “WIN32” into “_WIN32” (in
is because build warnings increases though can not changed to “_WIN32” by deleting “-std=c99” from “GCCOPTS” (in utils/songdb/Makefile). (It is necessary to correct to “_WIN32” because “WIN32” is not defined in MinGW’s gcc when “-std=c99” is in “GCCOPTS”.) changes: - sync r20619 - The correction to uisimulator/common/io.c was reduced. - For apps/tagcache.c, TAGCACHE_FILE_* returned to the state before it was corrected. (do not use TAGCACHE_PATH_*) - readme.txt: The description of options update.

Sorry, but I don’t understand why uisimulator/ files are even compiled in. The same goes for firmware/ files.

The attached patch implements your songdb tool using the existing external database tool (using the old Makefile), vastly reducing the number of changes needed.

Only tested on linux so far.

PS: This tool is apparently unable to build the database for an actual player, since the pathes don’t match the Rockbox’ ones on the player. I didn’t understand all the directory parameters though, maybe I’m doing anything
What’s “-m commit” used for? It appears to be unused entirely.

The patch file was reviewed further.

changes: - apps/metadata.c, apps/metadata/metadata_common.c do not change. - uisimulator/common/io.c use WIN32. (does not use _WIN32) and corrections not necessary to build the songdb was deleted. - For utils/songdb/Makfile, the options of GCCOPTS was made gradual and songdb.d does not create. - The songdb was able to be built by gcc 4 on Cygwin. - -m commit deletes.

About corrected
Correction necessary to build the




firmware/include/rbunicode.h

2) Correction to build the songdb without defining



firmware/include/file.h

3) Correction to build the songdb without using SDL
uisimulator/common/io.c

4) Include files that exists in Rockbox and Linux (Cygwin, MinGW) uses files that exists in Linux (Cygwin,
neither the function nor the function are enough by files that exist in
files written by <...> uses the one that exists in Linux (Cygwin, MinGW).

Files corrected by the above-mentioned reason is as follows. -


firmware/export/logf.h

5) Correction for the songdb build by MinGW to operate
apps/metadata/mp3.c

You seem to have ignored my patch and my questions...

I corrected your patch.


I corrected that the database tool was able to be build on Cygwin and
When optional -c >ISO-8859-8 | ISO-8859-11 | CP1256 | SJIS | GB-2312 | KSX-1001 | BIG5> was specified, the database tool works well. - When optional -d was specified, the database tool works
The option “-m create” deletes. - mutex_init(), mutex_lock(), mutex_unlock(), thread_sdl_thread_lock(), thread_sdl_thread_unlock() deletes form database.c

=> I corrected that these functions do not use in io.c. 

- fprintf() → printf()

=> I do not change stdio.c. Because stdin, stdout, and stderr do not defined on Cygwin and MinGW. (It is because of use for which stdio.h exists in the Rockbox. ) 

- I corrected that SDL include files was not used.

The following corrections were not done. Please let me hear your opinion.

1) I think that Makefile should not define
the database tool is not a simulator.

2) Because functions is insufficient only in the library that exists in Rockbox, the system library uses the one that exists in Linux, Cygwin, and MinGW.

When these are corrected, I will do the same correction as my patch file to your patch.

Nice.

I agree that SIMULATOR should not be defined.

I don’t quite understand your second question. But yes, we should be using the system’s header for standard functions, not the Rockbox’ ones (because our target plattform is the PC, not Rockbox).

My patch was corrected further.


The method of building the songdb changed. (When the sondb.c compiles, include files of the build environment is used. And other source files uses the Rockbox’s include
Change _ANSIDECL_H_ → _ANSI_DECL_H_ in
Cygwin with gcc 4, correction to prevent compilation of io.c from
I corrected that the dependence file of source files when the songdb is
the songdb is not updated when only header files was corrected.)

Moreover, rockbox-songdb_2.diff was corrected.
I did the same correction as the correction to songdb_13.patch. - I correctes that SIMULATOR does not define.

There are some questions. 1) How do you think though I think that the database tool should store on “utils” directory
Because “tools” directory is the directory that stores tools necessary to build the Rockbox firmware, codecs and, plugins.

2) How shall I do the name of the database tool ? database or songdb. or change other name
I think that the name of “database” is too general. So, I think that I should change the name of the database tool to another name. For example, songdb, musicdb, dbcreater etc.

I don’t really care in which folder it is or how we’re naming it. I just thought basing it off the existing database tool was a good idea to implement your patch with least changes needed.

Hmm, this looks pretty OK to me now.

Edit: What’s the difference between the two versions now?

The difference between songdb_13.patch and rockbox-songdb_3.diff is as
Directory that stored the database tool.

 songdb_13.patch: utils/songdb
rockbox-songdb_3.diff: tools/database

2) database tool name

 songdb_13.patch: songdb
rockbox-songdb_3.diff: database

Only the above-mentioned two points are different parts.


											

Can it build a database on the target?

Last time I tried it couldn’t. Rockbox’ paths are different. But also endianess needs to be taken into account.

Once it can create the database on the target (possibly through a special mode which will use rockbox’s paths), and once the GSOC database project is cleared up, it probably going to commit this.

Fine, I compiled this on windows using mingw32-make and powershell (no need for msys/sh/cygwin).

I could also create a database, however, there seemed to issues with creating a database off a song dir which is on another partition.

It corrected the database tool based on your comment as follows.

1) I added the option that specified the endian of the database. -b (–big-endian): for big
(–little-endian): for little endian.

2) Makefile was corrected like defining “ROCKBOX_BIG_ENDIAN” or “ROCKBOX_LITTLE_ENDIAN” according to an actual build environment.

But the database can not save in the place different from the Rockbox directory.

I confirmed whether the songdb was able to be made without installing msys (only MinGW + PowerShell). Because grep, sed, date do not exist on MinGW(These are installed by msys packages.), I cannot build the songdb.

Ah ok, I do have msys installed, but I typed make in the powershell, so I probably didn’t notice it used those tools.

Nice job about the endianess. So the remaining issue is the database for the actual target?

EDIT: How do you apply your patches on Windows? I couldn’t do it. I only made it by actually applying your patch on a linux system, creating a svn diff. The svn diff can be applied using tortoise. Patch (which comes with the svn cli client) for windows failed due to the line endings.

EDIT2: What are the changes you made to tagcache.c? Please do not do further changes that are unrelated to the songdb application. Keep the changes to a minimum and put unrelated changes to a separate task.

Uhm, sorry if I was unclear, but I actually meant to create the database in little or big endian, not to tell the tagcache which endianess was used (it will notice himself and correct it). I think we just ignore the endianess for now.

I’m still somewhat unconfortable with the changes to io.c. The old tool doesn’t need them. If you can explain why these are needed, I maybe feel better.

The other thing: database.h is a HACK. We want to include the tagcache as a (static) library. And thus we include tagcache.h. I know (played around with it myself) that it is troublesome, but database.h is basically just doing our own prototyping. I really don’t like that.

A lot of corrections to my patch. I think that changes in the functions becomes it at the end. changes: - sync r20712. (Please use the source since r20710.) - the songdb include codepage table data. (the songdb does not use codepages/*.cp.) - -i (--input-path) option add. - -o (--output-path) option add. - -d (--rockbox-directory) option deletes. (This option became unnecessary because of the above mentioned change. ) - I changed describing the definition of the songdb from Makefile to config-pctool.h. - config.h includes config-pctool.h - firmware/export/thread.h does not change. The correction of rockbox-songdb_vc_5.diff is made as soon as the confirmed of the songdb_15.patch is completed.

More comment add. About tagcache.c/h For songdb_13 -> songdb_14, tagcache.c was corrected as follows. 1) tagcache_init(void) -> tagcache_init(int endian) when __PCTOOL__ is defined. Should I newly make tagcache_set_endian() without changing tagcache_init(), and specify the endian of the database? 2) I corrected that whether database_idx.tcd was saved by the same endian as the specified endian was checked when it opened. For songdb_14 -> songdb_15. 1) I change the functions that database files open or delete. (open() -> db_open(), remove() -> db_remove().) It is a correction because it can dynamically change the directory saving of the database. db_open() and db_remove() are implemented by songdb/dbfile.c. 2) I corrected that an unnecessary header files was not included in tagcache.c. I confirm the tagcache.c again though I do not correct the above mentioned. About unicode.c. Until the songdb_14.patch, to convert the character code, codepage files that existed in the codepages directory had been used. For the songdb_15.patch, I corrected that codepages/*.cp do not used to convert the character code. The Rockbox directory need not exist when the songdb is executed by this correction. About config.h/config-pctool.h I corrected that definitions necessary to make the songdb was described in the config-pctool.h and the config.h read it. This modification is mimicked to read the config-*.h when the Rockbox firmware or simulator is maked . The problem might still remain though I confirmed operation on Cygwin, MinGW and Linux.

a) What's logf needed for? b) Remove the endianess thing please, the database will handle it without us. Unless we can change the endianess before creating, just telling the database which endianess the database is in doesn't make sense. c) Please get including tagcache.h to work. This file gives us the prototypes. Please don't use hacks like "extern void tagcache_init()".

a) I am using it as an output for debugging now. when the songdb will release, I don't use logf.c because it is unnecessary. b) Because the database is normally read even if the endian of the database is different, endian change options (-b/-l) will delete. c) tagcache_init() will return to original state. When the above mentioned correction ends, I will upload the correction version patch file.

Patch files updates. changes: - logf.c does not used. - -b/-l options delete. - tagacache_init() returns to original state. - stdio.h does not change.

sorry, rockbox-songdb_5.diff is wrong. changes: add database.h delete stdio.h, thread.h

sorry, rockbox-songdb.vc_6.diff is very wrong. I upload further more.

Why is Rockbox code pages not used anymore? Doesn't that create possible incompabilities? What's bad about the rockbox' codepages. Also, you re-create some file functions, what's the need for io.c then? The #defines in config-pctool look a bit weird imo. Please, also note that this database tool isn't the only __PCTOOL__, e.g. checkwps and the wps editor are __PCTOOL__ too.

I think that io.c is necessary. io.c is used to solve the following problems. 1) convert path. (pc path <-> Rockbox path) 2) convert character code used path. (For MinGW environment) 3) absorption of difference of build environment. The file that takes out only a necessary function of io.c . However, I think that I would rather use io.c than make such a file. If this is done to io.c, I should take out only the part where tagcache.c (and other sources) is necessary. I think that it is time-consuming to make the database tool without using the functions of Rockbox if this is done. changes: - bug fix: I correct the codepage.c. - does not use config-pctool.h.(#define write in Makefile.)

sorry, because an unrelated file had mixed, it uploads again.

I made the version that did not use uisimuator/common/io.c. There is a possibility that an unnecessary part remains still. And it doesn't test enough.

My patch file corrects. changes: - bug fix: I corrected that the songdb can be made on Linux. I will make the correction version of rockbox-songdb_vc_8.diff if there is no problem.

a) You don't need to put #ifdef's around cpu_boost(), it's a macro on __PCTOOL__ anyway. b) You didn't tell me why you use external codepages.

comment thanks. a) I corrected that I don't put #ifdef's around cpu_boost(). b) Becase it becomes unnecessary codepage files (*.cp) file. And it is possible to make the database by this even in the environment in which Rockbox is not installed.

"And it is possible to make the database by this even in the environment in which Rockbox is not installed." - I don't understand this. You'll always need the rockbox source, don't you? I just fear, that using different codepages, the database created by songdb might get incompatible to the Rockbox' one. I think it should be possible for the songdb to compile the codepages in, and if not, .cp files aren't too bad imo.

"And it is possible to make the database by this even in the environment in which Rockbox is not installed.": It means any file included in .rockbox directory though the songdb operates is not needed. (Of course, Rockbox sources is necessary to make the songdb.) The Rockbox and the songdb are the same codepage table data. codepage/iso.cp is created by using iso_decode() in tools/codepages.c. And in the songdb, iso codepages (iso8859-*, WIN*) data are created by using iso2ucs() in utils/songdb/codepage.c. But because iso_decode() and iso2ucs() are functions that do the same processing, codepage data is not different at all. The reason to make iso2ucs() newly is that iso_decode() function exists in tools/codepages.c and firmware/common/unicode.c. Because iso2ucs() becomes unnecessary if the function name of iso_decode() (in tools/codepages.c) is changed, it is not happened that the codepage data is different. Is it good in such a correction?

My patch updates. changes: -- deletes iso2ucs() from utils/songdb/codepage.c -- name change iso_decode() -> iso2ucs() from tools/codepgaes.c. -- use codepages.c Any codepage data is not different in Rockbox and the songdb.

sync. r21437

Hi, i just recently noticed that the tool now corrupts the database on the rockbox device (songdb.exe compiled in june 2009). Trying to apply this patch to the current rockbox code in order to compile a new .exe fails with a bunch of "HUNK Failed" messages. Any chance of seeing an updated version.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing